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 <thomas.lenda...@amd.com> > > 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 <jordan.l.jus...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> > --- > 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 <bd38da31-0985-2ffc-b60d-f867a0218ab2@redhat.com">http://mid.mail-archive.com/bd38da31-0985-2ffc-b60d-f867a0218ab2@redhat.com>, 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 <PiPei.h> > #include <Register/Amd/Cpuid.h> > #include <Register/Cpuid.h> > +#include <Register/Amd/Msr.h> > +#include <Library/BaseMemoryLib.h> > +#include <Library/MemoryAllocationLib.h> (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 <4029f533-9f2a-ba71-2ba6-348187dc7684@redhat.com">http://mid.mail-archive.com/4029f533-9f2a-ba71-2ba6-348187dc7684@redhat.com>. > + 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 <ler...@redhat.com> Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48366): https://edk2.groups.io/g/devel/message/48366 Mute This Topic: https://groups.io/mt/34203543/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
