Re: [edk2-devel] [RFC PATCH 07/28] OvmfPkg/PlatformPei: Move early GDT into ram when SEV-ES is enabled

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

On 08/19/19 23:35, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> The SEV support will clear the C-bit from non-RAM areas.  The early GDT
> lives in a non-RAM area, so when an exception occurs (like a #VC) the GDT
> will be read as un-encrypted even though it is encrypted. This will result
> in a failure to be able to handle the exception.
> 
> Move the GDT into RAM so it can be accessed without error when running as
> an SEV-ES guest.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/PlatformPei/AmdSev.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index 87ac842a1590..5f4983fd36d8 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -37,6 +37,8 @@ AmdSevEsInitialize (
>    PHYSICAL_ADDRESS  GhcbBasePa;
>    UINTN             GhcbPageCount;
>    RETURN_STATUS     DecryptStatus, PcdStatus;
> +  IA32_DESCRIPTOR   Gdtr;
> +  VOID              *Gdt;
>  
>    if (!MemEncryptSevEsIsEnabled ()) {
>      return;
> @@ -76,6 +78,20 @@ AmdSevEsInitialize (
>    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);
> +
> +  //
> +  // The SEV support will clear the C-bit from the non-RAM areas. Since
> +  // the GDT initially lives in that area and it will be read when a #VC
> +  // exception happens, it needs to be moved to RAM for an SEV-ES guest.
> +  //
> +  AsmReadGdtr (&Gdtr);
> +
> +  Gdt = AllocatePool (Gdtr.Limit + 1);
> +  ASSERT (Gdt);
> +
> +  CopyMem (Gdt, (VOID *) Gdtr.Base, Gdtr.Limit + 1);
> +  Gdtr.Base = (UINTN) Gdt;
> +  AsmWriteGdtr (&Gdtr);
>  }
>  
>  /**
> 

This doesn't look safe enough to me.

AllocatePool() in the PEI phase means the creation of an
EFI_HOB_TYPE_MEMORY_POOL HOB. The data allocated lives inside the HOB.
According to the PI spec v1.7, volume 3, section "4.5.2 HOB Construction
Rules":

  3. HOBs may be relocated in system memory by the HOB consumer phase.
     HOBs must not contain pointers to other data in the HOB list,
     including that in other HOBs. The table must be able to be copied
     without requiring internal pointer adjustment.

Additionally, in section "5.9 Memory Pool HOB",

  [...] The HOB consumer phase should be able to ignore these HOBs [...]

which seems to imply that the HOB might not survive into DXE at all (the
memory could be released and repurposed).

I don't feel good about pointing the GDTR into such a possibly ephemeral
HOB, even if we reload the GDTR with a different GDT address at a later
time.

Instead, I suggest AllocatePages().

AmdSevEsInitialize() is invoked as part of AmdSevInitialize(), which in
turn is invoked after PublishPeiMemory() returns. Therefore, using
AllocatePages() instead of AllocatePool() should be safe -- the address
produced by AllocatePages() should be stable.

Namely, in PI v1.7, volume 1, section "4.6 PEI Memory Services",
AllocatePages() is specified as:

  [...] Allocation made prior to permanent memory will be migrated to
  permanent memory [...] After InstallPeiMemory() is called, PEI will
  allocate pages within the region of memory provided by
  InstallPeiMemory() service [...]

The edk2 code agrees -- PublishPeiMemory() in OVMF's PlatformPei
exercises PeiInstallPeiMemory()
[MdeModulePkg/Core/Pei/Memory/MemoryServices.c], which sets
"SwitchStackSignal". Although actual RAM migration doesn't happen until
after PlatformPei exits, PeiAllocatePages() explicitly looks for

  (!PrivateData->PeiMemoryInstalled && PrivateData->SwitchStackSignal)

so it will do the right thing. As a result, AllocatePages() *before*
actual RAM migration (from temporary to permanent), but *after*
PeiInstallPeiMemory(), will allocate permanent memory.

Thanks
Laszlo