[edk2-devel] [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
Laszlo Ersek
lersek at redhat.com
Tue May 11 09:59:00 UTC 2021
On 05/07/21 22:38, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky at amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> 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.
>
> 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.
> 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.
> +
> +#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.
> +
> +#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.
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;
> + 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.
> + 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
> +
> #endif
>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74997): https://edk2.groups.io/g/devel/message/74997
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