From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by mx.groups.io with SMTP id smtpd.web10.15659.1621237149070497337 for ; Mon, 17 May 2021 00:39:10 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: byosoft.com.cn, ip: 58.240.74.242, mailfrom: gaoliming@byosoft.com.cn) Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Mon, 17 May 2021 15:39:01 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 58.246.60.130 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: , , Cc: "'Tom Lendacky'" , "'James Bottomley'" , "'Min Xu'" , "'Jiewen Yao'" , "'Jordan Justen'" , "'Ard Biesheuvel'" , "'Erdem Aktas'" , "'Michael D Kinney'" , "'Zhiguang Liu'" References: <20210512234615.1726-1-brijesh.singh@amd.com> <20210512234615.1726-7-brijesh.singh@amd.com> <3c0a3580-5e68-cc7b-0fdc-785b600b00f4@redhat.com> In-Reply-To: <3c0a3580-5e68-cc7b-0fdc-785b600b00f4@redhat.com> Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIHYyIDA2LzEzXSBNZGVQa2cvUmVnaXN0ZXIvQW1kOiBkZWZpbmUgR0hDQiBtYWNyb3MgZm9yIFNOUCBBUCBjcmVhdGlvbg==?= Date: Mon, 17 May 2021 15:39:03 +0800 Message-ID: <006601d74aef$b57daaf0$207900d0$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQF2u2c/CDb92q1a7UfRPFY1On/mSwGMvdt3AaMp7nmrjukF4A== Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn Laszlo: Thanks for your detail review. I have no comments for the changes in thi= s patch set. Reviewed-by: Liming Gao Thanks Liming > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io = =E4=BB=A3=E8=A1=A8 Laszlo Ersek > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2021=E5=B9=B45=E6=9C=8817=E6=97=A5= 11:09 > =E6=94=B6=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io; brijesh.singh@amd.com > =E6=8A=84=E9=80=81: Tom Lendacky ; James Bottom= ley > ; Min Xu ; Jiewen Yao > ; Jordan Justen ; Ard > Biesheuvel ; Erdem Aktas > ; Michael D Kinney > ; Liming Gao ; > Zhiguang Liu > =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Am= d: define GHCB > macros for SNP AP creation >=20 > Patches v2 01-05 look good to me, thanks for the updates. Now on to this > one: >=20 > On 05/13/21 01:46, Brijesh Singh wrote: > > From: Tom Lendacky > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3D3275 >=20 > (1) The "3D" seems like a typo in the bug ticket URL. (This crept into > v2 somehow; v1 didn't have it.) >=20 > > > > Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is enabl= ed > > in the guest VM. See the GHCB specification, Table 5 "List of Supporte= d > > Non-Automatic Events" and sections 4.1.9 and 4.3.2, for further detail= s. > > > > While at it, define the VMSA state save area that is required for crea= ting > > 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 forma= t of > > the save area segment registers is further defined in AMD APM volume 2= , > > sections 10 and 15.5. > > > > Cc: James Bottomley > > Cc: Min Xu > > Cc: Jiewen Yao > > Cc: Tom Lendacky > > Cc: Jordan Justen > > Cc: Ard Biesheuvel > > Cc: Laszlo Ersek > > Cc: Erdem Aktas > > Cc: Michael D Kinney > > Cc: Liming Gao > > Cc: Zhiguang Liu > > Reviewed-by: Liming Gao > > Signed-off-by: Tom Lendacky > > Signed-off-by: Brijesh Singh > > --- > > 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; > > + >=20 > I'm not saying anything is incorrect about this, but I *am* going to > rant about the APM. >=20 > It's simply impenetrable. I've been staring at it for ~50 minutes now, > and I still cannot fully connect it to your code. >=20 > [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. >=20 > [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". >=20 > [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? >=20 > So let me quote: >=20 > 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 =E2=80=9CP=E2=80=9D bit is used to signa= l NULL segments > (P=3D0) where permissible and/or relevant. >=20 > 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: >=20 > 55 54 53 52 47 46 45 44 43 42 41 40 >=20 > G D L AVL P [DPL] 1 1 C R A Code Segment Descriptor > * * * * * * ignored bits in 64-bit > mode >=20 > G D/B - AVL P [DPL] 1 0 E W A Data Segment Descriptor > * * * * * * * * * * ignored bits in 64-bit > mode >=20 > In other words, in the code segment descriptor, D, L, P, DPL, and C > matter. In the data segment descriptor, only P matters. >=20 > 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). >=20 > But then we continure reading [3], and we find (quote again): >=20 > 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=E2=80=94D, L, P, and R. > * SS=E2=80=94B, P, E, W, and Code/Data > * DS, ES, FS, GS =E2=80=94D, P, DPL, E, W, and Code/Data. > * LDTR=E2=80=94P, S, and Type (LDT) > * TR=E2=80=94P, S, and Type (32- or 16-bit TSS) >=20 > I'm going to ignore SS, LDTR, and TR, but let's check CS and DS. >=20 > 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 }. >=20 > 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 }. >=20 > 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]? >=20 > - And then I see no reference that SEV-ES uses the same > (circumstantially-defined) segment register format. >=20 >=20 > So anyway, I'll just accept that I don't understand the spec -- I think > it has inconsistencies. Let's look at the code: >=20 > - 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 descript= ors >=20 > SEV_ES_RESET_CODE_SEGMENT_TYPE has value 0xA (binary 1010), which > maps > to: 1=3D1, C=3D0, R=3D1, A=3D0. Problem: per [1], the R bit is ignored, = so I > have no clue why we set it. >=20 > (2) Can you please explain that? Just in this discussion thread, no need > ot change the code or the commit message. >=20 > The C ("conforming") bit actually matters. It is documented in section > "4.7.2 Code-Segment Descriptors" (Code-Segment Descriptor=E2=80=94Legacy= 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. >=20 > Then, on to SEV_ES_RESET_DATA_SEGMENT_TYPE. Value 0x2 (binary 0010). > Maps to [0,E,W,A], meaning 0=3D0, E=3D0, W=3D1, A=3D0. >=20 > (3) Why is W (writeable) set to 1, when, according to [1], it is ignored > in 64-bit mode? >=20 >=20 > - "Sbit" seems to stand for bit 44 from the original representation -- > constant 1. OK I think. >=20 > - "Dpl", "Present" and "Avl" look OK to me. >=20 > - 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.) >=20 > Am I *totally* mistaken here and we're not even talking long mode? >=20 > - "Db" and "Granularity" look OK. >=20 > - SEV_ES_RESET_LDT_TYPE (value 2) matches "64-bit LDT" in "4.8.3 System > Descriptors", "Table 4-6. System-Segment Descriptor Types=E2=80=94Long M= ode". OK. >=20 > - 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). >=20 >=20 > (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")? >=20 > (Please note that this is only superficial pattern matching from my side > ("TSS"); I don't know the first thing about "hardware task management".) >=20 >=20 > > +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 > > >=20 > This part looks good to me. >=20 > (I spent almost two hours reviewing this patch.) >=20 > Thanks! > Laszlo >=20 >=20 >=20 >=20 >=20