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, 25 Sep 2019 01:27:15 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 624B418C8909; Wed, 25 Sep 2019 08:27:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-25.rdu2.redhat.com [10.10.120.25]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8FAC05D9CC; Wed, 25 Sep 2019 08:27:11 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 07/44] OvmfPkg/PlatformPei: Reserve GHCB-related areas if S3 is supported 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" , Anthony Perard , Julien Grall References: <5aedd92ee8bcf72a70a0feaeb8f1a2a178cd9cb0.1568922728.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <1b2c4e93-df0d-9bb7-75d1-abc76f05460c@redhat.com> Date: Wed, 25 Sep 2019 10:27:10 +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: <5aedd92ee8bcf72a70a0feaeb8f1a2a178cd9cb0.1568922728.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.70]); Wed, 25 Sep 2019 08:27:14 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/19/19 21:52, Lendacky, Thomas wrote: > From: Tom Lendacky > > Protect the memory used by an SEV-ES guest when S3 is supported. This > includes the page table used to break down the 2MB page that contains > the GHCB so that it can be marked un-encrypted, as well as the GHCB > area. > > Regarding the lifecycle of the GHCB-related memory areas: > PcdOvmfSecGhcbPageTableBase > PcdOvmfSecGhcbBase > > (a) when and how it is initialized after first boot of the VM > > If SEV-ES is enabled, the GHCB-related areas are initialized during > the SEC phase [OvmfPkg/ResetVector/Ia32/PageTables64.asm]. > > (b) how it is protected from memory allocations during DXE > > If S3 and SEV-ES are enabled, then InitializeRamRegions() > [OvmfPkg/PlatformPei/MemDetect.c] protects the range with an AcpiNVS > memory allocation HOB, in PEI. (1) Please keep (and update, as needed) the paragraph about the "S3 disabled" case. The matching part of the whitepaper says, in (1b), """ If S3 was disabled, then this range is not protected. DXE's own page tables are first built while still in PEI (see HandOffToDxeCore() [MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c]). Those tables are located in permanent PEI memory. After CR3 is switched over to them (which occurs before jumping to the DXE core entry point), we don't have to preserve the initial tables. """ I guess we don't have to be as verbose as this. But, in case we're going to build a new GHCB for the DXE phase, and therefore we can simply forget about the early GHCB structures (with S3 disabled), we should mention that briefly. > > (c) how it is protected from the OS > > If S3 is enabled, then (1b) reserves it from the OS too. (2) s/1b/b/ > > If S3 is disabled, then the range needs no protection. Right, so this seems to be consistent with what I'm requesting under (1). > > (d) how it is accessed on the S3 resume path > > It is rewritten same as in (1a), which is fine because (1b) reserved it. (3) s/1a/a/; s/1b/b/ (Also, the original refers to (1c) rather than (1b), and that's not a typo; but this variant looks just as fine.) > > (e) how it is accessed on the warm reset path > > It is rewritten same as in (1a). (4) s/1a/a/ > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Anthony Perard > Cc: Julien Grall > Signed-off-by: Tom Lendacky > --- > OvmfPkg/PlatformPei/PlatformPei.inf | 4 ++++ > OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index 2736347a2e03..a9e424a6012a 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -84,6 +84,10 @@ [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd > gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize > gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize (5) Can you please keep these additions close to PcdOvmfSecPageTablesBase / PcdOvmfSecPageTablesSize? > diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c > index d451989f31c9..cd2e3abb7c9b 100644 > --- a/OvmfPkg/PlatformPei/MemDetect.c > +++ b/OvmfPkg/PlatformPei/MemDetect.c > @@ -32,6 +32,7 @@ Module Name: > #include > #include > #include > +#include > > #include "Platform.h" > #include "Cmos.h" > @@ -805,6 +806,28 @@ InitializeRamRegions ( > (UINT64)(UINTN) PcdGet32 (PcdOvmfSecPageTablesSize), > EfiACPIMemoryNVS > ); > + > + if (MemEncryptSevEsIsEnabled ()) { > + // > + // If SEV-ES is active, reserve the GHCB-related memory area. This > + // includes the extra page table used to break down the 2MB page > + // mapping into 4KB page entries where the GHCB resides and the > + // GHCB area itself. > + // > + // Since this memory range will be used by the Reset Vector on S3 > + // resume, it must be reserved as ACPI NVS. > + // > + BuildMemoryAllocationHob ( > + (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfSecGhcbPageTableBase), > + (UINT64)(UINTN) PcdGet32 (PcdOvmfSecGhcbPageTableSize), > + EfiACPIMemoryNVS > + ); > + BuildMemoryAllocationHob ( > + (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfSecGhcbBase), > + (UINT64)(UINTN) PcdGet32 (PcdOvmfSecGhcbSize), > + EfiACPIMemoryNVS > + ); > + } > #endif > } > > With the requested updates: Reviewed-by: Laszlo Ersek Thanks! Laszlo