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

["Laszlo Ersek" <lersek@redhat.com>]

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.

> 
> (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 <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