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

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

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.

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


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

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.

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

Thanks
Laszlo