Re: [edk2-devel] [PATCH v8 31/46] OvmfPkg: Create GHCB pages for use during Pei and Dxe phase

["Laszlo Ersek" <lersek@redhat.com>]

On 05/19/20 23:50, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> Allocate memory for the GHCB pages and the per-CPU variable 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).
> 
> The per-CPU variable page needs to be unique per AP. Using the page after
> the GHCB ensures that it is unique per AP. Only the GHCB page is marked as
> shared, keeping the per-CPU variable page encyrpted. The same logic is
> used in DXE using CreateIdentityMappingPageTables() before switching to
> the DXE pagetables.
> 
> The GHCB pages (one per vCPU) will be used by the PEI and DXE #VC
> exception handlers. The #VC exception handler will fill in the necessary
> fields of the GHCB and exit to the hypervisor using the VMGEXIT
> instruction. The hypervisor then accesses the GHCB associated with the
> vCPU in order to perform the requested function.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc             |  2 ++
>  OvmfPkg/OvmfPkgIa32X64.dsc          |  2 ++
>  OvmfPkg/OvmfPkgX64.dsc              |  2 ++
>  OvmfPkg/PlatformPei/PlatformPei.inf |  2 ++
>  OvmfPkg/PlatformPei/AmdSev.c        | 45 ++++++++++++++++++++++++++++-
>  5 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index b289260c1f2d..f849a7d698cd 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -611,6 +611,8 @@ [PcdsDynamicDefault]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>  
>    # Set SEV-ES defaults
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled|0
>  
>  !if $(SMM_REQUIRE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 00faf5036670..d1b4ffedc6af 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -623,6 +623,8 @@ [PcdsDynamicDefault]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>  
>    # Set SEV-ES defaults
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled|0
>  
>  !if $(SMM_REQUIRE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 58671828c5fe..ebc903d922cf 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -621,6 +621,8 @@ [PcdsDynamicDefault]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>  
>    # Set SEV-ES defaults
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled|0
>  
>  !if $(SMM_REQUIRE) == TRUE
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index a54d10ba90d5..4742e1bdf42b 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -102,6 +102,8 @@ [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
>    gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize
>    gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index 4dc5340caa7a..4fd4534cabea 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -10,12 +10,15 @@
>  // The package level header files this module uses
>  //
>  #include <IndustryStandard/Q35MchIch9.h>
> +#include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/HobLib.h>
>  #include <Library/MemEncryptSevLib.h>
> +#include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
>  #include <PiPei.h>
>  #include <Register/Amd/Cpuid.h>
> +#include <Register/Amd/Msr.h>
>  #include <Register/Cpuid.h>
>  #include <Register/Intel/SmramSaveStateMap.h>
>  
> @@ -32,7 +35,10 @@ AmdSevEsInitialize (
>    VOID
>    )
>  {
> -  RETURN_STATUS     PcdStatus;
> +  VOID              *GhcbBase;
> +  PHYSICAL_ADDRESS  GhcbBasePa;
> +  UINTN             GhcbPageCount, PageCount;
> +  RETURN_STATUS     PcdStatus, DecryptStatus;
>  
>    if (!MemEncryptSevEsIsEnabled ()) {
>      return;
> @@ -40,6 +46,43 @@ AmdSevEsInitialize (
>  
>    PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
>    ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  //
> +  // Allocate GHCB and per-CPU variable pages.
> +  //
> +  GhcbPageCount = mMaxCpuCount * 2;
> +  GhcbBase = AllocatePages (GhcbPageCount);
> +  ASSERT (GhcbBase != NULL);
> +
> +  GhcbBasePa = (PHYSICAL_ADDRESS)(UINTN) GhcbBase;
> +
> +  //
> +  // Each vCPU gets two consecutive pages, the first is the GHCB and the
> +  // second is the per-CPU variable page. Loop through the allocation and
> +  // only clear the encryption mask for the GHCB pages.
> +  //
> +  for (PageCount = 0; PageCount < GhcbPageCount; PageCount += 2) {
> +    DecryptStatus = MemEncryptSevClearPageEncMask (
> +      0,
> +      GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
> +      1,
> +      TRUE
> +      );
> +    ASSERT_RETURN_ERROR (DecryptStatus);
> +  }

This part is new in v7 (in sync with the updated commit message).

It looks OK to me, but please don't carry over R-b tags given previously
when modifying patches non-trivially.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


> +
> +  ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount));
> +
> +  PcdStatus = PcdSet64S (PcdGhcbBase, GhcbBasePa);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +  PcdStatus = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE (GhcbPageCount));
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  DEBUG ((DEBUG_INFO,
> +    "SEV-ES is enabled, %lu GHCB pages allocated starting at 0x%p\n",
> +    (UINT64)GhcbPageCount, GhcbBase));
> +
> +  AsmWriteMsr64 (MSR_SEV_ES_GHCB, GhcbBasePa);
>  }
>  
>  /**
>