From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 25 Sep 2019 01:09:45 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E5C45308402D; Wed, 25 Sep 2019 08:09:44 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-25.rdu2.redhat.com [10.10.120.25]) by smtp.corp.redhat.com (Postfix) with ESMTP id A5F7060E3E; Wed, 25 Sep 2019 08:09:42 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 06/44] OvmfPkg: Create a GHCB page for use during Sec phase To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" References: From: "Laszlo Ersek" Message-ID: <14c2d3e3-5178-0454-d399-3ca0218d07c7@redhat.com> Date: Wed, 25 Sep 2019 10:09:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Wed, 25 Sep 2019 08:09:45 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/19/19 21:52, Lendacky, Thomas wrote: > From: Tom Lendacky > > 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tom Lendacky > --- > 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