On 06/11/20 17:25, Tom Lendacky wrote:
> On 6/11/20 4:56 AM, Laszlo Ersek wrote:
>> Hi Tom,
>>
>> On 06/05/20 15:27, Tom Lendacky 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%7C38f855613c974b9f23e108d80dedc415%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637274662138263584&sdata=QfK586IbkE%2B8zOieyD4nQJ6ALvmzE2YlsNOnB9o7lpQ%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.
>>>
>>> Four new fixed PCDs are needed to support the SEC GHCB page:
>>>    - PcdOvmfSecGhcbBase  UINT32 value that is the base address of the
>>>                          GHCB used during the SEC phase.
>>>    - PcdOvmfSecGhcbSize  UINT32 value that is the size, in bytes, of the
>>>                          GHCB area used during the SEC phase.
>>>
>>>    - PcdOvmfSecGhcbPageTableBase  UINT32 value that is address of a page
>>>                          table page used to break down the 2MB page into
>>>                          512 4K pages.
>>>    - PcdOvmfSecGhcbPageTableSize  UINT32 value that is the size, in
>>> bytes,
>>>                          of the page table page.
>>>
>>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
>>> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
>>> ---
>>>   OvmfPkg/OvmfPkg.dec                       |  9 +++
>>>   OvmfPkg/OvmfPkgX64.fdf                    |  6 ++
>>>   OvmfPkg/ResetVector/ResetVector.inf       |  5 ++
>>>   OvmfPkg/ResetVector/Ia32/PageTables64.asm | 76 ++++++++++++++++++++
>>>   OvmfPkg/ResetVector/ResetVector.nasmb     | 17 +++++
>>>   5 files changed, 113 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
>>>
>>> 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..9f86ddf6f08f 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,46 @@ 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
>>> +
>>> +    mov     ecx, GHCB_SIZE / 4
>>> +    xor     eax, eax
>>> +clearGhcbMemoryLoop:
>>> +    mov     dword[ecx * 4 + GHCB_BASE - 4], eax
>>> +    loop    clearGhcbMemoryLoop
>>> +
>>
>> This patch is now identical to v6, modulo some (welcome) commit message
>> updates, and the PCD token value updates. Therefore, we can re-add my:
>>
>> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>>
>> from v6.
>>
>> However, in the v8 discussion of the patch, you indicated that the
>> clearing loop had been in the wrong spot (in v6) -- it should have been
>> placed after the CR3 setting, apparently:
>>
>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2F1177217e-74d9-ddb5-fd38-c5ffb02de3f3%40amd.com&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C38f855613c974b9f23e108d80dedc415%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637274662138263584&amp;sdata=lV7Ou2%2F047pUTLpbutO3sXwCoxDWQrKIPDXhRh9aOqM%3D&amp;reserved=0
>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F60284&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C38f855613c974b9f23e108d80dedc415%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637274662138263584&amp;sdata=GwGXhDbxjafSgjYLP8kDI7fvkijNKgZBbgqgVuVVHOM%3D&amp;reserved=0
>>
>>
>> But in this update (v9), the clearing loop has not been moved, relative
>> to v6; it's been re-instated in the same spot. (IIUC.)
>>
>> So what is the right spot for the clearing loop after all?
> 
> After looking at the code more closely, even though the CR3 register is
> loaded, paging hasn't been enabled (it's enabled on return from this
> function/area). So the location of the clearing loop didn't matter in
> the end and so I left it in the original location. The net effect is
> that per-CPU page for the DR7 value still ends up zeroed out.

Awesome, thanks!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61167): https://edk2.groups.io/g/devel/message/61167
Mute This Topic: https://groups.io/mt/74692444/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to