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

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

On 10/2/19 6:51 AM, Laszlo Ersek wrote:
> A few more comments:
> 
> 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
>>
>>
> 
> (1) I think it makes sense to split this patch in two, one half for SEC,
> another for PlatformPei.

With the way I'm re-ordering the patches (building the exception handling
up first), the per-CPU allocation will just be part of the overall GHCB
allocation in each phase and be separate patches.

> 
> (2) The PlatformPei hunk makes me realize that
> "mS3AcpiReservedMemorySize" in PublishPeiMemory() is already
> proportional to "mMaxCpuCount".
> 
> The current coefficient is PcdCpuApStackSize (= 32KB). If we're adding
> 8KB (2 pages) per CPU, for SEV-ES, that's not negligible (1/4th
> increase).
> 
> Can you please extend both patch#8, and the PlatformPei hunk split out
> of patch#10 (= this patch), to increase "mS3AcpiReservedMemorySize" in
> PublishPeiMemory(), if MemEncryptSevEsIsEnabled()?
> 
>    if (mS3Supported) {
>      mS3AcpiReservedMemorySize = ...;
> +    if (MemEncryptSevEsIsEnabled ()) {
> +      mS3AcpiReservedMemorySize += ...
> +    }
>      mS3AcpiReservedMemoryBase = ...
> 
> Otherwise, AmdSevEsInitialize() could run out of permanent PEI RAM
> during S3 resume.

Ok, I'll do that if we decide that S3 is supported after discussing it
some more.

> 
> 
> ... Side question: actually, do we support S3 with SEV enabled, at the
> moment? Last week or so I tried to test it, and it didn't work. I don't
> remember if we *intended* to support S3 in SEV guests at all. If we
> never cared, then we should document that, plus I shouldn't make the
> SEV-ES work needlessly difficult with S3 remarks... Brijesh, what's your
> recollection?
> 
> If the intent has always been to ignore S3 in SEV guests, then we should
> modify the S3Verification() function to catch QEMU configs where both
> features are enabled, and force the user to disable at least one of
> them. Otherwise, the user might suspend the OS to S3, and then lose data
> when resume fails. In such cases, the user should be forced -- during
> early boot -- to explicitly disable S3 on the QEMU cmdline, and to
> re-launch the guest. And then the OS won't ever attempt S3.
> 
> Hm.... I've now found some internal correspondence at Red Hat, from Aug
> 2017. I wrote,
> 
>> With SEV enabled, the S3 boot script would have to manipulate page
>> tables (which might require more memory pre-allocation), in order to
>> continue using the currently pre-reserved memory areas for guest-host
>> communication during S3 resume.

I guess I need to understand more about this. Does the page table
manipulation occur in the guest or hypervisor? If in the guest, then that
is ok. But the page tables can't be successfully manipulated by the
hypervisor.

>>
>> This kind of page table manipulation is very difficult to do with the
>> currently specified / standardized boot script opcodes.
>> EFI_BOOT_SCRIPT_DISPATCH_2_OPCODE *might* prove usable to call custom
>> code during S3 resume, from the boot script, but the callee seems to
>> need a custom assembly trampoline, and likely some magic for code
>> relocation too (or the code must be position independent). One example
>> seems to exist in the edk2 tree, but for OVMF this is uncharted
>> territory.
> 
> And then the participants in that discussion seemed to set S3+SEV aside,
> indefinitely.
> 
> ... I've also found some S3 references in the following blurb:
> 
> 
> http://mid.mail-archive.com/1499351394-1175-1-git-send-email-brijesh.singh@amd.com
> 
> We ended up not adding any SEV-related code to
> "OvmfPkg/Library/QemuFwCfgS3Lib", so I think S3 must have remained out
> of scope.

Brijesh commented in the referenced link that he was able to do
suspend/resume successfully. It's possible that some later changes caused
that to fail?

Maybe we need to understand how you did your S3 test vs. how Brijesh did
his.

> 
> If we agree now that S3 is out of scope (for both SEV and SEV-ES), then:
> 
> - I think we should ignore all S3-related code paths in this series,
> 
> - we should drop patches already written for S3 (sorry about that!),
> 
> - we should extend S3Verification() like described above.

It's probably worth doing this as the only S3-related patch in this series
until we understand the complete SEV-ES / S3 requirements. I'm a bit
hesitant to include base SEV in this until we discuss some more.

> 
> I apologize if my reviews are a bit incoherent; I can track only so many
> things in parallel :(

No worries, they're not!

Thanks,
Tom

> 
> Thanks
> Laszlo
>