Laszlo: Thanks for your detail review. I have no comments for the changes in this patch set. Reviewed-by: Liming Gao <gaolim...@byosoft.com.cn>
Thanks Liming > -----邮件原件----- > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek > 发送时间: 2021年5月17日 11:09 > 收件人: devel@edk2.groups.io; brijesh.si...@amd.com > 抄送: Tom Lendacky <thomas.lenda...@amd.com>; James Bottomley > <j...@linux.ibm.com>; Min Xu <min.m...@intel.com>; Jiewen Yao > <jiewen....@intel.com>; Jordan Justen <jordan.l.jus...@intel.com>; Ard > Biesheuvel <ardb+tianoc...@kernel.org>; Erdem Aktas > <erdemak...@google.com>; Michael D Kinney > <michael.d.kin...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>; > Zhiguang Liu <zhiguang....@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.lenda...@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 <j...@linux.ibm.com> > > Cc: Min Xu <min.m...@intel.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Tom Lendacky <thomas.lenda...@amd.com> > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > > Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Erdem Aktas <erdemak...@google.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > Cc: Zhiguang Liu <zhiguang....@intel.com> > > Reviewed-by: Liming Gao <gaolim...@byosoft.com.cn> > > Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> > > Signed-off-by: Brijesh Singh <brijesh.si...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
