Re: [PATCH 3/4] OvmfPkg: create a SEV secret area in the AmdSev memfd


Laszlo Ersek
 

On 11/20/20 07:29, James Bottomley wrote:
On Thu, 2020-11-19 at 13:41 -0600, Brijesh Singh wrote:
On 11/19/20 1:50 AM, Laszlo Ersek wrote:
On 11/18/20 21:23, James Bottomley wrote:
On Mon, 2020-11-16 at 23:46 +0100, Laszlo Ersek wrote:
On 11/12/20 01:13, James Bottomley wrote:
[... I made all the changes above this]
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7..7d3214e55d 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -35,6 +35,8 @@ ALIGN 16
; the build time RIP value. The GUID must always be 48
bytes
from the
; end of the firmware.
;
+; 0xffffffc2 (-0x3e) - Base Location of the SEV Launch
Secret
+; 0xffffffc6 (-0x3a) - Size of SEV Launch Secret
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
@@ -51,6 +53,8 @@ ALIGN 16
TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB
0

sevEsResetBlockStart:
+ DD SEV_LAUNCH_SECRET_BASE
+ DD SEV_LAUNCH_SECRET_SIZE
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
(5) I'd prefer if we could introduce a new GUID-ed structure
for these new fields. The logic in QEMU should be extended to
start scanning at 4GB-48 for GUIDS. If the GUID is not
recognized, then terminate scanning. Otherwise, act upon the
GUID-ed structure found there as necessary, and then determine
the next GUID *candidate* location by subtracting the last
recognized GUID-ed structure's "size" field.
So for this one, we can do it either way. However, the current
design of the sevEsRestBlock is (according to AMD) to allow the
addition of SEV specific information. Each piece of information
is a specific offset from the GUID and the length of the
structure can only grow, so the ordering is fixed once the info
is added and you can tell if the section contains what you're
looking for is present if the length covers it.

We can certainly move this to a fully GUID based system, which
would allow us to have an unordered list rather than the strict
definition the never decreasing length scheme allows, but if we
do that, the length word above becomes redundant.
Well, GUIDed structs in UEFI/PI are sometimes permitted to grow
compatibily, and for that, either a revision field or a size field
is necessary / used. I kind of desire both here -- it makes sense
to extend (for example) the SEV-ES reset block with relevant
information, and to add other blocks of information (identified
with different GUIDs).

Basically I wouldn't want to finalize the SEV-ES AP reset block
just yet, *but* I also think this new information does not beloing
in the SEV-ES *AP reset block*. The new info is related to SEV-ES
alright, but not to the AP reset block, in my opinion. If you read
the larger context (the docs) in the assembly source around
"sevEsResetBlockStart", the launch secret just doesn't seem to fit
that.

I don't have a huge preference for either mechanism ... they seem
to work equally well, but everyone should agree before I replace
the length based scheme. I agree we should all agree about it
first.
And, to reiterate, I'd like to keep both the length fields and the
GUID-ed identification. In other words, a GUID should not imply an
exact struct size, just a minimum struct size.
I agree with the GUID based approach, it aligns well with the future
needs. Looking forwardm we will need to reserve couple of pages
(secret and cpuid) for the SNP. In my WIP patches I extended reset
block to define a new GUID for those new fields.

https://github.com/AMDESE/ovmf/commit/87d47319411763d91219b377da709efdb057e662#diff-0ca7ec2856c316694c87b519c95db3270e0cac798eb09745cce167aad7f2d46dR28

And I am using this qemu patch to iterate through all the GUIDs and
call the corresponding callbacks.

https://github.com/AMDESE/qemu/commit/16a1266353d372cbb7c1998f27081fb8aa4d31e9
OK, if that's not yet upstream, I think we should do this properly:
that means having a guid and length that identifies the entire table
and then all the incorporated guids and lengths. That way we don't
have a double meaning for the reset block guid as both identifying the
start of the table and the reset vector data. Also it means we don't
need a zero guid to signal the end of the table. And also means the
reset block GUID doesn't have to always be present (if it got
deprecated for some reason).

However, the downside is that you'll have to pull out the table by this
new guid at 0xffffffd0 and its length and then iterate over the table
to find the reset block guid ... but that will make it very easy to add
the additional guids.
I agree with doing things properly.

Thanks
Laszlo

Join devel@edk2.groups.io to automatically receive all group messages.