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

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

On 11/24/20 20:05, James Bottomley wrote:
> On Mon, 2020-11-23 at 23:16 +0100, Laszlo Ersek wrote:
>> On 11/20/20 19:45, James Bottomley wrote:

>>> 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://edk2.groups.io/g/devel/message/67621
>>   
>> https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00781.html
>>
>> takes this into account, and only prepends 0 to 31 bytes (inclusive):
>>
>>   TIMES (31 - (guidedStructureEnd - guidedStructureStart + 31) % 32)
>> DB 0

[...]

> I get the mathematics, but I'm a bit hazy on the why.  I structured my
> patch on the basis that some zero padding seems necessary (which does
> mean you have to add extra zeros when the structure fits exactly into
> 32 bytes).  If we don't need any zero pad at all then I agree with what
> you say above, we should use the shortest feasible pad.

The outermost table footer reports the cumulative size, so a
zero-terminator at the front is not needed.


> My other question is why 32 above?  If the object is simply to push the
> table guid to 0xffffffd0 in the OVMF.fd then 
> 
> TIMES (15 - ((guidedStructureEnd - guidedStructureStart + 15) % 16)) DB 0
> 
> or
> 
> TIMES 16 - ((guidedStructureEnd - guidedStructureStart) % 16)) DB 0
> 
> Depending on whether we need zeros or not, always works because the
> code is align 16 not align 32.

Right, I think you are correct.


>> (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.
> 
> Actually, no ... or at least not unless we alter the reset vector.  The
> problem is the reset vector is actually:
> 
>     nop
>     nop
>     jmp     EarlyBspInitReal16
> 
> But that jmp is 16-bit code, meaning the relative jump which goes over
> the table is at most -32768, so the table can never be larger than
> about 32k bytes.

Great catch! :)


>> (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.
> 
> Well we have to be consistent, so if you're thinking backwards it's
> header and end, but the other way around it has to be footer and
> beginning, so I updated it.

OK, thanks. I guess I should be OK with either interpretation, as long
as we'll have considered it from a consistency perspective.

>> (9) The 16-bit size field of the SEV-ES reset block structure is not
>> documented.
> 
> Well, it is ... it's become part of the table structure, so it's
> documented at the top of the table as:
> 
> ; The table is composed of structures with the form:
> ;
> ; Data (arbitrary bytes identified by guid)
> ; length from start of data to end of guid (2 bytes)
> ; guid (16 bytes)
> 
> All the comment is doing is describing the layout of the data.
> 
> I can add a length here if you like, but I'd probably better add one to
> the secret table as well to be consistent.

Ahh wait, no, now I understand it.

When I wrote my previous comment, I didn't understand why you listed all
of the fields explicitly, *except* the size. In particular, why you
listed the GUID (*unlike* the size), in spite of the GUID *too* being
pre-declared (*like* the size).

But now I understand -- you didn't re-declare the GUID *field* for this
particular GUIDed structure, you only provided the *value* of the GUID
field.

Perhaps juggling the words could improve understanding:

GUID ("SEV-ES reset block"): 00f771de-1a7e-4fcb-890e-68c77e2fb44e

But, it's up to you. I agree the listing is consistent the way you proposed.

Thanks!
Laszlo