Re: [edk2-devel] [RFC PATCH v2 08/44] OvmfPkg: Create GHCB pages for use during Pei and Dxe phase

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

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.lendacky@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.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 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        | 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
<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
<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 <lersek@redhat.com>

Thanks!
Laszlo