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

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

On 5/26/20 10:41 AM, Tom Lendacky wrote:
> On 5/25/20 10:07 AM, Laszlo Ersek wrote:
>> On 05/19/20 23:50, Lendacky, Thomas wrote:
>>> BZ: 
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C39b71c622d2d4bbf9e5b08d800bd69a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260160817275268&sdata=hz43pd7UO60%2FWfNALLyUuUax8KX%2Bpq4SyU9NIN32Pfc%3D&reserved=0 
>>>
>>>
>>> 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 removed it because it actually wasn't clearing the GHCB at all. Since 
> this occurred before the new page tables are loaded, the page is accessed 
> encrypted. After loading the new page tables, the GHCB is now referenced 
> unencrypted and so the "zeroed" page isn't actually zeroes anymore, it is 
> cipher-text.
> 
> Since the GHCB is always cleared on #VC, I dropped it.
> 
>>
>> 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).
> 
> I'll add a loop to clear the GHCB page and the per-CPU page after 
> establishing the new page tables.
> 
>>
>> 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).
> 
> I'll actually drop your Reviewed-by: since I'll need to expand and move 
> the loop to clear the memory area from the original location in order for 
> the clearing of the pages to be correct.

It's not expanding (not sure why I typed that), just moving to after the 
setting of CR3.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
>> 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
>>>
>>