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

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

On 9/25/19 3:09 AM, Laszlo Ersek 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
>>
>> 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 to break down the 2MB
>> region where the GHCB page lives into 4K pagetable entries.
>>
>> Create a new entry in the OVMF memory layout for the new page table
>> page and for the SEC GHCB page. After breaking down the 2MB page, update
>> the GHCB page table entry to remove the encryption mask.
>>
>> 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/OvmfPkg.dec                       | 10 +++
>>  OvmfPkg/OvmfPkgX64.fdf                    |  6 ++
>>  OvmfPkg/ResetVector/ResetVector.inf       |  4 ++
>>  OvmfPkg/ResetVector/Ia32/PageTables64.asm | 79 +++++++++++++++++++++++
>>  OvmfPkg/ResetVector/ResetVector.nasmb     | 12 ++++
>>  5 files changed, 111 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 9640360f6245..b9287a023c94 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -218,6 +218,16 @@ [PcdsFixedAtBuild]
>>    #  The value should be a multiple of 4KB.
>>    gUefiOvmfPkgTokenSpaceGuid.PcdHighPmmMemorySize|0x400000|UINT32|0x31
>>  
>> +  ## Specify the extra page table needed to mark the GHCB as unencrypted.
>> +  #  The value should be a multiple of 4KB for each.
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|0x0|UINT32|0x32
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize|0x0|UINT32|0x33
>> +
>> +  ## Specify the GHCB base address and size.
>> +  #  The value should be a multiple of 4KB for each.
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0x0|UINT32|0x34
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0x0|UINT32|0x35
>> +
>>  [PcdsDynamic, PcdsDynamicEx]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index 74407072563b..a567131a0591 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -76,6 +76,12 @@ [FD.MEMFD]
>>  0x007000|0x001000
>>  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>>  
>> +0x008000|0x001000
>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>> +
>> +0x009000|0x001000
>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> +
>>  0x010000|0x010000
>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>  
>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
>> index 960b47cd0797..80c971354176 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>> @@ -37,3 +37,7 @@ [Pcd]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index 40f7814c1134..7e346661f2c8 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 + \
>> @@ -95,6 +100,37 @@ SevExit:
>>  
>>      OneTimeCallRet CheckSevFeature
>>  
>> +; Check if Secure Encrypted Virtualization - Encrypted State (SEV-ES) feature
>> +; is enabled.
>> +;
>> +; Modified:  EAX, EBX, ECX, EDX
> 
> (1) I think we should remove EDX from this list. It is restored at the
> end of the routine. And, in pageTableEntries4kLoop, we rely on EDX
> containing the encryption mask.

Yup, I'll remove that one.

