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, 21 Aug 2019 08:44:04 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 342CE2A09B0; Wed, 21 Aug 2019 15:44:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-79.ams2.redhat.com [10.36.117.79]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0AB537E4E; Wed, 21 Aug 2019 15:44:01 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH 07/28] OvmfPkg/PlatformPei: Move early GDT into ram when SEV-ES is enabled To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" References: <79bac50e4cea5e261c694a2b875cc2eff32bea68.1566250534.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <4029f533-9f2a-ba71-2ba6-348187dc7684@redhat.com> Date: Wed, 21 Aug 2019 17:44:01 +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: <79bac50e4cea5e261c694a2b875cc2eff32bea68.1566250534.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 21 Aug 2019 15:44:04 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/19/19 23:35, Lendacky, Thomas wrote: > From: Tom Lendacky > > The SEV support will clear the C-bit from non-RAM areas. The early GDT > lives in a non-RAM area, so when an exception occurs (like a #VC) the GDT > will be read as un-encrypted even though it is encrypted. This will result > in a failure to be able to handle the exception. > > Move the GDT into RAM so it can be accessed without error when running as > an SEV-ES guest. > > Signed-off-by: Tom Lendacky > --- > OvmfPkg/PlatformPei/AmdSev.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c > index 87ac842a1590..5f4983fd36d8 100644 > --- a/OvmfPkg/PlatformPei/AmdSev.c > +++ b/OvmfPkg/PlatformPei/AmdSev.c > @@ -37,6 +37,8 @@ AmdSevEsInitialize ( > PHYSICAL_ADDRESS GhcbBasePa; > UINTN GhcbPageCount; > RETURN_STATUS DecryptStatus, PcdStatus; > + IA32_DESCRIPTOR Gdtr; > + VOID *Gdt; > > if (!MemEncryptSevEsIsEnabled ()) { > return; > @@ -76,6 +78,20 @@ AmdSevEsInitialize ( > DEBUG ((DEBUG_INFO, "SEV-ES is enabled, %u GHCB pages allocated starting at 0x%lx\n", GhcbPageCount, GhcbBase)); > > AsmWriteMsr64 (MSR_SEV_ES_GHCB, (UINT64)GhcbBasePa); > + > + // > + // The SEV support will clear the C-bit from the non-RAM areas. Since > + // the GDT initially lives in that area and it will be read when a #VC > + // exception happens, it needs to be moved to RAM for an SEV-ES guest. > + // > + AsmReadGdtr (&Gdtr); > + > + Gdt = AllocatePool (Gdtr.Limit + 1); > + ASSERT (Gdt); > + > + CopyMem (Gdt, (VOID *) Gdtr.Base, Gdtr.Limit + 1); > + Gdtr.Base = (UINTN) Gdt; > + AsmWriteGdtr (&Gdtr); > } > > /** > This doesn't look safe enough to me. AllocatePool() in the PEI phase means the creation of an EFI_HOB_TYPE_MEMORY_POOL HOB. The data allocated lives inside the HOB. According to the PI spec v1.7, volume 3, section "4.5.2 HOB Construction Rules": 3. HOBs may be relocated in system memory by the HOB consumer phase. HOBs must not contain pointers to other data in the HOB list, including that in other HOBs. The table must be able to be copied without requiring internal pointer adjustment. Additionally, in section "5.9 Memory Pool HOB", [...] The HOB consumer phase should be able to ignore these HOBs [...] which seems to imply that the HOB might not survive into DXE at all (the memory could be released and repurposed). I don't feel good about pointing the GDTR into such a possibly ephemeral HOB, even if we reload the GDTR with a different GDT address at a later time. Instead, I suggest AllocatePages(). AmdSevEsInitialize() is invoked as part of AmdSevInitialize(), which in turn is invoked after PublishPeiMemory() returns. Therefore, using AllocatePages() instead of AllocatePool() should be safe -- the address produced by AllocatePages() should be stable. Namely, in PI v1.7, volume 1, section "4.6 PEI Memory Services", AllocatePages() is specified as: [...] Allocation made prior to permanent memory will be migrated to permanent memory [...] After InstallPeiMemory() is called, PEI will allocate pages within the region of memory provided by InstallPeiMemory() service [...] The edk2 code agrees -- PublishPeiMemory() in OVMF's PlatformPei exercises PeiInstallPeiMemory() [MdeModulePkg/Core/Pei/Memory/MemoryServices.c], which sets "SwitchStackSignal". Although actual RAM migration doesn't happen until after PlatformPei exits, PeiAllocatePages() explicitly looks for (!PrivateData->PeiMemoryInstalled && PrivateData->SwitchStackSignal) so it will do the right thing. As a result, AllocatePages() *before* actual RAM migration (from temporary to permanent), but *after* PeiInstallPeiMemory(), will allocate permanent memory. Thanks Laszlo