[edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
Laszlo Ersek
lersek at redhat.com
Mon May 17 03:08:50 UTC 2021
Patches v2 01-05 look good to me, thanks for the updates. Now on to this
one:
On 05/13/21 01:46, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky at amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3275
(1) The "3D" seems like a typo in the bug ticket URL. (This crept into
v2 somehow; v1 didn't have it.)
>
> Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is enabled
> in the guest VM. See the GHCB specification, Table 5 "List of Supported
> Non-Automatic Events" and sections 4.1.9 and 4.3.2, for further details.
>
> While at it, define the VMSA state save area that is required for creating
> the AP. The save area format is defined in AMD APM volume 2, Table B-4
> (there is a mistake in the table that defines the size of the reserved
> area at offset 0xc8 as a dword, when it is actually a word). The format of
> the save area segment registers is further defined in AMD APM volume 2,
> sections 10 and 15.5.
>
> 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>
> Reviewed-by: Liming Gao <gaoliming at byosoft.com.cn>
> 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 | 76 ++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
> index 029904b1c63a..4d1ee29e0a5e 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,73 @@ 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.
> +//
> +#define SEV_ES_RESET_CODE_SEGMENT_TYPE 0xA
> +#define SEV_ES_RESET_DATA_SEGMENT_TYPE 0x2
> +
> +#define SEV_ES_RESET_LDT_TYPE 0x2
> +#define SEV_ES_RESET_TSS_TYPE 0x3
> +
> +#pragma pack (1)
> +typedef union {
> + struct {
> + UINT16 Type:4;
> + UINT16 Sbit:1;
> + UINT16 Dpl:2;
> + UINT16 Present:1;
> + UINT16 Avl:1;
> + UINT16 Reserved1:1;
> + UINT16 Db:1;
> + UINT16 Granularity:1;
> + } Bits;
> + UINT16 Uint16;
> +} SEV_ES_SEGMENT_REGISTER_ATTRIBUTES;
> +
> +typedef struct {
> + UINT16 Selector;
> + SEV_ES_SEGMENT_REGISTER_ATTRIBUTES Attributes;
> + UINT32 Limit;
> + UINT64 Base;
> +} SEV_ES_SEGMENT_REGISTER;
> +
I'm not saying anything is incorrect about this, but I *am* going to
rant about the APM.
It's simply impenetrable. I've been staring at it for ~50 minutes now,
and I still cannot fully connect it to your code.
[1] In sections "4.8.1 Code-Segment Descriptors" and "4.8.2 Data-Segment
Descriptors", the reader is introduced to the "normal" (not SEV-ES, not
virtualized, not SMM) segment descriptors. Why *these* are relevant
*here* is nothing short of mind-boggling, but please bear with me.
[2] In section "10.2.3 SMRAM State-Save Area", "Table 10-1. AMD64
Architecture SMM State-Save Area", the reader is introduced to the
2+2+4+8 segment register representation. The table only lists "Selector,
Attributes, Limit, Base" as fields, and nothing about the actual
contents. Way too little information. I guess this is covered by the
commit message reference "section 10".
[3] In section "15.5 VMRUN Instruction", "15.5.1 Basic Operation",
paragraph "Segment State in the VMCB", we're given a long-winded,
*textual* only -- no diagram! -- and *differential* (relative)
explanation, on top of the two, above-quoted, locations of the spec. I'm
sorry but this is just impossible to follow. Would it have been a
unaffordable to insert a self-contained diagram here, with
self-contained, absolute explanation?
So let me quote:
The segment registers are stored in the VMCB in a format similar to
that for SMM: both base and limit are fully expanded; segment
attributes are stored as 12-bit values formed by the concatenation
of bits 55:52 and 47:40 from the original 64-bit (in-memory) segment
descriptors; the descriptor “P” bit is used to signal NULL segments
(P=0) where permissible and/or relevant.
So, if we apply this to [1] and [2], the "Selector", "Limit" and "Base"
fields of the SMM and VMCB segment register representation are
explained. Further, we get the following for the Attributes field, by
manual reconstruction:
55 54 53 52 47 46 45 44 43 42 41 40
G D L AVL P [DPL] 1 1 C R A Code Segment Descriptor
* * * * * * ignored bits in 64-bit mode
G D/B - AVL P [DPL] 1 0 E W A Data Segment Descriptor
* * * * * * * * * * ignored bits in 64-bit mode
In other words, in the code segment descriptor, D, L, P, DPL, and C
matter. In the data segment descriptor, only P matters.
In particular, what [3] says, quoted above, about the "P" bit, seems
sensible -- "P" is indeed *not* ignored for either segment descriptor
type (code or data).
But then we continure reading [3], and we find (quote again):
The loading of segment attributes from the VMCB (which may have been
overwritten by software) may result in attribute bit values that are
otherwise not allowed. However, only some of the attribute bits are
actually observed by hardware, depending on the segment register in
question:
* CS—D, L, P, and R.
* SS—B, P, E, W, and Code/Data
* DS, ES, FS, GS —D, P, DPL, E, W, and Code/Data.
* LDTR—P, S, and Type (LDT)
* TR—P, S, and Type (32- or 16-bit TSS)
I'm going to ignore SS, LDTR, and TR, but let's check CS and DS.
For CS, the spec says, "D, L, P, and R" are observed by the hardware.
But we've just shown that R is ignored *in general* for code segments in
64-bit mode! In other words, { D, L, P, R } is *not a subset* of { D, L,
P, DPL, C }.
For DS, the spec says here, "D, P, DPL, E, W, and Code/Data" are
observed. I'm going to give "Code/Data" a pass (bit 43 in the original
representation), but the rest is {D, P, DPL, E, W}, which is *not a
subset* of { P }.
Again, from [1], section 4.8.2 specifically, E (expand-down) and W
(writeable) are ignored, DPL is ignored, and D isn't even called D but
"D/B", and it is ignored. So how can the spec say in [3] that the
hardware observes { D, DPL, E, W } when all those are ignored per prior
definition [1]?
- And then I see no reference that SEV-ES uses the same
(circumstantially-defined) segment register format.
So anyway, I'll just accept that I don't understand the spec -- I think
it has inconsistencies. Let's look at the code:
- Bits 43:40 are packed into the "Type" bit-field. That means [1,C,R,A]
for the code segment descriptor, and [0,E,W,A] for data segment descriptors
SEV_ES_RESET_CODE_SEGMENT_TYPE has value 0xA (binary 1010), which maps
to: 1=1, C=0, R=1, A=0. Problem: per [1], the R bit is ignored, so I
have no clue why we set it.
(2) Can you please explain that? Just in this discussion thread, no need
ot change the code or the commit message.
The C ("conforming") bit actually matters. It is documented in section
"4.7.2 Code-Segment Descriptors" (Code-Segment Descriptor—Legacy Mode).
It seems like "non-conforming" is not a problem here, as long as we
don't use multiple privilege levels, which I think we don't, in
firmware-land. OK.
Then, on to SEV_ES_RESET_DATA_SEGMENT_TYPE. Value 0x2 (binary 0010).
Maps to [0,E,W,A], meaning 0=0, E=0, W=1, A=0.
(3) Why is W (writeable) set to 1, when, according to [1], it is ignored
in 64-bit mode?
- "Sbit" seems to stand for bit 44 from the original representation --
constant 1. OK I think.
- "Dpl", "Present" and "Avl" look OK to me.
- Should "Reserved1" really be called that? It seems to map to bit 53 in
the original representation, and the L (long mode) bit actually matters
for the code segment. (It indeed appears undefined / reserved for data
segments.)
Am I *totally* mistaken here and we're not even talking long mode?
- "Db" and "Granularity" look OK.
- SEV_ES_RESET_LDT_TYPE (value 2) matches "64-bit LDT" in "4.8.3 System
Descriptors", "Table 4-6. System-Segment Descriptor Types—Long Mode". OK.
- SEV_ES_RESET_TSS_TYPE (value 3) seems wrong. In Table 4-6, value 3 is
associated with "Reserved (Illegal)". And, according to "12.2.2 TSS
Descriptor", "The TSS descriptor is a system-segment descriptor", which
makes me think that I'm looking at value 3 in the proper table (Table 4-6).
(4) Can you please explain why SEV_ES_RESET_TSS_TYPE is 3, and not (say)
0x9 ("Available 64-bit TSS") or 0xB ("Busy 64-bit TSS")?
(Please note that this is only superficial pattern matching from my side
("TSS"); I don't know the first thing about "hardware task management".)
> +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];
> + UINT8 Vmpl;
> + UINT8 Reserved2[5];
> + UINT64 Efer;
> + UINT8 Reserved3[112];
> + UINT64 Cr4;
> + UINT8 Reserved4[8];
> + UINT64 Cr0;
> + UINT64 Dr7;
> + UINT64 Dr6;
> + UINT64 Rflags;
> + UINT64 Rip;
> + UINT8 Reserved5[232];
> + UINT64 GPat;
> + UINT8 Reserved6[320];
> + UINT64 SevFeatures;
> + UINT8 Reserved7[48];
> + UINT64 XCr0;
> + UINT8 Reserved8[24];
> + UINT32 Mxcsr;
> + UINT16 X87Ftw;
> + UINT8 Reserved9[2];
> + UINT16 X87Fcw;
> +} SEV_ES_SAVE_AREA;
> +#pragma pack ()
> +
> #endif
>
This part looks good to me.
(I spent almost two hours reviewing this patch.)
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75160): https://edk2.groups.io/g/devel/message/75160
Mute This Topic: https://groups.io/mt/82787287/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