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

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

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