From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, brijesh.singh@amd.com
Cc: Tom Lendacky <thomas.lendacky@amd.com>,
James Bottomley <jejb@linux.ibm.com>, Min Xu <min.m.xu@intel.com>,
Jiewen Yao <jiewen.yao@intel.com>,
Jordan Justen <jordan.l.justen@intel.com>,
Ard Biesheuvel <ardb+tianocore@kernel.org>,
Erdem Aktas <erdemaktas@google.com>,
Michael D Kinney <michael.d.kinney@intel.com>,
Liming Gao <gaoliming@byosoft.com.cn>,
Zhiguang Liu <zhiguang.liu@intel.com>
Subject: Re: [edk2-devel] [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
Date: Tue, 11 May 2021 11:59:00 +0200 [thread overview]
Message-ID: <541e97fd-7f49-1cd0-fa69-14dd58b2432b@redhat.com> (raw)
In-Reply-To: <20210507203838.23706-7-brijesh.singh@amd.com>
On 05/07/21 22:38, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky@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@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@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
next prev parent reply other threads:[~2021-05-11 9:59 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
2021-05-07 20:38 ` [PATCH 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition Brijesh Singh
2021-05-11 8:32 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 02/13] MdePkg/Amd: add white spaces to retain alignment for future expansion Brijesh Singh
2021-05-11 8:36 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection Brijesh Singh
2021-05-11 8:47 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure Brijesh Singh
2021-05-11 8:50 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change Brijesh Singh
2021-05-11 8:59 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation Brijesh Singh
2021-05-11 9:59 ` Laszlo Ersek [this message]
2021-05-11 15:43 ` [edk2-devel] " Lendacky, Thomas
2021-05-13 11:29 ` Laszlo Ersek
2021-05-07 20:38 ` [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction Brijesh Singh
2021-05-11 10:29 ` [edk2-devel] " Laszlo Ersek
2021-05-11 17:18 ` Brijesh Singh
2021-05-07 20:38 ` [PATCH 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction Brijesh Singh
2021-05-11 11:01 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask() Brijesh Singh
2021-05-11 11:16 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask Brijesh Singh
2021-05-11 11:18 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask Brijesh Singh
2021-05-11 11:19 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask() Brijesh Singh
2021-05-11 11:20 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter Brijesh Singh
2021-05-11 11:55 ` [edk2-devel] " Laszlo Ersek
2021-05-11 17:45 ` Brijesh Singh
2021-05-12 16:35 ` Brijesh Singh
2021-05-13 11:25 ` Laszlo Ersek
2021-05-13 11:24 ` Laszlo Ersek
2021-05-08 1:43 ` 回复: [edk2-devel] [PATCH 00/13] Add GHCBv2 macro and helpers gaoliming
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=541e97fd-7f49-1cd0-fa69-14dd58b2432b@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox