Re: [edk2-devel] [RFC PATCH 04/28] OvmfPkg: Create a GHCB page for use during Sec phase

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

On 8/21/19 9:25 AM, Laszlo Ersek via Groups.Io wrote:
> On 08/19/19 23:35, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> A GHCB page is needed during the Sec phase, so this new page must be
>> created.  Since the GHCB must be marked as an un-encrypted, or shared,
>> page, an additional pagetable page is required so break down the 2MB
>> region where the GHCB page lives into 4K pagetable entries.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  OvmfPkg/OvmfPkg.dec                        |  5 +++
>>  OvmfPkg/OvmfPkgX64.fdf                     | 11 ++++---
>>  OvmfPkg/PlatformPei/PlatformPei.inf        |  2 ++
>>  OvmfPkg/ResetVector/ResetVector.inf        |  2 ++
>>  UefiCpuPkg/Include/Register/Amd/Fam17Msr.h | 28 ++++++++++++++++
>>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 37 +++++++++++++++++++++-
>>  OvmfPkg/ResetVector/ResetVector.nasmb      |  2 +-
>>  7 files changed, 81 insertions(+), 6 deletions(-)
> 
> I've skipped patches 02 and 03 for now, because I'll have to go through
> them with a fine toothed comb -- in a subsequent submission, most
> probably. I'm just trying to provide formal comments, so that I do the
> actual review more easily, later.
> 
> As I requested under the blurb, this patch should be split in at least
> three parts, if possible -- OvmfPkg/PlatformPei, OvmfPkg/ResetVector,
> UefiCpuPkg. (The DEC and FDF changes can be kept squashed with the
> OvmfPkg patch that seems more suitable for that.)

Ok.

> 
> ... Having said that, why do you add PCDs to the PlatformPei INF file?
> The code in PlatformPei doesn't change. Could be a leftover from an
> earlier (abandoned) approach.

Yeah, most likely. I'll remove that.

Thanks,
Tom

> 
> Thanks
> Laszlo
> 
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 9640360f6245..2ead9a944af4 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -218,6 +218,11 @@ [PcdsFixedAtBuild]
>>    #  The value should be a multiple of 4KB.
>>    gUefiOvmfPkgTokenSpaceGuid.PcdHighPmmMemorySize|0x400000|UINT32|0x31
>>  
>> +  ## Specify the GHCB base address and size.
>> +  #  The value should be a multiple of 4KB for each.
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0x0|UINT32|0x32
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0x0|UINT32|0x33
>> +
>>  [PcdsDynamic, PcdsDynamicEx]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index 74407072563b..2a2427092382 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -67,13 +67,16 @@ [FD.MEMFD]
>>  BlockSize     = 0x10000
>>  NumBlocks     = 0xC0
>>  
>> -0x000000|0x006000
>> +0x000000|0x007000
>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>>  
>> -0x006000|0x001000
>> -gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>> -
>>  0x007000|0x001000
>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> +
>> +0x008000|0x001000
>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>> +
>> +0x009000|0x001000
>>  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>>  
>>  0x010000|0x010000
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index d9fd9c8f05b3..aed1f64b7c93 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -72,6 +72,8 @@ [Pcd]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>>    gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
>> index 960b47cd0797..d66f4dc29737 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>> @@ -37,3 +37,5 @@ [Pcd]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> diff --git a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
>> index 37b935dcdb30..55a5723e164e 100644
>> --- a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
>> +++ b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
>> @@ -17,6 +17,34 @@
>>  #ifndef __FAM17_MSR_H__
>>  #define __FAM17_MSR_H__
>>  
>> +/**
>> +  Secure Encrypted Virtualization - Encrypted State (SEV-ES) GHCB register
>> +
>> +**/
>> +#define MSR_SEV_ES_GHCB                    0xc0010130
>> +
>> +/**
>> +  MSR information returned for #MSR_SEV_ES_GHCB
>> +**/
>> +typedef union {
>> +  struct {
>> +    UINT32  GhcbNegotiateBit:1;
>> +
>> +    UINT32  Reserved:31;
>> +  } Bits;
>> +
>> +  struct {
>> +    UINT8   Reserved[3];
>> +    UINT8   SevEncryptionBitPos;
>> +    UINT16  SevEsProtocolMin;
>> +    UINT16  SevEsProtocolMax;
>> +  } GhcbProtocol;
>> +
>> +  VOID    *Ghcb;
>> +
>> +  UINT64  GhcbPhysicalAddress;
>> +} MSR_SEV_ES_GHCB_REGISTER;
>> +
>>  /**
>>    Secure Encrypted Virtualization (SEV) status register
>>  
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index c6071fe934de..fd4d5b1d8661 100644
>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> @@ -21,6 +21,11 @@ BITS    32
>>  %define PAGE_2M_MBO            0x080
>>  %define PAGE_2M_PAT          0x01000
>>  
>> +%define PAGE_4K_PDE_ATTR (PAGE_ACCESSED + \
>> +                          PAGE_DIRTY + \
>> +                          PAGE_READ_WRITE + \
>> +                          PAGE_PRESENT)
>> +
>>  %define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \
>>                            PAGE_ACCESSED + \
>>                            PAGE_DIRTY + \
>> @@ -120,7 +125,7 @@ SevNotActive:
>>      ; more permanent location by DxeIpl.
>>      ;
>>  
>> -    mov     ecx, 6 * 0x1000 / 4
>> +    mov     ecx, 7 * 0x1000 / 4
>>      xor     eax, eax
>>  clearPageTablesMemoryLoop:
>>      mov     dword[ecx * 4 + PT_ADDR (0) - 4], eax
>> @@ -157,6 +162,36 @@ pageTableEntriesLoop:
>>      mov     [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
>>      loop    pageTableEntriesLoop
>>  
>> +    ;
>> +    ; The GHCB will live at 0x807000 (just after the page tables)
>> +    ; and needs to be un-encrypted.  This requires the 2MB page
>> +    ; (index 4 in the first 1GB page) for this range be broken down
>> +    ; into 512 4KB pages.  All will be marked as encrypted, except
>> +    ; for the GHCB.
>> +    ;
>> +    mov     ecx, 4
>> +    mov     eax, PT_ADDR (0x6000) + PAGE_PDP_ATTR
>> +    mov     [ecx * 8 + PT_ADDR (0x2000)], eax
>> +
>> +    mov     ecx, 512
>> +pageTableEntries4kLoop:
>> +    mov     eax, ecx
>> +    dec     eax
>> +    shl     eax, 12
>> +    add     eax, 0x800000
>> +    add     eax, PAGE_4K_PDE_ATTR
>> +    mov     [ecx * 8 + PT_ADDR (0x6000 - 8)], eax
>> +    mov     [(ecx * 8 + PT_ADDR (0x6000 - 8)) + 4], edx
>> +    loop    pageTableEntries4kLoop
>> +
>> +    ;
>> +    ; Clear the encryption bit from the GHCB entry (index 7 in the
>> +    ; new PTE table - (0x807000 - 0x800000) >> 12).
>> +    ;
>> +    mov     ecx, 7
>> +    xor     edx, edx
>> +    mov     [(ecx * 8 + PT_ADDR (0x6000)) + 4], edx
>> +
>>      ;
>>      ; Set CR3 now that the paging structures are available
>>      ;
>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>> index 3b213cd05ab2..56d9b86ed943 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>> @@ -49,7 +49,7 @@
>>  %ifdef ARCH_X64
>>    #include <AutoGen.h>
>>  
>> -  %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x6000)
>> +  %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x7000)
>>      %error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
>>    %endif
>>  
>>
> 
> 
> 
>