On 8/4/21 3:20 PM, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
>
> Both the TDX and SEV support needs to reserve a page in MEMFD as a work
> area. The page will contain meta data specific to the guest type.
> Currently, the SEV-ES support reserves a page in MEMFD
> (PcdSevEsWorkArea) for the work area. This page can be reused as a TDX
> work area when Intel TDX is enabled.
>
> Based on the discussion [1], it was agreed to rename the SevEsWorkArea
> to the OvmfWorkArea, and add a header that can be used to indicate the
> work area type.
>
> [1] https://edk2.groups.io/g/devel/message/78262?p=,,,20,0,0,0::\
> created,0,SNP,20,2,0,84476064
>
> Cc: James Bottomley <j...@linux.ibm.com>
> Cc: Min Xu <min.m...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Erdem Aktas <erdemak...@google.com>
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
> OvmfPkg/OvmfPkg.dec | 6 +++
> OvmfPkg/OvmfPkgX64.fdf | 9 +++-
> OvmfPkg/PlatformPei/PlatformPei.inf | 4 +-
> OvmfPkg/Include/Library/MemEncryptSevLib.h | 21 +--------
> OvmfPkg/Include/WorkArea.h | 53 ++++++++++++++++++++++
> OvmfPkg/PlatformPei/MemDetect.c | 32 ++++++-------
> 6 files changed, 85 insertions(+), 40 deletions(-)
> create mode 100644 OvmfPkg/Include/WorkArea.h
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 2ab27f0c73c2..9d31ec45c78a 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -330,6 +330,12 @@ [PcdsFixedAtBuild]
> gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|0x0|UINT32|0x47
> gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize|0x0|UINT32|0x48
>
> + ## The base address and size of the work area used during the SEC
> + # phase by the SEV and TDX supports.
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase|0|UINT32|0x49
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize|0|UINT32|0x50
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaHeaderSize|4|UINT32|0x51
> +
> [PcdsDynamic, PcdsDynamicEx]
> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index 5fa8c0895808..418e0ea5add4 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -83,7 +83,7 @@ [FD.MEMFD]
>
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>
> 0x00B000|0x001000
> -gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
>
> 0x00C000|0x001000
>
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
> @@ -99,6 +99,13 @@ [FD.MEMFD]
>
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
> FV = DXEFV
>
> +##########################################################################################
> +# SEV specific PCD settings
> +SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaHeaderSize = 0x4
> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase = $(MEMFD_BASE_ADDRESS) +
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase +
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaHeaderSize
> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize =
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize -
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaHeaderSize
> +##########################################################################################
> +
>
> ################################################################################
>
> [FV.SECFV]
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 89d1f7636870..67eb7aa7166b 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -116,8 +116,8 @@ [FixedPcd]
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
> - gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
> - gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
>
> [FeaturePcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdCsmEnable
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 76d06c206c8b..adc490e466ec 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -12,6 +12,7 @@
> #define _MEM_ENCRYPT_SEV_LIB_H_
>
> #include <Base.h>
> +#include <WorkArea.h>
>
> //
> // Define the maximum number of #VCs allowed (e.g. the level of nesting
> @@ -36,26 +37,6 @@ typedef struct {
> VOID *GhcbBackupPages;
> } SEV_ES_PER_CPU_DATA;
>
> -//
> -// Internal structure for holding SEV-ES information needed during SEC phase
> -// and valid only during SEC phase and early PEI during platform
> -// initialization.
> -//
> -// This structure is also used by assembler files:
> -// OvmfPkg/ResetVector/ResetVector.nasmb
> -// OvmfPkg/ResetVector/Ia32/PageTables64.asm
> -// OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
> -// any changes must stay in sync with its usage.
> -//
> -typedef struct _SEC_SEV_ES_WORK_AREA {
> - UINT8 SevEsEnabled;
> - UINT8 Reserved1[7];
> -
> - UINT64 RandomData;
> -
> - UINT64 EncryptionMask;
> -} SEC_SEV_ES_WORK_AREA;
> -
> //
> // Memory encryption address range states.
> //
> diff --git a/OvmfPkg/Include/WorkArea.h b/OvmfPkg/Include/WorkArea.h
> new file mode 100644
> index 000000000000..0aaad7e1da67
> --- /dev/null
> +++ b/OvmfPkg/Include/WorkArea.h
> @@ -0,0 +1,53 @@
> +/** @file
> +
> + Work Area structure definition
> +
> + Copyright (c) 2021, AMD Inc.
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef __OVMF_WORK_AREA_H__
> +#define __OVMF_WORK_AREA_H__
> +
> +//
> +// Internal structure for holding SEV-ES information needed during SEC phase
> +// and valid only during SEC phase and early PEI during platform
> +// initialization.
> +//
> +// This structure is also used by assembler files:
> +// OvmfPkg/ResetVector/ResetVector.nasmb
> +// OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +// OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
> +// any changes must stay in sync with its usage.
> +//
> +typedef struct _SEC_SEV_ES_WORK_AREA {
> + UINT8 SevEsEnabled;
> + UINT8 Reserved1[7];
> +
> + UINT64 RandomData;
> +
> + UINT64 EncryptionMask;
> +} SEC_SEV_ES_WORK_AREA;
> +
> +//
> +// Guest type for the work area
> +//
> +typedef enum {
> + GUEST_TYPE_NON_ENCRYPTED,
> + GUEST_TYPE_AMD_SEV,
> + GUEST_TYPE_INTEL_TDX,
> +
> +} GUEST_TYPE;
> +
> +//
> +// The work area structure header definition.
> +//
> +typedef struct _OVMF_WORK_AREA {
> + UINT8 GuestType;
> + UINT8 Reserved1[3];
> +
> + SEC_SEV_ES_WORK_AREA SevEsWorkArea;
> +} OVMF_WORK_AREA;
> +
> +#endif
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 2deec128f464..4c53b0fdf2fe 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -939,23 +939,21 @@ InitializeRamRegions (
> }
>
> #ifdef MDE_CPU_X64
> - if (MemEncryptSevEsIsEnabled ()) {
> - //
> - // If SEV-ES is enabled, reserve the SEV-ES work area.
> - //
> - // Since this memory range will be used by the Reset Vector on S3
> - // resume, it must be reserved as ACPI NVS.
> - //
> - // If S3 is unsupported, then various drivers might still write to the
> - // work area. We ought to prevent DXE from serving allocation requests
> - // such that they would overlap the work area.
> - //
> - BuildMemoryAllocationHob (
> - (EFI_PHYSICAL_ADDRESS)(UINTN) FixedPcdGet32 (PcdSevEsWorkAreaBase),
> - (UINT64)(UINTN) FixedPcdGet32 (PcdSevEsWorkAreaSize),
> - mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
> - );
> - }
> + //
> + // Reserve the work area.
> + //
> + // Since this memory range will be used by the Reset Vector on S3
> + // resume, it must be reserved as ACPI NVS.
> + //
> + // If S3 is unsupported, then various drivers might still write to the
> + // work area. We ought to prevent DXE from serving allocation requests
> + // such that they would overlap the work area.
> + //
> + BuildMemoryAllocationHob (
> + (EFI_PHYSICAL_ADDRESS)(UINTN) FixedPcdGet32 (PcdOvmfWorkAreaBase),
> + (UINT64)(UINTN) FixedPcdGet32 (PcdOvmfWorkAreaSize),
> + mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
> + );
If SEV-ES is enabled, then we previously had already verified that the
work area was present. Without that check now, it may not be. Just for
safety, it is probably worth replacing the:
if (MemEncryptSevEsIsEnabled ()) {
with
if (FixedPcdGet32 (PcdOvmfWorkAreaSize) != 0) {
Thanks,
Tom
> #endif
> }
> }
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78977): https://edk2.groups.io/g/devel/message/78977
Mute This Topic: https://groups.io/mt/84670984/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
