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/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@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.

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
>>>
> 
> 
> 
>