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; Mon, 30 Sep 2019 12:16:03 -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 809C93082199; Mon, 30 Sep 2019 19:16:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-111.rdu2.redhat.com [10.10.121.111]) by smtp.corp.redhat.com (Postfix) with ESMTP id 42DDB60BF1; Mon, 30 Sep 2019 19:15:58 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 10/44] OvmfPkg: A per-CPU variable area for #VC usage To: "Lendacky, Thomas" , "devel@edk2.groups.io" Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" References: <6fac34b0-2d20-e2ff-66b6-de9bd7488f40@redhat.com> From: "Laszlo Ersek" Message-ID: <84459a29-fbf2-d7ad-f66a-9877e28729b4@redhat.com> Date: Mon, 30 Sep 2019 21:15:58 +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.42]); Mon, 30 Sep 2019 19:16:02 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/26/19 16:46, Lendacky, Thomas wrote: > On 9/26/19 3:17 AM, Laszlo Ersek wrote: >> On 09/19/19 21:52, Lendacky, Thomas wrote: >>> From: Tom Lendacky >>> >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 >>> >>> A per-CPU implementation for holding values specific to a CPU when >>> running as an SEV-ES guest, specifically to hold the Debug Register >>> value. Allocate an extra page immediately after the GHCB page for each >>> AP. >>> >>> Using the page after the GHCB ensures that it is unique per AP. But, >>> it also ends up being marked shared/unencrypted when it doesn't need to >>> be. It is possible during PEI to mark only the GHCB pages as shared (and >>> that is done), but DXE is not as easy. There needs to be a way to change >>> the pagetables created for DXE using CreateIdentityMappingPageTables() >>> before switching to them. >>> >>> Cc: Jordan Justen >>> Cc: Laszlo Ersek >>> Cc: Ard Biesheuvel >>> Signed-off-by: Tom Lendacky >>> --- >>> OvmfPkg/OvmfPkgX64.fdf | 2 +- >>> OvmfPkg/PlatformPei/AmdSev.c | 2 +- >>> OvmfPkg/ResetVector/ResetVector.nasmb | 2 +- >>> 3 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >>> index a567131a0591..84716952052d 100644 >>> --- a/OvmfPkg/OvmfPkgX64.fdf >>> +++ b/OvmfPkg/OvmfPkgX64.fdf >>> @@ -79,7 +79,7 @@ [FD.MEMFD] >>> 0x008000|0x001000 >>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize >>> >>> -0x009000|0x001000 >>> +0x009000|0x002000 >>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize >>> >>> 0x010000|0x010000 >>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c >>> index 30c0e4af7252..699bb8b11557 100644 >>> --- a/OvmfPkg/PlatformPei/AmdSev.c >>> +++ b/OvmfPkg/PlatformPei/AmdSev.c >>> @@ -48,7 +48,7 @@ AmdSevEsInitialize ( >>> // >>> // Allocate GHCB pages. >>> // >>> - GhcbPageCount = mMaxCpuCount; >>> + GhcbPageCount = mMaxCpuCount * 2; >>> GhcbBase = AllocatePages (GhcbPageCount); >>> ASSERT (GhcbBase); >>> >>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb >>> index 8909fc9313f4..d7c0ab3ada00 100644 >>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb >>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb >>> @@ -57,7 +57,7 @@ >>> %error "This implementation inherently depends on PcdOvmfSecGhcbPageTableSize" >>> %endif >>> >>> - %if (FixedPcdGet32 (PcdOvmfSecGhcbSize) != 0x1000) >>> + %if (FixedPcdGet32 (PcdOvmfSecGhcbSize) != 0x2000) >>> %error "This implementation inherently depends on PcdOvmfSecGhcbSize" >>> %endif >>> >>> >> >> In connection to my question at [1]: >> >> * Why do we add the extra page to SEC as well? > > We add the extra page because it may be referenced should a read or write > to DR7 be done during SEC. Based on the GHCB protocol, we need to cache > the value written (and not actually update the DR7 register) and return > it on read. > >> >> I thought that, after patch 4 ("OvmfPkg/ResetVector: Add support for a >> 32-bit SEV check"), we were all set for handling #VC, for the time of >> the initial SEV check; furthermore, that only CPUID would cause a #VC. > > Patch #4 covers the small window where the SEV support check is being done > in 32-bit mode in order to build the page tables for 64-bit mode. The > exception handling support is very specific at this stage to perform just > the GHCB CPUID protocol because we are not running in 64-bit mode and so a > GHCB page can't be used because it can't be shared with the hypervisor. > >> >> If that's the case, when exactly would be the new page (at 0x80_a000) >> be used? > > Patch #17 (UefiCpuPkg/CpuExceptionHandler: Add #VC exception handling for > Sec phase) is where the SEC exception handling is enabled which will use > the new pages at 0x80_9000 and 0x80_a000. The GHCB page has a specific > format and we can't store data in it, so another page is needed for the > DR7 data. Thanks, that seems to confirm my understanding of your other reply. > It would be nice if EDK2 had support for per-CPU variables so that this > extra page wouldn't be required. > > And since the GHCB_BASE is used by the SEC exception handler, I probably > need to rename PcdOvmfSecGhcbBase/Size to PcdUefiCpuSecGhcbBase/Size and > define them under UefiCpuPkg and just initialize them in the OvmfPkg, > right? Yes, that appears correct (also aligned with what I responded to your other email) -- UefiCpuPkg would offer the feature, and platform code in OvmfPkg would put it to use. > >> >> * Assuming we really need PcdOvmfSecGhcbSize = 0x002000, it is now >> theoretically possible that the 8KB area straddles a 2MB page >> boundary. >> >> Obviously we don't want to accommodate that corner case, but we should >> catch it. I think we should enforce -- with an %if / %error -- >> something like: >> >> (FixedPcdGet32 (PcdOvmfSecGhcbBase) >> 21) == >> ((FixedPcdGet32 (PcdOvmfSecGhcbBase) + FixedPcdGet32 (PcdOvmfSecGhcbSize) - 1) >> 21) >> >> That sanity check is likely best to squash into patch 6 ("OvmfPkg: >> Create a GHCB page for use during Sec phase"). > > Yup, I can add that. Thanks! Laszlo >> >> [1] http://mid.mail-archive.com/ad289751-c1b7-c87a-41d1-9ce9838d94f1@redhat.com >> https://edk2.groups.io/g/devel/message/48080 >> >> Thanks! >> Laszlo >>