[edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
Lendacky, Thomas
thomas.lendacky at amd.com
Mon May 17 14:17:34 UTC 2021
On 5/16/21 10:08 PM, Laszlo Ersek wrote:
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3D3275&data=04%7C01%7Cthomas.lendacky%40amd.com%7C55d74e19a5554988e0b608d918e1215c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568177488739589%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ZIy6xEXWmxgVxZm6mrdlroM7OPQajEjUSD8W5z2ohu0%3D&reserved=0
>
> (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.
Yes, the APM is definitely lacking in this area.
>
> 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.
The APs won't be in 64-bit mode when being started. In reset, they will be
in real mode, so the attributes will be set up for that. Please see Table
4-3 and Table 4-4 for how these will matter.
>
> 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.
For reset, when we are in real mode, that would correspond to executable /
readable type.
>
> (2) Can you please explain that? Just in this discussion thread, no need
> ot change the code or the commit message.
The idea is that we need to support real mode for the AP creation support,
but actually AP creation isn't limited to that. I didn't want to
overly-define the segment register for all the different modes, since it
would only be real mode that would be used by OVMF, but maybe that would
make it eaiser / clearer to understand...
>
> 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?
Same as previous comment, the AP will be starting in real 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).
I'll add a reference to table 14-2 under the "Processor Initialization"
section.
>
>
> (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")?
For processor reset, type 3 represents a busy 16-bit TSS.
So I can work on clarifying the comments around all of the definitions and
indicate that values are defined for processor reset support and that more
definitions would be needed if the segment registers want to be examined /
set in different modes. Thoughts?
Thanks,
Tom
>
> (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 (#75209): https://edk2.groups.io/g/devel/message/75209
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