[edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
Laszlo Ersek
lersek at redhat.com
Tue May 18 16:00:49 UTC 2021
On 05/17/21 16:17, Tom Lendacky wrote:
> 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?
My bad -- I feel really dumb now.
The four important macros which I got hung upon are all named
SEV_ES_RESET_*. Keyword being "reset". :/
With the typo (1) fixed:
Reviewed-by: Laszlo Ersek <lersek at redhat.com>
Thanks & sorry!
Laszlo
>
> 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 (#75258): https://edk2.groups.io/g/devel/message/75258
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