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

[Brijesh Singh <brijesh.singh@amd.com>]

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


>
> Thanks!
> Laszlo
>