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 9/26/19 3:00 AM, Laszlo Ersek wrote:
> Hi Tom,
> 
> 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).
> 
> skimming this patch and the next two ones for OvmfPkg (#10, #11), I'm a
> bit lost. I'm missing a parallel between the "early X64 page tables" and
> the GHCB-related pages.
> 
> The former are set up (in X64 OVMF) in SEC, and are used throughout PEI
> until the DXE IPL builds new ones for the DXE phase. The latter also
> *seemed* to be set up in SEC, and I thought they'd be used throughout
> PEI -- I assumed the next place we'd need to massage GHCB pages would be
> similarly in the DXE IPL, or thereabouts.
> 
> However, in this patch, we seem to allocate new pages for GHCB, and the
> commit message implies they are supposed to be used during PEI. That
> diverges from how long the "early X64 page tables" are used.

At this stage, we need a GHCB page for every (v)CPU. So a new allocation
is done and then the pages are marked unencrypted. Once the new GHCB
pages are allocated, the original GHCB page for SEC is no longer needed
because the new allocation replaces it in the BSP. But the early page
table is still required in order to access all of the memory from the 2MB
range (0x800000 to 0x9fffff).

> 
> I guess this difference could be justified, especially because we do MP
> stuff in PEI. (And we need separate GHCB stuff per VCPU -- in SEC we
> only consider the BSP.)
> 
> But then, the question becomes: what exactly do we need the GHCB page
> allocated in SEC for? From the blurb, it seems that the GHCB allows the

There are lots of different ways to cause a #VC. A #VC is generated for
debug statements that use port I/O, MMIO, intercept-able MSR accesses,
CPUID instructions, WBINVD instructions, etc. Many of these things happen
during SEC. With the debug serial output enabled, over 8,000 #VC
exceptions occur before allocating the new GHCB pages in
AmdSevEsInitialize().

> guest to selectively (actively) share information with the hypervisor --
> such as (parts of?) the register file, which the hypervisor cannot
> directly access, for a SEV-ES guest.
> 
> But, we never seem to place such information at PcdOvmfSecGhcbBase (aka
> GHCB_BASE) in SEC. We program the GHCB's base address, and then we clear
> the GHCB, but that seems to be it. Do we write anything non-zero to that
> block, ever?

Yes, that happens in the SEC exception handler. When the #VC occurs, the
GHCB information is filled in and a VMGEXIT instruction is issued to exit
to the hypervisor. The hypervisor then accesses the GHCB in order to
perform the requested function.

Thanks,
Tom

> 
> Thanks
> Laszlo
> 
>> 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
>> 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>
>>  
>>  #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);
>> +  ASSERT (GhcbBase);
>> +
>> +  GhcbBasePa = (PHYSICAL_ADDRESS)(UINTN) GhcbBase;
>> +
>> +  DecryptStatus = MemEncryptSevClearPageEncMask (
>> +    0,
>> +    GhcbBasePa,
>> +    GhcbPageCount,
>> +    TRUE
>> +    );
>> +  ASSERT_RETURN_ERROR (DecryptStatus);
>> +
>> +  SetMem (GhcbBase, GhcbPageCount * SIZE_4KB, 0);
>> +
>> +  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));
>> +
>> +  AsmWriteMsr64 (MSR_SEV_ES_GHCB, (UINT64)GhcbBasePa);
>>  }
>>  
>>  /**
>>
>