From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 02 Oct 2019 03:24:00 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 02586300CB2B; Wed, 2 Oct 2019 10:24:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-71.rdu2.redhat.com [10.10.120.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id B44C860605; Wed, 2 Oct 2019 10:23:57 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 08/44] OvmfPkg: Create GHCB pages for use during Pei and Dxe phase To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" References: <9799d415f652618c8a960cdb0040918185588652.1568922728.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <7aac09f5-8494-3fba-b114-b2b070b6e959@redhat.com> Date: Wed, 2 Oct 2019 12:23:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <9799d415f652618c8a960cdb0040918185588652.1568922728.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.46]); Wed, 02 Oct 2019 10:24:00 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit After the discussion elsewhere in this patch thread, which related to commit messages, and patch order in the series, I can make a few coding style comments on the patch. (No change to functionality.) On 09/19/19 21:52, Lendacky, Thomas wrote: > From: Tom Lendacky > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 > > Allocate memory for the GHCB pages during SEV initialization for use > during Pei and Dxe phases. The GHCB page(s) must be shared pages, so > clear the encryption mask from the current page table entries. Upon > successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize). > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tom Lendacky > --- > OvmfPkg/OvmfPkgIa32.dsc | 2 ++ > OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++ > OvmfPkg/OvmfPkgX64.dsc | 2 ++ > OvmfPkg/PlatformPei/PlatformPei.inf | 2 ++ > OvmfPkg/PlatformPei/AmdSev.c | 36 ++++++++++++++++++++++++++++- > 5 files changed, 43 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 0ce5c01722ef..4369cf6d55e5 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -560,6 +560,8 @@ [PcdsDynamicDefault] > > # Set SEV-ES defaults > gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0 > + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0 > + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0 > > !if $(SMM_REQUIRE) == TRUE > gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8 > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index e7455e35a55d..a74f5028068e 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -572,6 +572,8 @@ [PcdsDynamicDefault] > > # Set SEV-ES defaults > gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0 > + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0 > + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0 > > !if $(SMM_REQUIRE) == TRUE > gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8 > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 0b8305cd10a2..fd714d386e75 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -571,6 +571,8 @@ [PcdsDynamicDefault] > > # Set SEV-ES defaults > gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0 > + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0 > + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0 > > !if $(SMM_REQUIRE) == TRUE > gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8 > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index a9e424a6012a..62abc99f4622 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -105,6 +105,8 @@ [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize > gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive > + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase > + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize > > [FixedPcd] > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress (1) Once you move PcdSevEsActive near the other gEfiMdeModulePkgTokenSpaceGuid PCDs, per , please keep these grouped with it, too. > diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c > index 7ae2f26a2ba7..30c0e4af7252 100644 > --- a/OvmfPkg/PlatformPei/AmdSev.c > +++ b/OvmfPkg/PlatformPei/AmdSev.c > @@ -16,6 +16,9 @@ > #include > #include > #include > +#include > +#include > +#include (2) This #include list is currently sorted. Can you please keep it sorted? > > #include "Platform.h" > > @@ -30,7 +33,10 @@ AmdSevEsInitialize ( > VOID > ) > { > - RETURN_STATUS PcdStatus; > + VOID *GhcbBase; > + PHYSICAL_ADDRESS GhcbBasePa; > + UINTN GhcbPageCount; > + RETURN_STATUS PcdStatus, DecryptStatus; > > if (!MemEncryptSevEsIsEnabled ()) { > return; > @@ -38,6 +44,34 @@ AmdSevEsInitialize ( > > PcdStatus = PcdSetBoolS (PcdSevEsActive, 1); > ASSERT_RETURN_ERROR (PcdStatus); > + > + // > + // Allocate GHCB pages. > + // > + GhcbPageCount = mMaxCpuCount; > + GhcbBase = AllocatePages (GhcbPageCount); Yes, AllocatePages() looks safe, per . > + ASSERT (GhcbBase); (3) I don't really like using *only* an ASSERT for stopping, when we run out of resources in a place like this (i.e. where there's no way to recover, or to report the issue nicely). I'd suggest throwing in an explicit check and a CpuDeadLoop(), in addition to the ASSERT. Anyway, not really important, up to you. (4) The expression in the ASSERT() should compare GhcbBase against NULL explicitly, however. The edk2 coding style permits the implicit "!= 0" comparison (in controlling expressions) for BOOLEANs only. > + > + GhcbBasePa = (PHYSICAL_ADDRESS)(UINTN) GhcbBase; > + > + DecryptStatus = MemEncryptSevClearPageEncMask ( > + 0, > + GhcbBasePa, > + GhcbPageCount, > + TRUE > + ); > + ASSERT_RETURN_ERROR (DecryptStatus); > + > + SetMem (GhcbBase, GhcbPageCount * SIZE_4KB, 0); (5) The following would be more idiomatic: ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount)); (like you write below already) > + > + PcdStatus = PcdSet64S (PcdGhcbBase, (UINT64)GhcbBasePa); > + ASSERT_RETURN_ERROR (PcdStatus); > + PcdStatus = PcdSet64S (PcdGhcbSize, (UINT64)EFI_PAGES_TO_SIZE (GhcbPageCount)); > + ASSERT_RETURN_ERROR (PcdStatus); > + > + DEBUG ((DEBUG_INFO, "SEV-ES is enabled, %u GHCB pages allocated starting at 0x%lx\n", GhcbPageCount, GhcbBase)); (6) This line is too long; please try to stick with <=80 chars per line. (7) UINTN values (such as GhcbPageCount) should be converted to UINT64 explicitly, and then formatted with %lu. (8) GhcbBase is a pointer-to-void; please either format it with %p, or use GhcbBasePa. (We can rely on the latter being UINT64.) DEBUG ((DEBUG_INFO, "SEV-ES is enabled, %lu GHCB pages allocated starting at 0x%lx\n", (UINT64)GhcbPageCount, GhcbBasePa)); > + > + AsmWriteMsr64 (MSR_SEV_ES_GHCB, (UINT64)GhcbBasePa); > } > > /** > (9) If you like, you can drop the (UINT64) casts from all the "GhcbBasePa" references; all of edk2 uses PHYSICAL_ADDRESS and EFI_PHYSICAL_ADDRESS interchangeably, and the latter is UINT64 per spec. With the above updated -- (3) and (9) are optional -- Reviewed-by: Laszlo Ersek Thanks! Laszlo