From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.14259.1621220945416455842 for ; Sun, 16 May 2021 20:09:06 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fi0stVzv; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1621220943; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=rTeYjy+r+z5VwQ5WAp2ThZEJGnAjKPOt4I1nBbiKXJI=; b=fi0stVzvYgLH16E3axC4XAhClm3RALKdTdtw98iKrjvD5OvOSKXaAhOqyQlNZ5tmYrREJn XFj7fYNtXSlU47g99ZZLh2FiWv6lQt/OlOEtQHcRmyjzmMoAy7cMNyPE8AUpBmeROM0kDJ lxBU5oi25/pjxn5Jwc3vCHMCcjsn3dQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-59-jCZzEdyUPt2VVs9T8Ifebg-1; Sun, 16 May 2021 23:08:57 -0400 X-MC-Unique: jCZzEdyUPt2VVs9T8Ifebg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 476B5800D62; Mon, 17 May 2021 03:08:55 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-22.ams2.redhat.com [10.36.112.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1A3995E26F; Mon, 17 May 2021 03:08:51 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation To: devel@edk2.groups.io, brijesh.singh@amd.com Cc: Tom Lendacky , James Bottomley , Min Xu , Jiewen Yao , Jordan Justen , Ard Biesheuvel , Erdem Aktas , Michael D Kinney , Liming Gao , Zhiguang Liu References: <20210512234615.1726-1-brijesh.singh@amd.com> <20210512234615.1726-7-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: <3c0a3580-5e68-cc7b-0fdc-785b600b00f4@redhat.com> Date: Mon, 17 May 2021 05:08:50 +0200 MIME-Version: 1.0 In-Reply-To: <20210512234615.1726-7-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 > > 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 > 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; > + 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