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.6111.1620136853667634736 for ; Tue, 04 May 2021 07:00:53 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=gWw1zIxT; 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=1620136852; 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=iq5ikGJw3rDDJW8fZNvnnNKywDVVjoVIv6mSs0p0YKU=; b=gWw1zIxTyYmlJHBZ9T63phGNwL+iT2N3Vv2RymV9Xhh75QBvMaYl8+/vCvlTVQRmn3nsDk vb9/69ArNKQBCBoBbDDBXQ/lMZN05mWrrYb6zJdX/0l/Cz4/3DD+w8BYpqEJpKBPq9wAgA rXhbYZPmJcFAtjmOD2VrHv7tBI8Wkdw= 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-593-zwFoXe1WNWK2eaegMUJGrw-1; Tue, 04 May 2021 10:00:50 -0400 X-MC-Unique: zwFoXe1WNWK2eaegMUJGrw-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 F05079114D; Tue, 4 May 2021 14:00:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-105.ams2.redhat.com [10.36.113.105]) by smtp.corp.redhat.com (Postfix) with ESMTP id E1D235C1B4; Tue, 4 May 2021 14:00:00 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v2 04/28] MdePkg: Define the Page State Change VMGEXIT structures From: "Laszlo Ersek" To: devel@edk2.groups.io, brijesh.singh@amd.com Cc: James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Ard Biesheuvel , Erdem Aktas References: <20210430115148.22267-1-brijesh.singh@amd.com> <20210430115148.22267-5-brijesh.singh@amd.com> <47d74104-dd86-8bb9-ba5e-6c0e7371d5e4@redhat.com> Message-ID: <3782d6d7-bb52-5112-06f9-d05f8d404560@redhat.com> Date: Tue, 4 May 2021 15:59:59 +0200 MIME-Version: 1.0 In-Reply-To: <47d74104-dd86-8bb9-ba5e-6c0e7371d5e4@redhat.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/04/21 14:33, Laszlo Ersek wrote: > On 04/30/21 13:51, Brijesh Singh wrote: >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275 >> >> The Page State Change NAE exit will be used by the SEV-SNP guest to >> request a page state change using the GHCB protocol. See the GHCB >> spec section 4.1.6 and 2.3.1 for more detail on the structure >> definitions. >> >> Cc: James Bottomley >> Cc: Min Xu >> Cc: Jiewen Yao >> Cc: Tom Lendacky >> Cc: Jordan Justen >> Cc: Ard Biesheuvel >> Cc: Laszlo Ersek >> Cc: Erdem Aktas >> Signed-off-by: Brijesh Singh >> --- >> MdePkg/Include/Register/Amd/Fam17Msr.h | 15 ++++++++++ >> MdePkg/Include/Register/Amd/Ghcb.h | 29 ++++++++++++++++++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h >> index e19bd04b6c..432cee2feb 100644 >> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h >> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h >> @@ -58,6 +58,19 @@ typedef union { >> UINT64 GuestFrameNumber:52; >> } GhcbGpaRegister; >> >> + struct { >> + UINT64 Function:12; >> + UINT64 GuestFrameNumber:40; >> + UINT64 Operation:4; >> + UINT64 Reserved:8; >> + } SnpPageStateChangeRequest; >> + >> + struct { >> + UINT32 Function:12; >> + UINT32 Reserved:20; >> + UINT32 ErrorCode; >> + } SnpPageStateChangeResponse; >> + >> VOID *Ghcb; >> > This matches section 2.3.1 in rev 2.00. > >> UINT64 GhcbPhysicalAddress; >> @@ -69,6 +82,8 @@ typedef union { >> #define GHCB_INFO_CPUID_RESPONSE 5 >> #define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18 >> #define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19 >> +#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20 >> +#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE 21 >> #define GHCB_HYPERVISOR_FEATURES_REQUEST 128 >> #define GHCB_HYPERVISOR_FEATURES_RESPONSE 129 >> #define GHCB_INFO_TERMINATE_REQUEST 256 > > Matches section 2.3.1. > >> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h >> index 2d64a4c28f..1e7c0daed3 100644 >> --- a/MdePkg/Include/Register/Amd/Ghcb.h >> +++ b/MdePkg/Include/Register/Amd/Ghcb.h >> @@ -54,6 +54,7 @@ >> #define SVM_EXIT_NMI_COMPLETE 0x80000003ULL >> #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_HYPERVISOR_FEATURES 0x8000FFFDULL >> #define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL >> > > Matches "Table 5. List of Supported Non-Automatic Events". > >> @@ -160,4 +161,32 @@ typedef union { >> #define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1) >> #define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2) >> #define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3) >> + >> +// SNP Page State Change > > (1) Comment style. > >> +#define SNP_PAGE_STATE_MAX_NPAGES 4095 >> +#define SNP_PAGE_STATE_MAX_ENTRY 253 >> +#define SNP_PAGE_STATE_PRIVATE 1 >> +#define SNP_PAGE_STATE_SHARED 2 >> +#define SNP_PAGE_STATE_PSMASH 3 >> +#define SNP_PAGE_STATE_UNSMASH 4 > > (2) The PSMASH and UNSMASH operations are not documented in the rev 2.00 > spec, in the GHCB MSR protocol. That's probably because PSMASH and > UNSMASH can only be defined in terms of 2MB pages, and > GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST is suitable only for individual, > 4KB pages. I think it would be useful to point out somehow here that > PSMASH and UNSMASH are restricted to the GHCB shared area protocol > (perhaps extend the leading comment on this block of macros). > > (3) I don't understand what "MAX_NPAGES" stands for (4095). The rest of > the series never uses the macro, and I can't associate it with anything > from the spec. If the macro is supposed to relate to the 4KB / 2MB page > smashing / splitting, then its replacement text should be 512. Unless > the macro corresponds to a definition in the spec, I think we should > drop it. > >> + >> +typedef PACKED struct { >> + UINT64 CurrentPage:12; >> + UINT64 GuestFrameNumber:40; >> + UINT64 Op:4; >> + UINT64 PageSize:1; >> + UINT64 Rsvd: 7; >> +} SNP_PAGE_STATE_ENTRY; >> + >> +typedef PACKED struct { >> + UINT16 CurrentEntry; >> + UINT16 EndEntry; >> + UINT32 Rsvd; >> +} SNP_PAGE_STATE_HEADER; > > (4) We tend to write > > #pragma pack (1) > ... > #pragma pack () > > rather than PACKED -- but anyway, is packing really necessary? "Natural > alignment" is required in edk2. I'm OK with packing, but I think the > pragma is the preferred form. > > (5) Please spell out both "Rsvd" fields above as "Reserved". > > (6) Stray space character in "Rsvd: 7". > > (7) The field name "Op" is inconsistent with the other field name > "Operation". > > (8) I think there is a bug (typo) in the rev 2.00 spec, in 4.1.6 "SNP > Page State Change": it says > > ... calculated from the supplied guest physical frame number (GFN) for > the requested page size (GPA = GFN << 12). > > But, if you can choose 2MB page size in the request, then the (GPA = GFN > << 12) formula is not g Sorry, unfinished sentence: I meant that the formula was not generally correct. Thanks Laszlo > > (9) If my understanding of the spec is correct, "EndEntry" has > *inclusive* meaning. That's unusual. Any particular reason for not > making "EndEntry" exclusive (in the spec)? > >> + >> +typedef struct { >> + SNP_PAGE_STATE_HEADER Header; >> + SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY]; >> +} SNP_PAGE_STATE_CHANGE_INFO; >> + >> #endif >> > > Yes, this looks OK. Size is 2+2+4+253*8 = 2032 bytes, which matches the > size of GHCB.SharedBuffer. > > (10) However, *if* you decide to declare SNP_PAGE_STATE_ENTRY and > SNP_PAGE_STATE_HEADER explicitly as packed, then you should do the same > for SNP_PAGE_STATE_CHANGE_INFO. > > (11) Like I mentioned earlier, it's probably helpful if you start the > subject line with > > MdePkg/Register/Amd: ... > > on all of these MdePkg patches. If that becomes too tight, for some of > the MdePkg patches, then I suggest "MdePkg/Amd: ..." (i.e., drop > "Register"). > > Thanks > Laszlo >