On 05/26/20 17:41, 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.jus...@intel.com> >>> Cc: Laszlo Ersek <ler...@redhat.com> >>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >>> Reviewed-by: Laszlo Ersek <ler...@redhat.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 | 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.
Thank you, that works for me (both code-wise and process-wise). Cheers, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#60336): https://edk2.groups.io/g/devel/message/60336 Mute This Topic: https://groups.io/mt/74336585/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-