[edk2-devel] [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
Lendacky, Thomas
thomas.lendacky at amd.com
Tue May 11 15:43:43 UTC 2021
On 5/11/21 4:59 AM, Laszlo Ersek wrote:
> On 05/07/21 22:38, Brijesh Singh wrote:
>> From: Tom Lendacky <thomas.lendacky at amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cthomas.lendacky%40amd.com%7C92c1323bd1e84a2a38e208d914636ddf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563239563579592%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=DMDhcseilROSsL6EISUoT9p0pI%2BmXtEC3rLHIQS4NmI%3D&reserved=0
>>
>> Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is
>> enabled in the guest VM. See the GHCB spec section for additional
>> details.
>
> (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.
There are some needed changes to this patch, so I can fix that up. I just
avoided putting actual section numbers in there because it is possible
that they can change in future versions.
>
>>
>> While at it, define the VMSA state save area that are required for
>
> (2) I think "area" (singular) is correct here, so we should say "is".
> I'll update it.
I can fix that up.
>
>> creating the AP. The save area format is defined in AMD APM volume
>> 2 (Table B-4).
>>
>> Cc: James Bottomley <jejb at linux.ibm.com>
>> Cc: Min Xu <min.m.xu at intel.com>
>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Erdem Aktas <erdemaktas at google.com>
>> Cc: Michael D Kinney <michael.d.kinney at intel.com>
>> Cc: Liming Gao <gaoliming at byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky at amd.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
>> ---
>> MdePkg/Include/Register/Amd/Ghcb.h | 70 ++++++++++++++++++++++++++++++
>> 1 file changed, 70 insertions(+)
>>
>> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
>> index a15b4b7e2760..956cefbc003c 100644
>> --- a/MdePkg/Include/Register/Amd/Ghcb.h
>> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
>> @@ -55,6 +55,7 @@
>> #define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
>> #define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
>> #define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
>> +#define SVM_EXIT_SNP_AP_CREATION 0x80000013ULL
>> #define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
>> #define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
>>
>> @@ -83,6 +84,12 @@
>> #define IOIO_SEG_ES 0
>> #define IOIO_SEG_DS (BIT11 | BIT10)
>>
>> +//
>> +// AP Creation Information
>> +//
>> +#define SVM_VMGEXIT_SNP_AP_CREATE_ON_INIT 0
>> +#define SVM_VMGEXIT_SNP_AP_CREATE 1
>> +#define SVM_VMGEXIT_SNP_AP_DESTROY 2
>>
>> typedef PACKED struct {
>> UINT8 Reserved1[203];
>> @@ -195,4 +202,67 @@ typedef struct {
>> SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
>> } SNP_PAGE_STATE_CHANGE_INFO;
>>
>> +//
>> +// SEV-ES save area mapping structures used for SEV-SNP AP Creation.
>> +// Only the fields required to be set to a non-zero value are defined.
>> +//
>> +#pragma pack(1)
>> +typedef struct {
>> + UINT16 Selector;
>> + UINT16 Attributes;
>> + UINT32 Limit;
>> + UINT64 Base;
>> +} SEV_ES_SEGMENT_REGISTER;
>> +#pragma pack()
>
> (3) there should be a space character between "pack" and "(" -- two
> instances.
Will do.
>
>> +
>> +#define SEV_ES_RESET_CS_ATTRIBUTES (BIT7 | BIT4 | BIT3 | BIT1)
>> +#define SEV_ES_RESET_DS_ATTRIBUTES (BIT7 | BIT4 | BIT1)
>> +#define SEV_ES_RESET_ES_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>> +#define SEV_ES_RESET_FS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>> +#define SEV_ES_RESET_GS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>> +#define SEV_ES_RESET_SS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>> +
>> +#define SEV_ES_RESET_GDTR_ATTRIBUTES 0
>> +#define SEV_ES_RESET_LDTR_ATTRIBUTES (BIT7 | 2)
>> +#define SEV_ES_RESET_IDTR_ATTRIBUTES 0
>> +#define SEV_ES_RESET_TR_ATTRIBUTES (BIT7 | 3)
>
> (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.
Yeah, it is a strange format. The format is documented in sections 15.5
(VMRUN Instruction) and 10 (System-Management Mode).
I can try to further document the bit assignments, e.g.
#define SEV_ES_SEGMENT_ATTRIBUTE_PRESENT BIT7
#define SEV_ES_SEGMENT_ATTRIBUTE_USER BIT4
etc.
>
>> +
>> +#pragma pack(1)
>> +typedef struct {
>> + SEV_ES_SEGMENT_REGISTER Es;
>> + SEV_ES_SEGMENT_REGISTER Cs;
>> + SEV_ES_SEGMENT_REGISTER Ss;
>> + SEV_ES_SEGMENT_REGISTER Ds;
>> + SEV_ES_SEGMENT_REGISTER Fs;
>> + SEV_ES_SEGMENT_REGISTER Gs;
>> + SEV_ES_SEGMENT_REGISTER Gdtr;
>> + SEV_ES_SEGMENT_REGISTER Ldtr;
>> + SEV_ES_SEGMENT_REGISTER Idtr;
>> + SEV_ES_SEGMENT_REGISTER Tr;
>> + UINT8 Reserved1[42];
>
> (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.
Right, the offsets are correct, the use of "dword" is incorrect.
>
> 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.
Will do.
>
>> + UINT8 Vmpl;
>> + UINT8 Reserved2[5];
>> + UINT64 Efer;
>> + UINT8 Reserved3[112];
>
> OK
>
>> + UINT64 Cr4;
>> + UINT8 Reserved4[8];
>
> OK (although I'm not sure much is saved over spelling out "Cr3")
>
>> + UINT64 Cr0;
>> + UINT64 Dr7;
>> + UINT64 Dr6;
>> + UINT64 Rflags;
>> + UINT64 Rip;
>> + UINT8 Reserved5[232];
>
> OK
>
>> + UINT64 GPat;
>> + UINT8 Reserved6[320];
>
> OK
>
>> + UINT64 SevFeatures;
>> + UINT8 Reserved7[48];
>
> OK
>
>> + UINT64 XCr0;
>> + UINT8 Reserved8[24];
>
> OK
>
>> + UINT32 Mxcsr;
>> + UINT64 X87Ftw;
>
> (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.
Yes, that is incorrect. They should be UINT16 entries as you state below.
>
>> + UINT64 Reserved9[8];
>> + UINT64 X87Fcw;
>
> (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;
>> +#pragma pack()
>
> (8) same as (3); please insert a space character between
Will do.
Thanks,
Tom
>
>> +
>> #endif
>>
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75016): https://edk2.groups.io/g/devel/message/75016
Mute This Topic: https://groups.io/mt/82665185/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