> 
>> +;
>> +; If SEV-ES is enabled then EAX will be non-zero.
>> +; If SEV-ES is disabled then EAX will be zero.
>> +;
>> +CheckSevEsFeature:
>> +    xor       eax, eax
>> +
>> +    ; SEV-ES can't be enabled if SEV isn't, so first check the encryption
>> +    ; mask.
>> +    test      edx, edx
>> +    jz        NoSevEs
>> +
>> +    ; Save current value of encryption mask
>> +    mov       ebx, edx
>> +
>> +    ; Check if SEV-ES is enabled
>> +    ;  MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
>> +    mov       ecx, 0xc0010131
>> +    rdmsr
>> +    and       eax, 2
>> +
>> +    ; Restore encryption mask
>> +    mov       edx, ebx
>> +
>> +NoSevEs:
>> +    OneTimeCallRet CheckSevEsFeature
>> +
>>  ;
>>  ; Modified:  EAX, EBX, ECX, EDX
>>  ;
>> @@ -159,6 +195,49 @@ pageTableEntriesLoop:
>>      mov     [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
>>      loop    pageTableEntriesLoop
>>  
>> +    OneTimeCall   CheckSevEsFeature
>> +    test    eax, eax
>> +    jz      SetCr3
>> +
>> +    ;
>> +    ; The initial GHCB will live at 0x809000 and needs to be un-encrypted.
> 
> (2) Can you replace 0x809000 with GHCB_BASE, in the comment?

Yes, will do.

> 
>> +    ; 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 encrypted,
>> +    ; except for the GHCB.
>> +    ;
>> +    mov     ecx, 4
> 
> (3) Can we please:
> - remove the remark "index 4 in the first 1GB page",
> - and replace the constant 4 with (GHCB_BASE >> 21), in the instruction?

Yes, will do throughout in regards to GHCB_BASE. It was my intention to
use GHCB_BASE throughout when I created it, but it looks like forgot to
make those changes.

> 
>> +    mov     eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
>> +    mov     [ecx * 8 + PT_ADDR (0x2000)], eax
>> +
>> +    ;
>> +    ; Page Table Entries (512 * 4KB entries => 2MB)
>> +    ;
>> +    mov     ecx, 512
>> +pageTableEntries4kLoop:
>> +    mov     eax, ecx
>> +    dec     eax
>> +    shl     eax, 12
>> +    add     eax, 0x800000
>> +    add     eax, PAGE_4K_PDE_ATTR
>> +    mov     [ecx * 8 + GHCB_PT_ADDR - 8], eax
>> +    mov     [ecx * 8 + GHCB_PT_ADDR - 4], edx
> 
> (4) I find it easier to understand if we stick with the pattern seen in
> the previous loop, namely [(ecx * 8 + GHCB_PT_ADDR - 8) + 4].

Ok, I'll change that to match.

> 
>> +    loop    pageTableEntries4kLoop
> 
> (5) Can you please replace the constant 0x800000 with the following
> expression:
> 
>  GHCB_BASE & 0xffe0_0000
> 
> (NASM supports the underscore too)
> 
>> +
>> +    ;
>> +    ; Clear the encryption bit from the GHCB entry (index 9 in the
>> +    ; new PTE table: (0x809000 - 0x800000) >> 12)).
>> +    ;
>> +    mov     ecx, 9
> 
> (6) I'd suggest removing the parenthesized part of the comment, with the
> constants. Instead, we should be able to explain the logic in the mov
> instruction itself:
> 
>   mov ecx, (GHCB_BASE & 0x1f_ffff) >> 12
> 
>> +    xor     edx, edx
>> +    mov     [ecx * 8 + GHCB_PT_ADDR + 4], edx
> 
> (7) It would be nice to preserve the encryption mask in EDX, as an
> invariant; we've relied on it in the present patch too.
> 
> I suggest we do:
> 
>   mov [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
> 
> Assembled / disassembled as the following 11 bytes:
> 
> 00000000  C704CD0480800000  mov dword [ecx*8+0x808004],0x0
>          -000000

Ok, will do.

> 
> 
> Alternatively, we could hoist the "xor eax, eax" from just below, and
> then store eax, not edx, to the most significant dword.

I'd rather not, just in case there are some changes in the future and
suddenly eax is no longer zero afterwards - unlikely, but it is safe.

I'll use your suggestion from above.

Thanks,
Tom

> 
>> +
>> +    mov     ecx, GHCB_SIZE / 4
>> +    xor     eax, eax
>> +clearGhcbMemoryLoop:
>> +    mov     dword[ecx * 4 + GHCB_BASE - 4], eax
>> +    loop    clearGhcbMemoryLoop
>> +
>> +SetCr3:
>>      ;
>>      ; Set CR3 now that the paging structures are available
>>      ;
>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>> index 3b213cd05ab2..8909fc9313f4 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>> @@ -53,7 +53,19 @@
>>      %error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
>>    %endif
>>  
>> +  %if (FixedPcdGet32 (PcdOvmfSecGhcbPageTableSize) != 0x1000)
>> +    %error "This implementation inherently depends on PcdOvmfSecGhcbPageTableSize"
>> +  %endif
>> +
>> +  %if (FixedPcdGet32 (PcdOvmfSecGhcbSize) != 0x1000)
>> +    %error "This implementation inherently depends on PcdOvmfSecGhcbSize"
>> +  %endif
>> +
>>    %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
>> +
>> +  %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>> +  %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>> +  %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>>  %include "Ia32/Flat32ToFlat64.asm"
>>    %define SEV_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
>>  %include "Ia32/PageTables64.asm"
>>
> 
> Thanks!
> Laszlo
>