Re: [edk2-devel] [RFC PATCH v2 07/44] OvmfPkg/PlatformPei: Reserve GHCB-related areas if S3 is supported

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 9/25/19 3:27 AM, Laszlo Ersek wrote:
> On 09/19/19 21:52, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@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.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@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 <lersek@redhat.com>
> 
> Thanks!
> Laszlo
>