回复: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
gaoliming
gaoliming at byosoft.com.cn
Mon May 17 07:39:03 UTC 2021
Laszlo:
Thanks for your detail review. I have no comments for the changes in this patch set. Reviewed-by: Liming Gao <gaoliming at byosoft.com.cn>
Thanks
Liming
> -----邮件原件-----
> 发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 Laszlo Ersek
> 发送时间: 2021年5月17日 11:09
> 收件人: devel at edk2.groups.io; brijesh.singh at amd.com
> 抄送: Tom Lendacky <thomas.lendacky at amd.com>; James Bottomley
> <jejb at linux.ibm.com>; Min Xu <min.m.xu at intel.com>; Jiewen Yao
> <jiewen.yao at intel.com>; Jordan Justen <jordan.l.justen at intel.com>; Ard
> Biesheuvel <ardb+tianocore at kernel.org>; Erdem Aktas
> <erdemaktas at google.com>; Michael D Kinney
> <michael.d.kinney at intel.com>; Liming Gao <gaoliming at byosoft.com.cn>;
> Zhiguang Liu <zhiguang.liu at intel.com>
> 主题: Re: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB
> macros for SNP AP creation
>
> 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 (#75191): https://edk2.groups.io/g/devel/message/75191
Mute This Topic: https://groups.io/mt/82881496/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