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

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

On 10/2/19 7:05 AM, Laszlo Ersek via Groups.Io wrote:
> 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.

Will do.

> 
>> +  ASSERT (Gdt);
> 
> (2) Please write (Gdt != NULL).

Yup.

Thanks,
Tom

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