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.
>
>>
>> * 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 (#48291): https://edk2.groups.io/g/devel/message/48291
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]
-=-=-=-=-=-=-=-=-=-=-=-
