[edk2-devel] [PATCH v2 3/6] OvmfPkg: convert ES Reset Block structure to be guided
James Bottomley
jejb at linux.ibm.com
Tue Nov 24 19:05:13 UTC 2020
On Mon, 2020-11-23 at 23:16 +0100, 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.
> >
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> > Signed-off-by: James Bottomley <jejb at 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)
Done.
> > 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
>
> - 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
> ^^^ ^
> ^^
> | |
> | c
> ompensate
> | i
> n the
> | r
> emainder
> |
> 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.
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.
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.
> > +
> > +; 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)
Yes, I think header because it's what I come to first traversing
backwards but perhaps footer is better because most people don't think
backwards.
> > +; (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.
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.
That's not to say we can't have a larger table ... we definitely can,
but it can't be where it is now. we'd have to do something different
like make the table guid describe where the actual table is located (as
a 32 bit offset and length) so as not to break up the 16 bit jump code.
> > +;
> > +; 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"
done.
> > +; 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.
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.
> > +;
> > +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?)
Yes, current QEMU upstream has no support yet for searching for this
table although I think patches have been sent.
> (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.
> > ;
> > ; 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)
done.
Regards,
James
> > +; 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67903): https://edk2.groups.io/g/devel/message/67903
Mute This Topic: https://groups.io/mt/78455899/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list