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; Thu, 26 Sep 2019 01:17:47 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3EEE030ADBA3; Thu, 26 Sep 2019 08:17:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0BE7F19C5B; Thu, 26 Sep 2019 08:17:44 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 10/44] OvmfPkg: A per-CPU variable area for #VC usage 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: <6fac34b0-2d20-e2ff-66b6-de9bd7488f40@redhat.com> Date: Thu, 26 Sep 2019 10:17:44 +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.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Thu, 26 Sep 2019 08:17:47 +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 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? 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. If that's the case, when exactly would be the new page (at 0x80_a000) be used? * 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"). [1] http://mid.mail-archive.com/ad289751-c1b7-c87a-41d1-9ce9838d94f1@redhat.com https://edk2.groups.io/g/devel/message/48080 Thanks! Laszlo