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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 10/2/19 5:23 AM, Laszlo Ersek wrote:
> 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.

Will do.

> 
>> 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.

Ok, let me think about that. If ASSERT is enabled, you'll get the ASSERT
message (since the SEC GHCB is in place and OVMF is running in 64-bit
mode). If ASSERT is not enabled, then the ZeroMem will segfault on a NULL
pointer, which will give a bit more info than the CpuDeadLoop() which
would look more like a hang.

> 
> (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.

Will do.

> 
>> +
>> +  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));

Will do.

> 
> (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.

Huh, don't know how I missed that one. I'll fix that.

> 
> (7) UINTN values (such as GhcbPageCount) should be converted to UINT64
> explicitly, and then formatted with %lu.

Ok, will do (and I'll check the rest of my patches for this).

> 
> (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.

Nice, that will clean it up a bit.

Thanks, Tom

> 
> With the above updated -- (3) and (9) are optional --
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!
> Laszlo
>