Re: [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
On 05/07/21 22:38, Brijesh Singh wrote:
From: Tom Lendacky <thomas.lendacky@...>(1) The actual section reference is missing. I'll fix it up: from where
the spec introduces exit code 0x8000_0013, the sections appear to be
4.1.9 and 4.3.2. Also, Table 5. "List of Supported Non-Automatic Events"
is relevant for the SVM_VMGEXIT_SNP_AP_* macros.
(2) I think "area" (singular) is correct here, so we should say "is".
I'll update it.
creating the AP. The save area format is defined in AMD APM volume(3) there should be a space character between "pack" and "(" -- two
+(4) ... I guess I can't go ahead merging this myself, after all (Liming
may of course still merge the MdePkg patches, if he wants to).
My problem here is that the bit positions are cryptic.
I've found the *normal* (not SEV-ES) segment descriptor attributes in
the AMD APM (publication #24593, revision 3.37, date March 2021, volume
2, sections sections 4.7 and 4.8).
However, the bit positions SEV-ES descriptors are surely different. For
the "normal" segment descriptors, we already have the
IA32_SEGMENT_DESCRIPTOR type in edk2, with the nicely broken-out
attribute bits. The bit meanings within
"SEV_ES_SEGMENT_REGISTER.Attributes" remain unclear to me.
Please at least provide a *specific* documentation reference in the
commit message where I can verify (or at least "decode") the attribute bits.
+(5) This doesn't seem right. The comment higher up says that "Only the
fields required to be set to a non-zero value are defined", which is
fine. But in Table B-4, between fields "Tr" and "Vmpl", we have 5 qword
fields (PL0_SSP through PL3_SSP, plus U_CET), and a reserved dword
field. That makes for 5*8+4 = 44 bytes, not 42.
Hmmm. If I look at the table differently, I get a different result.
Namely, the first offset right after Tr is 0A0h, and the start offset of
Vmpl is 0CAh. The difference is indeed 42 (decimal).
Ah, I've found it. The bug is in the spec. The Reserved field at offset
0C8h is listed with size "dword", but the field right after it, namely
VMPL, starts at offset 0CAh -- that is, only two bytes later. Which
means that the "dword" size for Reserved is wrong; it should be "word" only.
I couldn't find a more recent release of the APM than "revision 3.37".
Please add a remark to the commit message on this patch that highlights
this typo in the spec.
+ UINT8 Vmpl;OK
+ UINT64 Cr4;OK (although I'm not sure much is saved over spelling out "Cr3")
+ UINT64 Cr0;OK
+ UINT64 GPat;OK
+ UINT64 SevFeatures;OK
+ UINT64 XCr0;OK
+ UINT32 Mxcsr;(6) If you are packing all four words (X87_FTW, X87_FSW, X87_FCW,
X87_FOP) into a qword, then please give the latter a different name. The
spec associates X87_FTW with just the word at offset 40Ch. I propose the
name "FpState" for the UINT64 field in the edk2 structure.
+ UINT64 Reserved9;(7) Ugh, wait, something's wrong -- you *are* apparently adding a field
for X87_FCW separately! But then the type of X87Ftw is wrong -- it
should be UINT16. Same for X87Fcw.
The distance between them is also wrong, it should be only 2 bytes
(basically the X87_FSW field). I have no idea at all where the 8*8=64
bytes padding comes from!
+} SEV_ES_SAVE_AREA;(8) same as (3); please insert a space character between