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

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

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
> 
> 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.
> 
> 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/PlatformPei/AmdSev.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index 699bb8b11557..d6733447bdf2 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -37,6 +37,8 @@ AmdSevEsInitialize (
>    PHYSICAL_ADDRESS  GhcbBasePa;
>    UINTN             GhcbPageCount;
>    RETURN_STATUS     PcdStatus, DecryptStatus;
> +  IA32_DESCRIPTOR   Gdtr;
> +  VOID              *Gdt;
>  
>    if (!MemEncryptSevEsIsEnabled ()) {
>      return;
> @@ -72,6 +74,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 = AllocatePages (EFI_SIZE_TO_PAGES (Gdtr.Limit + 1));

EFI_SIZE_TO_PAGES() expects an UINTN argument. "Gdtr.Limit" is UINT16
("unsigned short"), which is promoted (as part of the integer
promotions) to INT32 ("int"). The addition is then performed in INT32.

(1) Please cast either Gdtr.Limit, or the sum, to UINTN explicitly.

> +  ASSERT (Gdt);

(2) Please write (Gdt != NULL).

> +
> +  CopyMem (Gdt, (VOID *) Gdtr.Base, Gdtr.Limit + 1);
> +  Gdtr.Base = (UINTN) Gdt;
> +  AsmWriteGdtr (&Gdtr);
>  }
>  
>  /**
> 

With those changes:

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

Thanks
Laszlo