From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.32846.1590419279683492025 for ; Mon, 25 May 2020 08:08:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FsF89/XJ; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590419278; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=tKr4JRzU4L2r02zFPfcDdWHYqyAC9iehN+CoaCtdpg0=; b=FsF89/XJ0W5m5S10N1jQ5h25SurFLuJd+cCs/wcRdM6ycjlhhz+HNwjYTNcJ2mwlB70mxC vbZfpxRwwZqNrp5qLaMu47xnGOUzALjH60Mp21sCR43Oo1KRp2BklKB0mDnvr4wEqhEBv6 ZCNnht5Qf/ULW/f53ViZZnI5AdfJSdc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-510-OJ7OthDxPtGmisoQ5D4MiQ-1; Mon, 25 May 2020 11:07:55 -0400 X-MC-Unique: OJ7OthDxPtGmisoQ5D4MiQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 970298018A3; Mon, 25 May 2020 15:07:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-168.ams2.redhat.com [10.36.114.168]) by smtp.corp.redhat.com (Postfix) with ESMTP id 75E745C1BB; Mon, 25 May 2020 15:07:51 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 29/46] 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 , Brijesh Singh References: <85002209886afa16aa3599e4b1cb844c06f236f5.1589925074.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <3f1b71c3-7e8f-61bf-16dd-b4b0cd7ff5e9@redhat.com> Date: Mon, 25 May 2020 17:07:50 +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: <85002209886afa16aa3599e4b1cb844c06f236f5.1589925074.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Reviewed-by: Laszlo Ersek > Signed-off-by: Tom Lendacky > --- > 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 > >