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

["Laszlo Ersek" <lersek@redhat.com>]

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.

Thanks!
Laszlo