On 9/25/19 3:27 AM, Laszlo Ersek wrote:
> On 09/19/19 21:52, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lenda...@amd.com>
>>
>> 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.
>
Ok, will do. Not sure how I missed including the second paragraph under
"b".
>>
>> (c) how it is protected from the OS
>>
>> If S3 is enabled, then (1b) reserves it from the OS too.
>
> (2) s/1b/b/
Yup, I'll fix it here and in the other locations identified.
Thanks,
Tom
>
>>
>> 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 <jordan.l.jus...@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> Cc: Anthony Perard <anthony.per...@citrix.com>
>> Cc: Julien Grall <julien.gr...@arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
>> ---
>> 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 <Library/ResourcePublicationLib.h>
>> #include <Library/MtrrLib.h>
>> #include <Library/QemuFwCfgLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>>
>> #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 <ler...@redhat.com>
>
> Thanks!
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48043): https://edk2.groups.io/g/devel/message/48043
Mute This Topic: https://groups.io/mt/34203542/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
