Re: [edk2-devel] [RFC PATCH v2 10/44] OvmfPkg: A per-CPU variable area for #VC usage

["Laszlo Ersek" <lersek@redhat.com>]

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@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@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@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] http://mid.mail-archive.com/ad289751-c1b7-c87a-41d1-9ce9838d94f1@redhat.com
>>     https://edk2.groups.io/g/devel/message/48080
>>
>> Thanks!
>> Laszlo
>>