On 9/30/19 2:15 PM, Laszlo Ersek via Groups.Io wrote: > 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 <thomas.lenda...@amd.com> >>>> >>>> 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 <jordan.l.jus...@intel.com> >>>> Cc: Laszlo Ersek <ler...@redhat.com> >>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >>>> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> >>>> --- >>>> 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.
Ok, let me see how I can rework this area. Thanks, Tom > >> >>> >>> * 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] >>> ad289751-c1b7-c87a-41d1-9ce9838d94f1@redhat.com">http://mid.mail-archive.com/ad289751-c1b7-c87a-41d1-9ce9838d94f1@redhat.com >>> https://edk2.groups.io/g/devel/message/48080 >>> >>> Thanks! >>> Laszlo >>> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48297): https://edk2.groups.io/g/devel/message/48297 Mute This Topic: https://groups.io/mt/34203546/21656 Mute #vc: https://groups.io/mk?hashtag=vc&subid=3846945 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-