From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 02 Oct 2019 04:51:23 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 46E35307BCC4; Wed, 2 Oct 2019 11:51:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-71.rdu2.redhat.com [10.10.120.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id A746560BE0; Wed, 2 Oct 2019 11:51:20 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 10/44] OvmfPkg: A per-CPU variable area for #VC usage To: devel@edk2.groups.io, thomas.lendacky@amd.com, "Singh, Brijesh" Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni References: From: "Laszlo Ersek" Message-ID: <280a8459-6258-5b04-8ecc-125d7d991d21@redhat.com> Date: Wed, 2 Oct 2019 13:51:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Wed, 02 Oct 2019 11:51:23 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit A few more comments: On 09/19/19 21:52, Lendacky, Thomas wrote: > From: Tom Lendacky > > 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tom Lendacky > --- > 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