Re: [PATCH v2 3/6] OvmfPkg: convert ES Reset Block structure to be guided

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 11/23/20 4:16 PM, Laszlo Ersek wrote:
> On 11/20/20 19:45, James Bottomley wrote:
>> Convert the current ES reset block structure to an extensible guid
>> based structure by appending a header and length, which allow for
>> multiple guid based data packets to be inserted.

I was wondering if this patch should be submitted outside of this series? 
I'm not sure it makes any difference, other than being able to be merged 
(possibly) quicker and independent of the series. Then this series simply 
takes advantage of it.

Thanks,
Tom

>>
>> Ref: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3077&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cd9de2a47d7d74b94e04208d88ffd6f03%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637417665948466692%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=x%2F%2Fbn5J9dNcXZqSs8Ui0GgABg6XDbmx%2BUh285EqwLcA%3D&reserved=0
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>
>> ---
>>
>> v2: added
>> ---
>>   OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 49 +++++++++++++++-----
>>   1 file changed, 38 insertions(+), 11 deletions(-)
> 
> (1) Please update the subject line to:
> 
> OvmfPkg/ResetVector: convert SEV-ES Reset Block structure to be GUIDed
> 
> - edk2 prefers including module names too in the patch subjects
> - "ES" is harder to understand than "SEV-ES"
> - "GUIDed" is harder to misread as "guided"
> - subject length is still OK (70 chars)
> 
> 
>>
>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>> index 980e0138e7fe..baf9d09f3625 100644
>> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>> @@ -25,21 +25,40 @@ ALIGN   16
>>       TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0
>>   %endif
>>
>> +;
>> +; Padding to ensure first guid starts at 0xffffffd0
>> +;
>> +TIMES (32 - ((guidedStructureEnd - guidedStructureStart) % 32)) DB 0
> 
> (2) This will insert 32 zero bytes if the size is already aligned to 32
> bytes (because 32-0 = 32). In other words, the above produces 1 to 32
> zero bytes, dependent on table size.
> 
> The variant I proposed in point (5) at
> 
>    https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F67621&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Cd9de2a47d7d74b94e04208d88ffd6f03%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637417665948466692%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nLOZvHpzFMRLV6uEeQvETY1SqI4AaSRc92WYbz8r9cA%3D&amp;reserved=0
>    https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-November%2Fmsg00781.html&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Cd9de2a47d7d74b94e04208d88ffd6f03%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637417665948466692%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=yMYv%2BKKFtCs9G7qgdheAVmS2iLexxJWjbgSTTFcCetM%3D&amp;reserved=0
> 
> takes this into account, and only prepends 0 to 31 bytes (inclusive):
> 
>    TIMES (31 - (guidedStructureEnd - guidedStructureStart + 31) % 32) DB 0
> 
> - This variant subtracts 1 inside the remainder operation (which is
>    expressed as adding 31).
> 
> - For compensation, it adds 1 just outside of the remainder operation.
>    This addition in effect increases the subtrahend for the leftmost 32.
>    Therefore this (-1) addend is ultimately folded into the leftmost 32,
>    yielding 31 on the leftmost side.
> 
>    TIMES (32 - ((guidedStructureEnd - guidedStructureStart - 1) % 32 + 1)) DB 0
>                                                            ^^^       ^^^
>                                                            |         |
>                                                            |         compensate
>                                                            |         in the
>                                                            |         remainder
>                                                            |
>                                                            slide down residue
>                                                            class modulo 32
> 
> 
>   TIMES (32 - ((guidedStructureEnd - guidedStructureStart + 31) % 32) - 1) DB 0
>                                                           ^^^^        ^^^
>                                                           |           |
>                                                           |           unnest
>                                                           |           increment
>                                                           |           from
>                                                           |           subtrahend
>                                                           |
>                                                           express modular
>                                                           subtraction as
>                                                           addition, to avoid
>                                                           using % on a negative
>                                                           integer (in case size
>                                                           were 0)
> 
>   TIMES (31 - ((guidedStructureEnd - guidedStructureStart + 31) % 32)) DB 0
>          ^^
>          |
>          fold previous (-1) addend into leftmost constant
> 
> - This juggling of 1 results in no changes for residue classes 1 through
>    31, but wraps the outermost result (the padding size) for residue
>    class 0, from 32 to 0.
> 
> 
>> +
>> +; Guided structure.  To traverse this you should first verify the
>> +; presence of the table header guid
> 
> (3) suggest "table footer GUID" (the GUID follows the data, in address
> order)
> 
>> +; (96b582de-1fb2-45f7-baea-a366c55a082d) at 0xffffffd0.  If that
>> +; is found, the two bytes at 0xffffffce are the entire table length.
> 
> (4) can we make the whole table size field 32-bit? I don't have a
> particular use case in mind, it just looks more extensible than 16-bit.
> We can still keep the individual structs we have in mind 16-bit sized.
> 
>> +;
>> +; The table is composed of structures with the form:
>> +;
>> +; Data (arbitrary bytes identified by guid)
>> +; length from start of guid to end of data (2 bytes)
> 
> (5) This is hard to interpret, as "data" precedes "guid" in address
> space (guid is a footer, not a header).
> 
> I suggest "length from start of data to end of GUID"
> 
> 
>> +; guid (16 bytes)
>> +;
>> +; so work back from the header using the length to traverse until you
> 
> (6) suggest "from the footer"
> 
> 
>> +; either find the guid you're looking for or run off the end of the
>> +; table.
> 
> (7) suggest "run off the beginning of the table"
> 
> ... I realize "start" and "end" can be interpreted temporally and
> spatially. In a forward traversal they are the same, but now they
> aren't. I suggest we use the spatial (address space order)
> interpretation.
> 
>> +;
>> +guidedStructureStart:
>> +
>>   ;
>>   ; SEV-ES Processor Reset support
>>   ;
>>   ; sevEsResetBlock:
>>   ;   For the initial boot of an AP under SEV-ES, the "reset" RIP must be
>> -;   programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A known offset
>> -;   and GUID will be used to locate this block in the firmware and extract
>> -;   the build time RIP value. The GUID must always be 48 bytes from the
>> -;   end of the firmware.
>> +;   programmed to the RAM area defined by SEV_ES_AP_RESET_IP. The data
>> +;   format is
>>   ;
>> -;   0xffffffca (-0x36) - IP value
>> -;   0xffffffcc (-0x34) - CS segment base [31:16]
>> -;   0xffffffce (-0x32) - Size of the SEV-ES reset block
>> -;   0xffffffd0 (-0x30) - SEV-ES reset block GUID
>> -;                        (00f771de-1a7e-4fcb-890e-68c77e2fb44e)
>> +;   IP value [0:15]
>> +;   CS segment base [31:16]
>> +;
>> +;   SEV-ES reset block GUID: 00f771de-1a7e-4fcb-890e-68c77e2fb44e
> 
> (8) Did I understand from the v1 discussion that the corresponding QEMU
> parser is not upstream yet? (Or at least not released?)
> 
> (9) The 16-bit size field of the SEV-ES reset block structure is not
> documented.
> 
> 
>>   ;
>>   ;   A hypervisor reads the CS segement base and IP value. The CS segment base
>>   ;   value represents the high order 16-bits of the CS segment base, so the
>> @@ -48,8 +67,6 @@ ALIGN   16
>>   ;   program the EIP register with the IP value as read.
>>   ;
>>
>> -TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0
>> -
>>   sevEsResetBlockStart:
>>       DD      SEV_ES_AP_RESET_IP
>>       DW      sevEsResetBlockEnd - sevEsResetBlockStart
>> @@ -57,6 +74,16 @@ sevEsResetBlockStart:
>>       DB      0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E
>>   sevEsResetBlockEnd:
>>
>> +;
>> +; Table header: length of whole table followed by table header
> 
> (10) I suggest "table footer" (twice)
> 
> 
>> +; guid: 96b582de-1fb2-45f7-baea-a366c55a082d
>> +;
>> +    DW      guidedStructureEnd - guidedStructureStart
>> +    DB      0xDE, 0x82, 0xB5, 0x96, 0xB2, 0x1F, 0xF7, 0x45
>> +    DB      0xBA, 0xEA, 0xA3, 0x66, 0xC5, 0x5A, 0x08, 0x2D
>> +
>> +guidedStructureEnd:
>> +
>>   ALIGN   16
>>
>>   applicationProcessorEntryPoint:
>>
> 
> Thanks!
> Laszlo
>