Re: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation

["Laszlo Ersek" <lersek@redhat.com>]

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@amd.com>
>>>
>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3D3275&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C55d74e19a5554988e0b608d918e1215c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568177488739589%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ZIy6xEXWmxgVxZm6mrdlroM7OPQajEjUSD8W5z2ohu0%3D&amp;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@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>
>>> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@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@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
>>
>