On 5/11/21 4:59 AM, Laszlo Ersek wrote: > On 05/07/21 22:38, Brijesh Singh wrote: >> From: Tom Lendacky <thomas.lenda...@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 <j...@linux.ibm.com> >> Cc: Min Xu <min.m...@intel.com> >> Cc: Jiewen Yao <jiewen....@intel.com> >> Cc: Tom Lendacky <thomas.lenda...@amd.com> >> Cc: Jordan Justen <jordan.l.jus...@intel.com> >> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Erdem Aktas <erdemak...@google.com> >> Cc: Michael D Kinney <michael.d.kin...@intel.com> >> Cc: Liming Gao <gaolim...@byosoft.com.cn> >> Cc: Zhiguang Liu <zhiguang....@intel.com> >> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> >> Signed-off-by: Brijesh Singh <brijesh.si...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-