[edk2-devel] [RFC PATCH v2 10/44] OvmfPkg: A per-CPU variable area for #VC usage
Lendacky, Thomas
thomas.lendacky at amd.com
Mon Sep 30 19:52:39 UTC 2019
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.lendacky at 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.justen at intel.com>
>>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky at 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] 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/1813853
Mute #vc: https://groups.io/mk?hashtag=vc&subid=3943202
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list