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.web10.210.1621353663592437686 for ; Tue, 18 May 2021 09:01:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Jpe1aDfc; 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=1621353662; 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=E64z+RRVLnSo+C0qGzFc1J8KYIXD0ikRYwTQ4NtL3/k=; b=Jpe1aDfciguXkcWyRN8s+Jp7AfFl451LlHaaRW8k2irVbz35rSk3kRzS7mRC9To+e+KTNm 1WmJm0htAheAURQ+9W3Bftf06hqJSK9syXYZYOxIlern3Y9/69wt74+X+5SNu6/NwVKb8+ Y4//AtHdlZmjR9FNn3LzKGpc0QG344M= 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-419-ebPburIgMOO3TBGk2Z7-mA-1; Tue, 18 May 2021 12:00:57 -0400 X-MC-Unique: ebPburIgMOO3TBGk2Z7-mA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E3BF51020C39; Tue, 18 May 2021 16:00:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-30.ams2.redhat.com [10.36.112.30]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0CAF919D7C; Tue, 18 May 2021 16:00:50 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation To: Tom Lendacky , devel@edk2.groups.io, brijesh.singh@amd.com Cc: 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> <3c0a3580-5e68-cc7b-0fdc-785b600b00f4@redhat.com> <0482bfd5-969c-9aec-2032-e40f9d5ca2e8@amd.com> From: "Laszlo Ersek" Message-ID: <16267c37-ad5e-0e1e-9dbe-540d2e1cf481@redhat.com> Date: Tue, 18 May 2021 18:00:49 +0200 MIME-Version: 1.0 In-Reply-To: <0482bfd5-969c-9aec-2032-e40f9d5ca2e8@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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 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 >>> >>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3D3275&data=04%7C01%7Cthomas.lendacky%40amd.com%7C55d74e19a5554988e0b608d918e1215c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568177488739589%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ZIy6xEXWmxgVxZm6mrdlroM7OPQajEjUSD8W5z2ohu0%3D&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 >>> 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. > > 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 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 >> >