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.10619.1620727151556841489 for ; Tue, 11 May 2021 02:59:11 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Vc//R4DP; 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=1620727150; 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=IkpjJs1NTXeLeiBtrP1th/9kP0SUS/aM9nyrEMNUnT0=; b=Vc//R4DPAGzJUipcswykO+lYA82dGu6nU0SnNPKdMV6o9mfuz5EIQtiS7WW6ae05LEK8Ve DwxepYQVdyd3NReVACiqtn60UaUFfubZPH0Rwry8eBV+oi+d/FdLJ989bG6W7uXfiaHTUy gcWZFUJtn266oFok1DWw3fZJy4+XEy8= 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-159-W9mgxDfSN8ibzu8QqaEGeQ-1; Tue, 11 May 2021 05:59:06 -0400 X-MC-Unique: W9mgxDfSN8ibzu8QqaEGeQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4D3AD801817; Tue, 11 May 2021 09:59:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-233.ams2.redhat.com [10.36.112.233]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8A9F263C3F; Tue, 11 May 2021 09:59:01 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 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: <20210507203838.23706-1-brijesh.singh@amd.com> <20210507203838.23706-7-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: <541e97fd-7f49-1cd0-fa69-14dd58b2432b@redhat.com> Date: Tue, 11 May 2021 11:59:00 +0200 MIME-Version: 1.0 In-Reply-To: <20210507203838.23706-7-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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: 7bit On 05/07/21 22:38, Brijesh Singh wrote: > From: Tom Lendacky > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275 > > Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is > enabled in the guest VM. See the GHCB spec section for additional > details. (1) The actual section reference is missing. I'll fix it up: from where the spec introduces exit code 0x8000_0013, the sections appear to be 4.1.9 and 4.3.2. Also, Table 5. "List of Supported Non-Automatic Events" is relevant for the SVM_VMGEXIT_SNP_AP_* macros. > > While at it, define the VMSA state save area that are required for (2) I think "area" (singular) is correct here, so we should say "is". I'll update it. > creating the AP. The save area format is defined in AMD APM volume > 2 (Table B-4). > > 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 > Signed-off-by: Tom Lendacky > Signed-off-by: Brijesh Singh > --- > MdePkg/Include/Register/Amd/Ghcb.h | 70 ++++++++++++++++++++++++++++++ > 1 file changed, 70 insertions(+) > > diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h > index a15b4b7e2760..956cefbc003c 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,67 @@ 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. > +// > +#pragma pack(1) > +typedef struct { > + UINT16 Selector; > + UINT16 Attributes; > + UINT32 Limit; > + UINT64 Base; > +} SEV_ES_SEGMENT_REGISTER; > +#pragma pack() (3) there should be a space character between "pack" and "(" -- two instances. > + > +#define SEV_ES_RESET_CS_ATTRIBUTES (BIT7 | BIT4 | BIT3 | BIT1) > +#define SEV_ES_RESET_DS_ATTRIBUTES (BIT7 | BIT4 | BIT1) > +#define SEV_ES_RESET_ES_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES > +#define SEV_ES_RESET_FS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES > +#define SEV_ES_RESET_GS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES > +#define SEV_ES_RESET_SS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES > + > +#define SEV_ES_RESET_GDTR_ATTRIBUTES 0 > +#define SEV_ES_RESET_LDTR_ATTRIBUTES (BIT7 | 2) > +#define SEV_ES_RESET_IDTR_ATTRIBUTES 0 > +#define SEV_ES_RESET_TR_ATTRIBUTES (BIT7 | 3) (4) ... I guess I can't go ahead merging this myself, after all (Liming may of course still merge the MdePkg patches, if he wants to). My problem here is that the bit positions are cryptic. I've found the *normal* (not SEV-ES) segment descriptor attributes in the AMD APM (publication #24593, revision 3.37, date March 2021, volume 2, sections sections 4.7 and 4.8). However, the bit positions SEV-ES descriptors are surely different. For the "normal" segment descriptors, we already have the IA32_SEGMENT_DESCRIPTOR type in edk2, with the nicely broken-out attribute bits. The bit meanings within "SEV_ES_SEGMENT_REGISTER.Attributes" remain unclear to me. Please at least provide a *specific* documentation reference in the commit message where I can verify (or at least "decode") the attribute bits. > + > +#pragma pack(1) > +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]; (5) This doesn't seem right. The comment higher up says that "Only the fields required to be set to a non-zero value are defined", which is fine. But in Table B-4, between fields "Tr" and "Vmpl", we have 5 qword fields (PL0_SSP through PL3_SSP, plus U_CET), and a reserved dword field. That makes for 5*8+4 = 44 bytes, not 42. Hmmm. If I look at the table differently, I get a different result. Namely, the first offset right after Tr is 0A0h, and the start offset of Vmpl is 0CAh. The difference is indeed 42 (decimal). Ah, I've found it. The bug is in the spec. The Reserved field at offset 0C8h is listed with size "dword", but the field right after it, namely VMPL, starts at offset 0CAh -- that is, only two bytes later. Which means that the "dword" size for Reserved is wrong; it should be "word" only. I couldn't find a more recent release of the APM than "revision 3.37". Please add a remark to the commit message on this patch that highlights this typo in the spec. > + UINT8 Vmpl; > + UINT8 Reserved2[5]; > + UINT64 Efer; > + UINT8 Reserved3[112]; OK > + UINT64 Cr4; > + UINT8 Reserved4[8]; OK (although I'm not sure much is saved over spelling out "Cr3") > + UINT64 Cr0; > + UINT64 Dr7; > + UINT64 Dr6; > + UINT64 Rflags; > + UINT64 Rip; > + UINT8 Reserved5[232]; OK > + UINT64 GPat; > + UINT8 Reserved6[320]; OK > + UINT64 SevFeatures; > + UINT8 Reserved7[48]; OK > + UINT64 XCr0; > + UINT8 Reserved8[24]; OK > + UINT32 Mxcsr; > + UINT64 X87Ftw; (6) If you are packing all four words (X87_FTW, X87_FSW, X87_FCW, X87_FOP) into a qword, then please give the latter a different name. The spec associates X87_FTW with just the word at offset 40Ch. I propose the name "FpState" for the UINT64 field in the edk2 structure. > + UINT64 Reserved9[8]; > + UINT64 X87Fcw; (7) Ugh, wait, something's wrong -- you *are* apparently adding a field for X87_FCW separately! But then the type of X87Ftw is wrong -- it should be UINT16. Same for X87Fcw. The distance between them is also wrong, it should be only 2 bytes (basically the X87_FSW field). I have no idea at all where the 8*8=64 bytes padding comes from! > +} SEV_ES_SAVE_AREA; > +#pragma pack() (8) same as (3); please insert a space character between > + > #endif > Thanks Laszlo