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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

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.

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?

> 
> * 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,
Tom

> 
> [1] http://mid.mail-archive.com/ad289751-c1b7-c87a-41d1-9ce9838d94f1@redhat.com
>     https://edk2.groups.io/g/devel/message/48080
> 
> Thanks!
> Laszlo
>