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

["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
> 
> 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.

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

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

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

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


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

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