Re: [edk2-devel] [PATCH v8 29/46] OvmfPkg: Create a GHCB page for use during Sec phase

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

On 05/19/20 23:50, Lendacky, Thomas wrote:
> 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 #VC exception handler routines assume that a per-CPU
> variable area is immediately after the GHCB, this per-CPU variable area
> must also 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 and per-CPU variable pages. After breaking down
> the 2MB page, update the GHCB page table entry to remove the encryption
> mask.
> 
> The GHCB page will be used by the SEC #VC exception handler. The #VC
> exception handler will fill in the necessary fields of the GHCB and exit
> to the hypervisor using the VMGEXIT instruction. The hypervisor then
> accesses the GHCB in order to perform the requested function.
> 
> Two new fixed PCDs are needed to support the SEC GHCB page:
>   - PcdOvmfSecGhcbBase  UINT64 value that is the base address of the
>                         GHCB used during the SEC phase.
>   - PcdOvmfSecGhcbSize  UINT64 value that is the size, in bytes, of the
>                         GHCB area used during the SEC phase.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/OvmfPkg.dec                       |  9 +++
>  OvmfPkg/OvmfPkgX64.fdf                    |  6 ++
>  OvmfPkg/ResetVector/ResetVector.inf       |  5 ++
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm | 70 +++++++++++++++++++++++
>  OvmfPkg/ResetVector/ResetVector.nasmb     | 17 ++++++
>  5 files changed, 107 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 65bb2bb0eb4c..02ad62ed9f43 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -281,6 +281,15 @@ [PcdsFixedAtBuild]
>    ## Number of page frames to use for storing grant table entries.
>    gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
>  
> +  ## 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|0x3a
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize|0x0|UINT32|0x3b
> +
> +  ## The base address of the SEC GHCB page used by SEV-ES.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0|UINT32|0x3c
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0|UINT32|0x3d
> +
>  [PcdsDynamic, PcdsDynamicEx]
>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10

OK, the token values have been updated, due to:

- commit 7efce2e59c20 ("OvmfPkg/PvScsiDxe: Report the number of targets
and LUNs", 2020-03-30)

- commit c4c15b870239 ("OvmfPkg/PvScsiDxe: Support sending SCSI request
and receive response", 2020-03-30)

- commit 093cceaf79b5 ("OvmfPkg/MptScsiDxe: Report targets and one LUN",
2020-05-05)

(Independently, when I reviewed what would become 505812ae1d2d
("OvmfPkg/MptScsiDxe: Implement the PassThru method", 2020-05-05), I
missed that 0x39 is followed by 0x3A, not 0x40. Oh well.)


> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index bfca1eff9e83..88b1e880e603 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|0x002000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
> +
>  0x010000|0x010000
>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>  
> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
> index b0ddfa5832a2..483fd90fe785 100644
> --- a/OvmfPkg/ResetVector/ResetVector.inf
> +++ b/OvmfPkg/ResetVector/ResetVector.inf
> @@ -26,6 +26,7 @@ [Sources]
>  [Packages]
>    OvmfPkg/OvmfPkg.dec
>    MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
>    UefiCpuPkg/UefiCpuPkg.dec
>  
>  [BuildOptions]
> @@ -33,5 +34,9 @@ [BuildOptions]
>     *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
>  
>  [Pcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index abad009f20f5..c3587a1b7814 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 + \
> @@ -75,6 +80,37 @@ NoSev:
>  SevExit:
>      OneTimeCallRet CheckSevFeature
>  
> +; Check if Secure Encrypted Virtualization - Encrypted State (SEV-ES) feature
> +; is enabled.
> +;
> +; Modified:  EAX, EBX, ECX
> +;
> +; 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
>  ;
> @@ -139,6 +175,40 @@ 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 GHCB_BASE and needs to be un-encrypted.
> +    ; This requires the 2MB page for this range be broken down into 512 4KB
> +    ; pages.  All will be marked encrypted, except for the GHCB.
> +    ;
> +    mov     ecx, (GHCB_BASE >> 21)
> +    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, GHCB_BASE & 0xFFE0_0000
> +    add     eax, PAGE_4K_PDE_ATTR
> +    mov     [ecx * 8 + GHCB_PT_ADDR - 8], eax
> +    mov     [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
> +    loop    pageTableEntries4kLoop
> +
> +    ;
> +    ; Clear the encryption bit from the GHCB entry
> +    ;
> +    mov     ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
> +    mov     [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
> +

(1) Why did you remove "clearGhcbMemoryLoop" (in the v6->v7 transition)?

I think that's exactly the clearing loop (minimally for the CPU#0
per-CPU page) that I was just looking for in point (8) under
"OvmfPkg/VmgExitLib: Add support for DR7 Read/Write NAE events" (v8 26/46).

Hm... the v7 blurb says, "Ensure the per-CPU variable page remains
encrypted". OK, but that still doesn't explain why we don't clear it
(just for the guest to see).

Also, if the patch was non-trivially modified in v7, then arguably my
R-b (given originally under "RFC PATCH v3 26/43") should have been removed.

Please re-instate "clearGhcbMemoryLoop" (and then keep the R-b).

Thanks,
Laszlo

> +SetCr3:
>      ;
>      ; Set CR3 now that the paging structures are available
>      ;
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 75cfe16654b1..bfb77e439105 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -53,8 +53,25 @@
>      %error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
>    %endif
>  
> +  %if (FixedPcdGet32 (PcdOvmfSecGhcbPageTableSize) != 0x1000)
> +    %error "This implementation inherently depends on PcdOvmfSecGhcbPageTableSize"
> +  %endif
> +
> +  %if (FixedPcdGet32 (PcdOvmfSecGhcbSize) != 0x2000)
> +    %error "This implementation inherently depends on PcdOvmfSecGhcbSize"
> +  %endif
> +
> +  %if ((FixedPcdGet32 (PcdOvmfSecGhcbBase) >> 21) != \
> +       ((FixedPcdGet32 (PcdOvmfSecGhcbBase) + FixedPcdGet32 (PcdOvmfSecGhcbSize) - 1) >> 21))
> +    %error "This implementation inherently depends on PcdOvmfSecGhcbBase not straddling a 2MB boundary"
> +  %endif
> +
>    %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
>  %include "Ia32/Flat32ToFlat64.asm"
> +
> +  %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
> +  %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
> +  %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>  %include "Ia32/PageTables64.asm"
>  %endif
>  
>