[PATCH 0/3] reuse the SevEsWork area

[PATCH 0/3] reuse the SevEsWork area

[Brijesh Singh]

Based on the discussion on the mailing list, we agreed that instead
of wasting extra page in the MEMFD, we can reuse the SevEsWorkArea
buffer for the TDX. To avoid any confusion, lets introduce a OvmfWorkArea
that will contains 32 bytes of header followed by the actual workarea.

While at it, move the code to clear the GHCB page from PageTable build
to AmdSev.asm.

I have used the existing TDX BZ for it because the request came
during the TDX patch review. if anyone have concern please let me know
and I will happily create a new BZ.

Full tree is at: https://github.com/AMDESE/ovmf/tree/sev-new-work-area

Brijesh Singh (3):
  OvmfPkg: introduce a common work area
  OvmfPkg/ResetVector: update SEV support to use new work area format
  OvmfPkg/ResetVector: move the GHCB page setup in AmdSev.asm

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Erdem Aktas <erdemaktas@google.com>

 OvmfPkg/OvmfPkg.dec                        |   6 ++
 OvmfPkg/OvmfPkgX64.fdf                     |   9 +-
 OvmfPkg/PlatformPei/PlatformPei.inf        |   4 +-
 OvmfPkg/ResetVector/ResetVector.inf        |   1 +
 OvmfPkg/Sec/SecMain.inf                    |   1 +
 OvmfPkg/Include/Library/MemEncryptSevLib.h |  21 +---
 OvmfPkg/Include/WorkArea.h                 |  53 ++++++++++
 OvmfPkg/PlatformPei/MemDetect.c            |  32 +++---
 OvmfPkg/Sec/SecMain.c                      |  25 ++++-
 OvmfPkg/ResetVector/Ia32/AmdSev.asm        | 111 +++++++++++++++++----
 OvmfPkg/ResetVector/Ia32/PageTables64.asm  |  57 ++---------
 OvmfPkg/ResetVector/ResetVector.nasmb      |   1 +
 12 files changed, 213 insertions(+), 108 deletions(-)
 create mode 100644 OvmfPkg/Include/WorkArea.h

-- 
2.17.1

[PATCH 1/3] OvmfPkg: introduce a common work area

[Brijesh Singh]

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 <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@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
+      );
 #endif
   }
 }
-- 
2.17.1

Re: [PATCH 1/3] OvmfPkg: introduce a common work area

[Lendacky, Thomas]

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 <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@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
>    }
>  }
> 

Re: [PATCH 1/3] OvmfPkg: introduce a common work area

[Brijesh Singh]

On 8/9/21 11:40 AM, Tom Lendacky wrote:
> 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 <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@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) {
> 

Noted.


thanks

[PATCH 2/3] OvmfPkg/ResetVector: update SEV support to use new work area format

[Brijesh Singh]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

Update the SEV support to switch to using the newer work area format.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/ResetVector/ResetVector.inf       |  1 +
 OvmfPkg/Sec/SecMain.inf                   |  1 +
 OvmfPkg/Sec/SecMain.c                     | 25 ++++++++++++++++++++++-
 OvmfPkg/ResetVector/Ia32/AmdSev.asm       |  8 ++++++++
 OvmfPkg/ResetVector/Ia32/PageTables64.asm |  4 ++++
 OvmfPkg/ResetVector/ResetVector.nasmb     |  1 +
 6 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index d028c92d8cfa..6ec9cca40c3a 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -34,6 +34,7 @@ [BuildOptions]
    *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
 
 [Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
   gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
index 7f78dcee2772..82910dcbd5c2 100644
--- a/OvmfPkg/Sec/SecMain.inf
+++ b/OvmfPkg/Sec/SecMain.inf
@@ -56,6 +56,7 @@ [Ppis]
   gEfiTemporaryRamSupportPpiGuid                # PPI ALWAYS_PRODUCED
 
 [Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
   gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index 9db67e17b2aa..dda572c7ad7d 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -807,6 +807,29 @@ SevEsProtocolCheck (
   Ghcb->GhcbUsage = GHCB_STANDARD_USAGE;
 }
 
+/**
+ Determine if the SEV is active.
+
+ During the early booting, GuestType is set in the work area. Verify that it
+ is an SEV guest.
+
+ @retval TRUE   SEV is enabled
+ @retval FALSE  SEV is not enabled
+
+**/
+STATIC
+BOOLEAN
+IsSevGuest (
+  VOID
+  )
+{
+  OVMF_WORK_AREA             *WorkArea;
+
+  WorkArea = (OVMF_WORK_AREA *) FixedPcdGet32 (PcdOvmfWorkAreaBase);
+
+  return ((WorkArea != NULL) && (WorkArea->GuestType == GUEST_TYPE_AMD_SEV));
+}
+
 /**
   Determine if SEV-ES is active.
 
@@ -828,7 +851,7 @@ SevEsIsEnabled (
 
   SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
 
-  return ((SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0));
+  return (((IsSevGuest()) && SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0));
 }
 
 VOID
diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
index aa95d06eaddb..87d81b01e263 100644
--- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
+++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
@@ -171,6 +171,9 @@ CheckSevFeatures:
     bt        eax, 0
     jnc       NoSev
 
+    ; Set the work area header to indicate that the SEV is enabled
+    mov     byte[WORK_AREA_GUEST_TYPE], 1
+
     ; Check for SEV-ES memory encryption feature:
     ; CPUID  Fn8000_001F[EAX] - Bit 3
     ;   CPUID raises a #VC exception if running as an SEV-ES guest
@@ -257,6 +260,11 @@ SevExit:
 IsSevEsEnabled:
     xor       eax, eax
 
+    ; During CheckSevFeatures, the WORK_AREA_GUEST_TYPE is set
+    ; to 1 if SEV is enabled.
+    cmp       byte[WORK_AREA_GUEST_TYPE], 1
+    jne       SevEsDisabled
+
     ; During CheckSevFeatures, the SEV_ES_WORK_AREA was set to 1 if
     ; SEV-ES is enabled.
     cmp       byte[SEV_ES_WORK_AREA], 1
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index eacdb69ddb9f..f688909f1c7d 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -42,6 +42,10 @@ BITS    32
 ;
 SetCr3ForPageTables64:
 
+    ; Clear the WorkArea header. The SEV probe routines will populate the
+    ; work area when detected.
+    mov     byte[WORK_AREA_GUEST_TYPE], 0
+
     OneTimeCall   CheckSevFeatures
     xor     edx, edx
     test    eax, eax
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index acec46a32450..d1d800c56745 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -72,6 +72,7 @@
   %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
   %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
   %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
+  %define WORK_AREA_GUEST_TYPE (FixedPcdGet32 (PcdOvmfWorkAreaBase))
   %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
   %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
   %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
-- 
2.17.1

Re: [PATCH 2/3] OvmfPkg/ResetVector: update SEV support to use new work area format

[Lendacky, Thomas]

On 8/4/21 3:20 PM, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
> 
> Update the SEV support to switch to using the newer work area format.
> 
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/ResetVector/ResetVector.inf       |  1 +
>  OvmfPkg/Sec/SecMain.inf                   |  1 +
>  OvmfPkg/Sec/SecMain.c                     | 25 ++++++++++++++++++++++-
>  OvmfPkg/ResetVector/Ia32/AmdSev.asm       |  8 ++++++++
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm |  4 ++++
>  OvmfPkg/ResetVector/ResetVector.nasmb     |  1 +
>  6 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
> index d028c92d8cfa..6ec9cca40c3a 100644
> --- a/OvmfPkg/ResetVector/ResetVector.inf
> +++ b/OvmfPkg/ResetVector/ResetVector.inf
> @@ -34,6 +34,7 @@ [BuildOptions]
>     *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
>  
>  [Pcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase

Laszlo was trying to keep things sorted, so you should move this down to
the end of the list.

>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
> index 7f78dcee2772..82910dcbd5c2 100644
> --- a/OvmfPkg/Sec/SecMain.inf
> +++ b/OvmfPkg/Sec/SecMain.inf
> @@ -56,6 +56,7 @@ [Ppis]
>    gEfiTemporaryRamSupportPpiGuid                # PPI ALWAYS_PRODUCED
>  
>  [Pcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase

Ditto here, even though the list isn't truly sorted to begin with.

>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 9db67e17b2aa..dda572c7ad7d 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -807,6 +807,29 @@ SevEsProtocolCheck (
>    Ghcb->GhcbUsage = GHCB_STANDARD_USAGE;
>  }
>  
> +/**
> + Determine if the SEV is active.
> +
> + During the early booting, GuestType is set in the work area. Verify that it
> + is an SEV guest.
> +
> + @retval TRUE   SEV is enabled
> + @retval FALSE  SEV is not enabled
> +
> +**/
> +STATIC
> +BOOLEAN
> +IsSevGuest (
> +  VOID
> +  )
> +{
> +  OVMF_WORK_AREA             *WorkArea;
> +
> +  WorkArea = (OVMF_WORK_AREA *) FixedPcdGet32 (PcdOvmfWorkAreaBase);
> +
> +  return ((WorkArea != NULL) && (WorkArea->GuestType == GUEST_TYPE_AMD_SEV));
> +}
> +
>  /**
>    Determine if SEV-ES is active.
>  
> @@ -828,7 +851,7 @@ SevEsIsEnabled (
>  
>    SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
>  
> -  return ((SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0));
> +  return (((IsSevGuest()) && SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0));

The IsSevGuest() function checks for a NULL work area, so there's no need
to check for SevEsWorkArea being non-NULL now. I think it would read
better, though, to do:

    if (!IsSevGuest ()) {
      return FALSE;
    }

    SevEsWorkArea = ...

    return (SevEsWorkArea->SevEsEnabled != 0);

>  }
>  
>  VOID
> diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> index aa95d06eaddb..87d81b01e263 100644
> --- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> +++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> @@ -171,6 +171,9 @@ CheckSevFeatures:
>      bt        eax, 0
>      jnc       NoSev
>  
> +    ; Set the work area header to indicate that the SEV is enabled

s/the SEV/SEV/

> +    mov     byte[WORK_AREA_GUEST_TYPE], 1

The "1" should probably be defined in ResetVector.nasmb as a %define.

> +
>      ; Check for SEV-ES memory encryption feature:
>      ; CPUID  Fn8000_001F[EAX] - Bit 3
>      ;   CPUID raises a #VC exception if running as an SEV-ES guest
> @@ -257,6 +260,11 @@ SevExit:
>  IsSevEsEnabled:
>      xor       eax, eax
>  
> +    ; During CheckSevFeatures, the WORK_AREA_GUEST_TYPE is set
> +    ; to 1 if SEV is enabled.
> +    cmp       byte[WORK_AREA_GUEST_TYPE], 1
> +    jne       SevEsDisabled
> +
>      ; During CheckSevFeatures, the SEV_ES_WORK_AREA was set to 1 if
>      ; SEV-ES is enabled.
>      cmp       byte[SEV_ES_WORK_AREA], 1
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index eacdb69ddb9f..f688909f1c7d 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -42,6 +42,10 @@ BITS    32
>  ;
>  SetCr3ForPageTables64:
>  
> +    ; Clear the WorkArea header. The SEV probe routines will populate the

How about:
    ; Initialize the WorkArea header to indicate a legacy guest. The ...

> +    ; work area when detected.
> +    mov     byte[WORK_AREA_GUEST_TYPE], 0

And then use a %define here for the '0'

> +
>      OneTimeCall   CheckSevFeatures
>      xor     edx, edx
>      test    eax, eax
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
> index acec46a32450..d1d800c56745 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -72,6 +72,7 @@
>    %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>    %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>    %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
> +  %define WORK_AREA_GUEST_TYPE (FixedPcdGet32 (PcdOvmfWorkAreaBase))

Create %defines for each of the defined enum types from the first patch.

Thanks,
Tom

>    %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
>    %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
>    %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
> 

Re: [PATCH 2/3] OvmfPkg/ResetVector: update SEV support to use new work area format

[Brijesh Singh]

On 8/9/21 11:54 AM, Tom Lendacky wrote:
> On 8/4/21 3:20 PM, Brijesh Singh wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
>>
>> Update the SEV support to switch to using the newer work area format.
>>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   OvmfPkg/ResetVector/ResetVector.inf       |  1 +
>>   OvmfPkg/Sec/SecMain.inf                   |  1 +
>>   OvmfPkg/Sec/SecMain.c                     | 25 ++++++++++++++++++++++-
>>   OvmfPkg/ResetVector/Ia32/AmdSev.asm       |  8 ++++++++
>>   OvmfPkg/ResetVector/Ia32/PageTables64.asm |  4 ++++
>>   OvmfPkg/ResetVector/ResetVector.nasmb     |  1 +
>>   6 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
>> index d028c92d8cfa..6ec9cca40c3a 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>> @@ -34,6 +34,7 @@ [BuildOptions]
>>      *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
>>   
>>   [Pcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
> 
> Laszlo was trying to keep things sorted, so you should move this down to
> the end of the list.

Yes, I will try to keep it sorted.


> 
>>     gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
>> index 7f78dcee2772..82910dcbd5c2 100644
>> --- a/OvmfPkg/Sec/SecMain.inf
>> +++ b/OvmfPkg/Sec/SecMain.inf
>> @@ -56,6 +56,7 @@ [Ppis]
>>     gEfiTemporaryRamSupportPpiGuid                # PPI ALWAYS_PRODUCED
>>   
>>   [Pcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
> 
> Ditto here, even though the list isn't truly sorted to begin with.

Noted.

> 
>>     gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
>> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
>> index 9db67e17b2aa..dda572c7ad7d 100644
>> --- a/OvmfPkg/Sec/SecMain.c
>> +++ b/OvmfPkg/Sec/SecMain.c
>> @@ -807,6 +807,29 @@ SevEsProtocolCheck (
>>     Ghcb->GhcbUsage = GHCB_STANDARD_USAGE;
>>   }
>>   
>> +/**
>> + Determine if the SEV is active.
>> +
>> + During the early booting, GuestType is set in the work area. Verify that it
>> + is an SEV guest.
>> +
>> + @retval TRUE   SEV is enabled
>> + @retval FALSE  SEV is not enabled
>> +
>> +**/
>> +STATIC
>> +BOOLEAN
>> +IsSevGuest (
>> +  VOID
>> +  )
>> +{
>> +  OVMF_WORK_AREA             *WorkArea;
>> +
>> +  WorkArea = (OVMF_WORK_AREA *) FixedPcdGet32 (PcdOvmfWorkAreaBase);
>> +
>> +  return ((WorkArea != NULL) && (WorkArea->GuestType == GUEST_TYPE_AMD_SEV));
>> +}
>> +
>>   /**
>>     Determine if SEV-ES is active.
>>   
>> @@ -828,7 +851,7 @@ SevEsIsEnabled (
>>   
>>     SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
>>   
>> -  return ((SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0));
>> +  return (((IsSevGuest()) && SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0));
> 
> The IsSevGuest() function checks for a NULL work area, so there's no need
> to check for SevEsWorkArea being non-NULL now. I think it would read
> better, though, to do:
> 
>      if (!IsSevGuest ()) {
>        return FALSE;
>      }
> 
>      SevEsWorkArea = ...
> 
>      return (SevEsWorkArea->SevEsEnabled != 0);
> 
>>   }
>>   

Sure, it makes it a bit more readiable and avoids unessary checks.

>>   VOID
>> diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>> index aa95d06eaddb..87d81b01e263 100644
>> --- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>> @@ -171,6 +171,9 @@ CheckSevFeatures:
>>       bt        eax, 0
>>       jnc       NoSev
>>   
>> +    ; Set the work area header to indicate that the SEV is enabled
> 
> s/the SEV/SEV/

Noted.
> 
>> +    mov     byte[WORK_AREA_GUEST_TYPE], 1
> 
> The "1" should probably be defined in ResetVector.nasmb as a %define.

Sure, I will define the constant

> 
>> +
>>       ; Check for SEV-ES memory encryption feature:
>>       ; CPUID  Fn8000_001F[EAX] - Bit 3
>>       ;   CPUID raises a #VC exception if running as an SEV-ES guest
>> @@ -257,6 +260,11 @@ SevExit:
>>   IsSevEsEnabled:
>>       xor       eax, eax
>>   
>> +    ; During CheckSevFeatures, the WORK_AREA_GUEST_TYPE is set
>> +    ; to 1 if SEV is enabled.
>> +    cmp       byte[WORK_AREA_GUEST_TYPE], 1
>> +    jne       SevEsDisabled
>> +
>>       ; During CheckSevFeatures, the SEV_ES_WORK_AREA was set to 1 if
>>       ; SEV-ES is enabled.
>>       cmp       byte[SEV_ES_WORK_AREA], 1
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index eacdb69ddb9f..f688909f1c7d 100644
>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> @@ -42,6 +42,10 @@ BITS    32
>>   ;
>>   SetCr3ForPageTables64:
>>   
>> +    ; Clear the WorkArea header. The SEV probe routines will populate the
> 
> How about:
>      ; Initialize the WorkArea header to indicate a legacy guest. The ...
> 
>> +    ; work area when detected.
>> +    mov     byte[WORK_AREA_GUEST_TYPE], 0
> 
> And then use a %define here for the '0'
> 

I will make the required changes.

>> +
>>       OneTimeCall   CheckSevFeatures
>>       xor     edx, edx
>>       test    eax, eax
>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>> index acec46a32450..d1d800c56745 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>> @@ -72,6 +72,7 @@
>>     %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>>     %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>>     %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>> +  %define WORK_AREA_GUEST_TYPE (FixedPcdGet32 (PcdOvmfWorkAreaBase))
> 
> Create %defines for each of the defined enum types from the first patch.
> 

Got it. thanks

[PATCH 3/3] OvmfPkg/ResetVector: move the GHCB page setup in AmdSev.asm

[Brijesh Singh]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

While build the initial page table, the SetCr3ForPageTables64 checks
whether SEV-ES is enabled. If so, clear the page encryption mask from the
GHCB page. Move the logic to clear the page encryption mask in the
AmdSev.asm.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/ResetVector/Ia32/AmdSev.asm       | 113 +++++++++++++++++-----
 OvmfPkg/ResetVector/Ia32/PageTables64.asm |  53 ++--------
 2 files changed, 94 insertions(+), 72 deletions(-)

diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
index 87d81b01e263..fd2e6abcd4a0 100644
--- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
+++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
@@ -44,6 +44,27 @@ BITS    32
 ; The unexpected response code
 %define TERM_UNEXPECTED_RESP_CODE   2
 
+%define PAGE_PRESENT            0x01
+%define PAGE_READ_WRITE         0x02
+%define PAGE_USER_SUPERVISOR    0x04
+%define PAGE_WRITE_THROUGH      0x08
+%define PAGE_CACHE_DISABLE     0x010
+%define PAGE_ACCESSED          0x020
+%define PAGE_DIRTY             0x040
+%define PAGE_PAT               0x080
+%define PAGE_GLOBAL           0x0100
+%define PAGE_2M_MBO            0x080
+%define PAGE_2M_PAT          0x01000
+
+%define PAGE_4K_PDE_ATTR (PAGE_ACCESSED + \
+                          PAGE_DIRTY + \
+                          PAGE_READ_WRITE + \
+                          PAGE_PRESENT)
+
+%define PAGE_PDP_ATTR (PAGE_ACCESSED + \
+                       PAGE_READ_WRITE + \
+                       PAGE_PRESENT)
+
 
 ; Macro is used to issue the MSR protocol based VMGEXIT. The caller is
 ; responsible to populate values in the EDX:EAX registers. After the vmmcall
@@ -117,6 +138,72 @@ BITS    32
 SevEsUnexpectedRespTerminate:
     TerminateVmgExit    TERM_UNEXPECTED_RESP_CODE
 
+; If SEV-ES is enabled then initialize the make the GHCB page shared
+SevClearPageEncMaskFromGHCBPage:
+    ; Check if SEV is enabled
+    cmp       byte[WORK_AREA_GUEST_TYPE], 1
+    jnz       SevClearPageEncMaskFromGHCBPageExit
+
+    ; Check if SEV-ES is enabled
+    cmp       byte[SEV_ES_WORK_AREA], 1
+    jnz       SevClearPageEncMaskFromGHCBPageExit
+
+    ;
+    ; The initial GHCB will live at GHCB_BASE and needs to be un-encrypted.
+    ; This requires the 2MB page for this range be broken down into 512 4KB
+    ; pages.  All will be marked encrypted, except for the GHCB.
+    ;
+    mov     ecx, (GHCB_BASE >> 21)
+    mov     eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
+    mov     [ecx * 8 + PT_ADDR (0x2000)], eax
+
+    ;
+    ; Page Table Entries (512 * 4KB entries => 2MB)
+    ;
+    mov     ecx, 512
+pageTableEntries4kLoop:
+    mov     eax, ecx
+    dec     eax
+    shl     eax, 12
+    add     eax, GHCB_BASE & 0xFFE0_0000
+    add     eax, PAGE_4K_PDE_ATTR
+    mov     [ecx * 8 + GHCB_PT_ADDR - 8], eax
+    mov     [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
+    loop    pageTableEntries4kLoop
+
+    ;
+    ; Clear the encryption bit from the GHCB entry
+    ;
+    mov     ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
+    mov     [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
+
+    mov     ecx, GHCB_SIZE / 4
+    xor     eax, eax
+clearGhcbMemoryLoop:
+    mov     dword[ecx * 4 + GHCB_BASE - 4], eax
+    loop    clearGhcbMemoryLoop
+
+SevClearPageEncMaskFromGHCBPageExit:
+    OneTimeCallRet SevClearPageEncMaskFromGHCBPage
+
+; Check if SEV is enabled, and get the C-bit mask above 31.
+; Modified: EDX
+;
+; The value is returned in the EDX
+GetSevCBitMaskAbove31:
+    ; Check if SEV is enabled
+    cmp       byte[WORK_AREA_GUEST_TYPE], 1
+    jnz       NoCbitValue
+
+    mov       edx, dword[SEV_ES_WORK_AREA_ENC_MASK + 4]
+    jmp       GetSevCBitMaskAbove31Exit
+
+NoCbitValue:
+    xor       edx, edx
+
+GetSevCBitMaskAbove31Exit:
+    OneTimeCallRet GetSevCBitMaskAbove31
+
 ; Check if Secure Encrypted Virtualization (SEV) features are enabled.
 ;
 ; Register usage is tight in this routine, so multiple calls for the
@@ -249,32 +336,6 @@ SevExit:
 
     OneTimeCallRet CheckSevFeatures
 
-; Check if Secure Encrypted Virtualization - Encrypted State (SEV-ES) feature
-; is enabled.
-;
-; Modified:  EAX
-;
-; If SEV-ES is enabled then EAX will be non-zero.
-; If SEV-ES is disabled then EAX will be zero.
-;
-IsSevEsEnabled:
-    xor       eax, eax
-
-    ; During CheckSevFeatures, the WORK_AREA_GUEST_TYPE is set
-    ; to 1 if SEV is enabled.
-    cmp       byte[WORK_AREA_GUEST_TYPE], 1
-    jne       SevEsDisabled
-
-    ; During CheckSevFeatures, the SEV_ES_WORK_AREA was set to 1 if
-    ; SEV-ES is enabled.
-    cmp       byte[SEV_ES_WORK_AREA], 1
-    jne       SevEsDisabled
-
-    mov       eax, 1
-
-SevEsDisabled:
-    OneTimeCallRet IsSevEsEnabled
-
 ; Start of #VC exception handling routines
 ;
 
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index f688909f1c7d..0e8ba4dde534 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -46,16 +46,13 @@ SetCr3ForPageTables64:
     ; work area when detected.
     mov     byte[WORK_AREA_GUEST_TYPE], 0
 
+    ; Check whether the SEV is active and populate the SevEsWorkArea
     OneTimeCall   CheckSevFeatures
-    xor     edx, edx
-    test    eax, eax
-    jz      SevNotActive
 
-    ; If SEV is enabled, C-bit is always above 31
-    sub     eax, 32
-    bts     edx, eax
-
-SevNotActive:
+    ; If SEV is enabled, the C-bit position is always above 31.
+    ; The mask will be saved in the EDX and applied during the
+    ; the page table build below.
+    OneTimeCall   GetSevCBitMaskAbove31
 
     ;
     ; For OVMF, build some initial page tables at
@@ -105,44 +102,8 @@ pageTableEntriesLoop:
     mov     [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
     loop    pageTableEntriesLoop
 
-    OneTimeCall   IsSevEsEnabled
-    test    eax, eax
-    jz      SetCr3
-
-    ;
-    ; The initial GHCB will live at GHCB_BASE and needs to be un-encrypted.
-    ; This requires the 2MB page for this range be broken down into 512 4KB
-    ; pages.  All will be marked encrypted, except for the GHCB.
-    ;
-    mov     ecx, (GHCB_BASE >> 21)
-    mov     eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
-    mov     [ecx * 8 + PT_ADDR (0x2000)], eax
-
-    ;
-    ; Page Table Entries (512 * 4KB entries => 2MB)
-    ;
-    mov     ecx, 512
-pageTableEntries4kLoop:
-    mov     eax, ecx
-    dec     eax
-    shl     eax, 12
-    add     eax, GHCB_BASE & 0xFFE0_0000
-    add     eax, PAGE_4K_PDE_ATTR
-    mov     [ecx * 8 + GHCB_PT_ADDR - 8], eax
-    mov     [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
-    loop    pageTableEntries4kLoop
-
-    ;
-    ; Clear the encryption bit from the GHCB entry
-    ;
-    mov     ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
-    mov     [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
-
-    mov     ecx, GHCB_SIZE / 4
-    xor     eax, eax
-clearGhcbMemoryLoop:
-    mov     dword[ecx * 4 + GHCB_BASE - 4], eax
-    loop    clearGhcbMemoryLoop
+    ; Clear the C-bit from the GHCB page if the SEV-ES is enabled.
+    OneTimeCall   SevClearPageEncMaskFromGHCBPage
 
 SetCr3:
     ;
-- 
2.17.1

Re: [PATCH 3/3] OvmfPkg/ResetVector: move the GHCB page setup in AmdSev.asm

[Lendacky, Thomas]

On 8/4/21 3:20 PM, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
> 
> While build the initial page table, the SetCr3ForPageTables64 checks
> whether SEV-ES is enabled. If so, clear the page encryption mask from the
> GHCB page. Move the logic to clear the page encryption mask in the
> AmdSev.asm.
> 
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/ResetVector/Ia32/AmdSev.asm       | 113 +++++++++++++++++-----
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm |  53 ++--------
>  2 files changed, 94 insertions(+), 72 deletions(-)
> 
> diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> index 87d81b01e263..fd2e6abcd4a0 100644
> --- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> +++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> @@ -44,6 +44,27 @@ BITS    32
>  ; The unexpected response code
>  %define TERM_UNEXPECTED_RESP_CODE   2
>  
> +%define PAGE_PRESENT            0x01
> +%define PAGE_READ_WRITE         0x02
> +%define PAGE_USER_SUPERVISOR    0x04
> +%define PAGE_WRITE_THROUGH      0x08
> +%define PAGE_CACHE_DISABLE     0x010
> +%define PAGE_ACCESSED          0x020
> +%define PAGE_DIRTY             0x040
> +%define PAGE_PAT               0x080
> +%define PAGE_GLOBAL           0x0100
> +%define PAGE_2M_MBO            0x080
> +%define PAGE_2M_PAT          0x01000
> +
> +%define PAGE_4K_PDE_ATTR (PAGE_ACCESSED + \
> +                          PAGE_DIRTY + \
> +                          PAGE_READ_WRITE + \
> +                          PAGE_PRESENT)
> +
> +%define PAGE_PDP_ATTR (PAGE_ACCESSED + \
> +                       PAGE_READ_WRITE + \
> +                       PAGE_PRESENT)
> +
>  
>  ; Macro is used to issue the MSR protocol based VMGEXIT. The caller is
>  ; responsible to populate values in the EDX:EAX registers. After the vmmcall
> @@ -117,6 +138,72 @@ BITS    32
>  SevEsUnexpectedRespTerminate:
>      TerminateVmgExit    TERM_UNEXPECTED_RESP_CODE
>  
> +; If SEV-ES is enabled then initialize the make the GHCB page shared

s/the make/and make/ ?

> +SevClearPageEncMaskFromGHCBPage:

Just a nit, maybe SevClearPageEncMaskForGhcbPage?

> +    ; Check if SEV is enabled
> +    cmp       byte[WORK_AREA_GUEST_TYPE], 1
> +    jnz       SevClearPageEncMaskFromGHCBPageExit
> +
> +    ; Check if SEV-ES is enabled
> +    cmp       byte[SEV_ES_WORK_AREA], 1
> +    jnz       SevClearPageEncMaskFromGHCBPageExit
> +
> +    ;
> +    ; The initial GHCB will live at GHCB_BASE and needs to be un-encrypted.
> +    ; This requires the 2MB page for this range be broken down into 512 4KB
> +    ; pages.  All will be marked encrypted, except for the GHCB.
> +    ;
> +    mov     ecx, (GHCB_BASE >> 21)
> +    mov     eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
> +    mov     [ecx * 8 + PT_ADDR (0x2000)], eax
> +
> +    ;
> +    ; Page Table Entries (512 * 4KB entries => 2MB)
> +    ;
> +    mov     ecx, 512
> +pageTableEntries4kLoop:
> +    mov     eax, ecx
> +    dec     eax
> +    shl     eax, 12
> +    add     eax, GHCB_BASE & 0xFFE0_0000
> +    add     eax, PAGE_4K_PDE_ATTR
> +    mov     [ecx * 8 + GHCB_PT_ADDR - 8], eax
> +    mov     [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
> +    loop    pageTableEntries4kLoop
> +
> +    ;
> +    ; Clear the encryption bit from the GHCB entry
> +    ;
> +    mov     ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
> +    mov     [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
> +
> +    mov     ecx, GHCB_SIZE / 4
> +    xor     eax, eax
> +clearGhcbMemoryLoop:
> +    mov     dword[ecx * 4 + GHCB_BASE - 4], eax
> +    loop    clearGhcbMemoryLoop
> +
> +SevClearPageEncMaskFromGHCBPageExit:
> +    OneTimeCallRet SevClearPageEncMaskFromGHCBPage
> +
> +; Check if SEV is enabled, and get the C-bit mask above 31.
> +; Modified: EDX
> +;
> +; The value is returned in the EDX
> +GetSevCBitMaskAbove31:
> +    ; Check if SEV is enabled
> +    cmp       byte[WORK_AREA_GUEST_TYPE], 1
> +    jnz       NoCbitValue
> +
> +    mov       edx, dword[SEV_ES_WORK_AREA_ENC_MASK + 4]
> +    jmp       GetSevCBitMaskAbove31Exit
> +
> +NoCbitValue:
> +    xor       edx, edx

How about moving the xor as the first line of this routine and jumping to
GetSevCBitMaskAbove31Exit if the first cmp is non-zero. Then you can just
do the move from SEV_ES_WORK_AREA_ENC_MASK + 4 and eliminate the extra jmp
statement and NoCbitValue label.

Thanks,
Tom

> +
> +GetSevCBitMaskAbove31Exit:
> +    OneTimeCallRet GetSevCBitMaskAbove31
> +
>  ; Check if Secure Encrypted Virtualization (SEV) features are enabled.
>  ;
>  ; Register usage is tight in this routine, so multiple calls for the
> @@ -249,32 +336,6 @@ SevExit:
>  
>      OneTimeCallRet CheckSevFeatures
>  
> -; Check if Secure Encrypted Virtualization - Encrypted State (SEV-ES) feature
> -; is enabled.
> -;
> -; Modified:  EAX
> -;
> -; If SEV-ES is enabled then EAX will be non-zero.
> -; If SEV-ES is disabled then EAX will be zero.
> -;
> -IsSevEsEnabled:
> -    xor       eax, eax
> -
> -    ; During CheckSevFeatures, the WORK_AREA_GUEST_TYPE is set
> -    ; to 1 if SEV is enabled.
> -    cmp       byte[WORK_AREA_GUEST_TYPE], 1
> -    jne       SevEsDisabled
> -
> -    ; During CheckSevFeatures, the SEV_ES_WORK_AREA was set to 1 if
> -    ; SEV-ES is enabled.
> -    cmp       byte[SEV_ES_WORK_AREA], 1
> -    jne       SevEsDisabled
> -
> -    mov       eax, 1
> -
> -SevEsDisabled:
> -    OneTimeCallRet IsSevEsEnabled
> -
>  ; Start of #VC exception handling routines
>  ;
>  
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index f688909f1c7d..0e8ba4dde534 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -46,16 +46,13 @@ SetCr3ForPageTables64:
>      ; work area when detected.
>      mov     byte[WORK_AREA_GUEST_TYPE], 0
>  
> +    ; Check whether the SEV is active and populate the SevEsWorkArea
>      OneTimeCall   CheckSevFeatures
> -    xor     edx, edx
> -    test    eax, eax
> -    jz      SevNotActive
>  
> -    ; If SEV is enabled, C-bit is always above 31
> -    sub     eax, 32
> -    bts     edx, eax
> -
> -SevNotActive:
> +    ; If SEV is enabled, the C-bit position is always above 31.
> +    ; The mask will be saved in the EDX and applied during the
> +    ; the page table build below.
> +    OneTimeCall   GetSevCBitMaskAbove31
>  
>      ;
>      ; For OVMF, build some initial page tables at
> @@ -105,44 +102,8 @@ pageTableEntriesLoop:
>      mov     [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
>      loop    pageTableEntriesLoop
>  
> -    OneTimeCall   IsSevEsEnabled
> -    test    eax, eax
> -    jz      SetCr3
> -
> -    ;
> -    ; The initial GHCB will live at GHCB_BASE and needs to be un-encrypted.
> -    ; This requires the 2MB page for this range be broken down into 512 4KB
> -    ; pages.  All will be marked encrypted, except for the GHCB.
> -    ;
> -    mov     ecx, (GHCB_BASE >> 21)
> -    mov     eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
> -    mov     [ecx * 8 + PT_ADDR (0x2000)], eax
> -
> -    ;
> -    ; Page Table Entries (512 * 4KB entries => 2MB)
> -    ;
> -    mov     ecx, 512
> -pageTableEntries4kLoop:
> -    mov     eax, ecx
> -    dec     eax
> -    shl     eax, 12
> -    add     eax, GHCB_BASE & 0xFFE0_0000
> -    add     eax, PAGE_4K_PDE_ATTR
> -    mov     [ecx * 8 + GHCB_PT_ADDR - 8], eax
> -    mov     [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
> -    loop    pageTableEntries4kLoop
> -
> -    ;
> -    ; Clear the encryption bit from the GHCB entry
> -    ;
> -    mov     ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
> -    mov     [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
> -
> -    mov     ecx, GHCB_SIZE / 4
> -    xor     eax, eax
> -clearGhcbMemoryLoop:
> -    mov     dword[ecx * 4 + GHCB_BASE - 4], eax
> -    loop    clearGhcbMemoryLoop
> +    ; Clear the C-bit from the GHCB page if the SEV-ES is enabled.
> +    OneTimeCall   SevClearPageEncMaskFromGHCBPage
>  
>  SetCr3:
>      ;
> 

Re: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area

[Yao, Jiewen]

HI Brijesh
Thanks for the startup. Feedback below:

1) I don't think we need a PCD to indicate the header.
  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaHeaderSize|4|UINT32|0x51

Instead, if we define a HEADER structure, we can use sizeof() naturally. Otherwise, when we update this header, we need update 2 different places, which is not preferred.

typedef struct {
  UINT8                   GuestType;
  UINT8                   Reserved1[3];
} CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;

2) I don't think we can define a common structure OVMF_WORK_AREA to contain SEV specific field.

typedef struct _OVMF_WORK_AREA {
  UINT8                   GuestType;
  UINT8                   Reserved1[3];

  SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
} OVMF_WORK_AREA;

A common patter is to define each individual structure, then use UNION.

For example, 

typedef struct {
  UINT8                   GuestType;
  UINT8                   Reserved1[3];

  SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
} SEV_WORK_AREA;

typedef union {
  CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER Header;
  SEV_WORK_AREA  Sev;
} OVMF_WORK_AREA;



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Thursday, August 5, 2021 4:20 AM
> To: devel@edk2.groups.io
> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Brijesh
> Singh <brijesh.singh@amd.com>
> Subject: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area
> 
> Based on the discussion on the mailing list, we agreed that instead
> of wasting extra page in the MEMFD, we can reuse the SevEsWorkArea
> buffer for the TDX. To avoid any confusion, lets introduce a OvmfWorkArea
> that will contains 32 bytes of header followed by the actual workarea.
> 
> While at it, move the code to clear the GHCB page from PageTable build
> to AmdSev.asm.
> 
> I have used the existing TDX BZ for it because the request came
> during the TDX patch review. if anyone have concern please let me know
> and I will happily create a new BZ.
> 
> Full tree is at: https://github.com/AMDESE/ovmf/tree/sev-new-work-area
> 
> Brijesh Singh (3):
>   OvmfPkg: introduce a common work area
>   OvmfPkg/ResetVector: update SEV support to use new work area format
>   OvmfPkg/ResetVector: move the GHCB page setup in AmdSev.asm
> 
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Erdem Aktas <erdemaktas@google.com>
> 
>  OvmfPkg/OvmfPkg.dec                        |   6 ++
>  OvmfPkg/OvmfPkgX64.fdf                     |   9 +-
>  OvmfPkg/PlatformPei/PlatformPei.inf        |   4 +-
>  OvmfPkg/ResetVector/ResetVector.inf        |   1 +
>  OvmfPkg/Sec/SecMain.inf                    |   1 +
>  OvmfPkg/Include/Library/MemEncryptSevLib.h |  21 +---
>  OvmfPkg/Include/WorkArea.h                 |  53 ++++++++++
>  OvmfPkg/PlatformPei/MemDetect.c            |  32 +++---
>  OvmfPkg/Sec/SecMain.c                      |  25 ++++-
>  OvmfPkg/ResetVector/Ia32/AmdSev.asm        | 111 +++++++++++++++++----
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  |  57 ++---------
>  OvmfPkg/ResetVector/ResetVector.nasmb      |   1 +
>  12 files changed, 213 insertions(+), 108 deletions(-)
>  create mode 100644 OvmfPkg/Include/WorkArea.h
> 
> --
> 2.17.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area

[Brijesh Singh]

Hi Jiewen,

Thanks for the quick feedback. I will make the recommended change and
send the updated patch. I was under assumption that union will be done
when Min adds the SGX support because that's when we start reusing the
WorkArea for SEV and TDX. But I guess its good idea for me to do it now
so that Min does not have to touch the SEV code in his series.

thanks

On 8/4/21 9:18 PM, Yao, Jiewen wrote:
> HI Brijesh
> Thanks for the startup. Feedback below:
>
> 1) I don't think we need a PCD to indicate the header.
>   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaHeaderSize|4|UINT32|0x51
>
> Instead, if we define a HEADER structure, we can use sizeof() naturally. Otherwise, when we update this header, we need update 2 different places, which is not preferred.
>
> typedef struct {
>   UINT8                   GuestType;
>   UINT8                   Reserved1[3];
> } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
>
> 2) I don't think we can define a common structure OVMF_WORK_AREA to contain SEV specific field.
>
> typedef struct _OVMF_WORK_AREA {
>   UINT8                   GuestType;
>   UINT8                   Reserved1[3];
>
>   SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
> } OVMF_WORK_AREA;
>
> A common patter is to define each individual structure, then use UNION.
>
> For example, 
>
> typedef struct {
>   UINT8                   GuestType;
>   UINT8                   Reserved1[3];
>
>   SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
> } SEV_WORK_AREA;
>
> typedef union {
>   CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER Header;
>   SEV_WORK_AREA  Sev;
> } OVMF_WORK_AREA;
>
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
>> Singh via groups.io
>> Sent: Thursday, August 5, 2021 4:20 AM
>> To: devel@edk2.groups.io
>> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>;
>> Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
>> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
>> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
>> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Brijesh
>> Singh <brijesh.singh@amd.com>
>> Subject: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area
>>
>> Based on the discussion on the mailing list, we agreed that instead
>> of wasting extra page in the MEMFD, we can reuse the SevEsWorkArea
>> buffer for the TDX. To avoid any confusion, lets introduce a OvmfWorkArea
>> that will contains 32 bytes of header followed by the actual workarea.
>>
>> While at it, move the code to clear the GHCB page from PageTable build
>> to AmdSev.asm.
>>
>> I have used the existing TDX BZ for it because the request came
>> during the TDX patch review. if anyone have concern please let me know
>> and I will happily create a new BZ.
>>
>> Full tree is at: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Ftree%2Fsev-new-work-area&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C4c55a642f1804a803c4e08d957b75e61%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637637267367225365%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NSsUVfQodJMDUcpLCsHSpTaRDHM8et%2BWZJOS8lCS3Kw%3D&amp;reserved=0
>>
>> Brijesh Singh (3):
>>   OvmfPkg: introduce a common work area
>>   OvmfPkg/ResetVector: update SEV support to use new work area format
>>   OvmfPkg/ResetVector: move the GHCB page setup in AmdSev.asm
>>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>>
>>  OvmfPkg/OvmfPkg.dec                        |   6 ++
>>  OvmfPkg/OvmfPkgX64.fdf                     |   9 +-
>>  OvmfPkg/PlatformPei/PlatformPei.inf        |   4 +-
>>  OvmfPkg/ResetVector/ResetVector.inf        |   1 +
>>  OvmfPkg/Sec/SecMain.inf                    |   1 +
>>  OvmfPkg/Include/Library/MemEncryptSevLib.h |  21 +---
>>  OvmfPkg/Include/WorkArea.h                 |  53 ++++++++++
>>  OvmfPkg/PlatformPei/MemDetect.c            |  32 +++---
>>  OvmfPkg/Sec/SecMain.c                      |  25 ++++-
>>  OvmfPkg/ResetVector/Ia32/AmdSev.asm        | 111 +++++++++++++++++----
>>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  |  57 ++---------
>>  OvmfPkg/ResetVector/ResetVector.nasmb      |   1 +
>>  12 files changed, 213 insertions(+), 108 deletions(-)
>>  create mode 100644 OvmfPkg/Include/WorkArea.h
>>
>> --
>> 2.17.1
>>
>>
>>
>> 
>>

Re: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area

[Brijesh Singh]

Hi Jiewen,

On 8/4/21 9:18 PM, Yao, Jiewen wrote:
> HI Brijesh
> Thanks for the startup. Feedback below:
> 
> 1) I don't think we need a PCD to indicate the header.
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaHeaderSize|4|UINT32|0x51
> 
> Instead, if we define a HEADER structure, we can use sizeof() naturally. Otherwise, when we update this header, we need update 2 different places, which is not preferred.
> 

Can you use the sizeof() inside the OvmfPkg.fdf ? I was not able to find 
a reference usage for it in this file. Also, can a .fdf refer the C 
header file ?

We need to know the size of the header so that we can set the fixed 
PcdOvmfSevEsWorkArea. In the current approach the .fdf sets the PCD as 
shown below:

SET PcdOvmfSevEsWorkArea = PcdOvmfWorkAreaBase + <HeaderSize>

I am hard coding the header size to be 4 so that pcd points to the 
correct location within the WorkArea.

thanks


> typedef struct {
>    UINT8                   GuestType;
>    UINT8                   Reserved1[3];
> } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> 
> 2) I don't think we can define a common structure OVMF_WORK_AREA to contain SEV specific field.
> 
> typedef struct _OVMF_WORK_AREA {
>    UINT8                   GuestType;
>    UINT8                   Reserved1[3];
> 
>    SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
> } OVMF_WORK_AREA;
> 
> A common patter is to define each individual structure, then use UNION.
> 
> For example,
> 
> typedef struct {
>    UINT8                   GuestType;
>    UINT8                   Reserved1[3];
> 
>    SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
> } SEV_WORK_AREA;
> 
> typedef union {
>    CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER Header;
>    SEV_WORK_AREA  Sev;
> } OVMF_WORK_AREA;
> 
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
>> Singh via groups.io
>> Sent: Thursday, August 5, 2021 4:20 AM
>> To: devel@edk2.groups.io
>> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>;
>> Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
>> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
>> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
>> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Brijesh
>> Singh <brijesh.singh@amd.com>
>> Subject: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area
>>
>> Based on the discussion on the mailing list, we agreed that instead
>> of wasting extra page in the MEMFD, we can reuse the SevEsWorkArea
>> buffer for the TDX. To avoid any confusion, lets introduce a OvmfWorkArea
>> that will contains 32 bytes of header followed by the actual workarea.
>>
>> While at it, move the code to clear the GHCB page from PageTable build
>> to AmdSev.asm.
>>
>> I have used the existing TDX BZ for it because the request came
>> during the TDX patch review. if anyone have concern please let me know
>> and I will happily create a new BZ.
>>
>> Full tree is at: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Ftree%2Fsev-new-work-area&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C4c55a642f1804a803c4e08d957b75e61%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637637267367225365%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NSsUVfQodJMDUcpLCsHSpTaRDHM8et%2BWZJOS8lCS3Kw%3D&amp;reserved=0
>>
>> Brijesh Singh (3):
>>    OvmfPkg: introduce a common work area
>>    OvmfPkg/ResetVector: update SEV support to use new work area format
>>    OvmfPkg/ResetVector: move the GHCB page setup in AmdSev.asm
>>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>>
>>   OvmfPkg/OvmfPkg.dec                        |   6 ++
>>   OvmfPkg/OvmfPkgX64.fdf                     |   9 +-
>>   OvmfPkg/PlatformPei/PlatformPei.inf        |   4 +-
>>   OvmfPkg/ResetVector/ResetVector.inf        |   1 +
>>   OvmfPkg/Sec/SecMain.inf                    |   1 +
>>   OvmfPkg/Include/Library/MemEncryptSevLib.h |  21 +---
>>   OvmfPkg/Include/WorkArea.h                 |  53 ++++++++++
>>   OvmfPkg/PlatformPei/MemDetect.c            |  32 +++---
>>   OvmfPkg/Sec/SecMain.c                      |  25 ++++-
>>   OvmfPkg/ResetVector/Ia32/AmdSev.asm        | 111 +++++++++++++++++----
>>   OvmfPkg/ResetVector/Ia32/PageTables64.asm  |  57 ++---------
>>   OvmfPkg/ResetVector/ResetVector.nasmb      |   1 +
>>   12 files changed, 213 insertions(+), 108 deletions(-)
>>   create mode 100644 OvmfPkg/Include/WorkArea.h
>>
>> --
>> 2.17.1
>>
>>
>>
>> 
>>
> 

Re: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area

[Yao, Jiewen]

Ah, sorry, I did not realize that you are using that in FDF.

If we have to define the PcdOvmfWorkAreaHeaderSize, then I would suggest:
1) Add detail comment in the PCD definition - it must be same as sizeof(Header).
2) Add ASSERT in the code to ensure PcdOvmfWorkAreaHeaderSize is same as sizeof(Header).

Just in case someone change only one value, it can be caught in the code.

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Thursday, August 5, 2021 10:44 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: brijesh.singh@amd.com; James Bottomley <jejb@linux.ibm.com>; Xu, Min M
> <min.m.xu@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Erdem Aktas <erdemaktas@google.com>;
> Michael Roth <Michael.Roth@amd.com>
> Subject: Re: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area
> 
> Hi Jiewen,
> 
> On 8/4/21 9:18 PM, Yao, Jiewen wrote:
> > HI Brijesh
> > Thanks for the startup. Feedback below:
> >
> > 1) I don't think we need a PCD to indicate the header.
> >
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaHeaderSize|4|UINT32|0x51
> >
> > Instead, if we define a HEADER structure, we can use sizeof() naturally.
> Otherwise, when we update this header, we need update 2 different places,
> which is not preferred.
> >
> 
> Can you use the sizeof() inside the OvmfPkg.fdf ? I was not able to find
> a reference usage for it in this file. Also, can a .fdf refer the C
> header file ?
> 
> We need to know the size of the header so that we can set the fixed
> PcdOvmfSevEsWorkArea. In the current approach the .fdf sets the PCD as
> shown below:
> 
> SET PcdOvmfSevEsWorkArea = PcdOvmfWorkAreaBase + <HeaderSize>
> 
> I am hard coding the header size to be 4 so that pcd points to the
> correct location within the WorkArea.
> 
> thanks
> 
> 
> > typedef struct {
> >    UINT8                   GuestType;
> >    UINT8                   Reserved1[3];
> > } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> >
> > 2) I don't think we can define a common structure OVMF_WORK_AREA to
> contain SEV specific field.
> >
> > typedef struct _OVMF_WORK_AREA {
> >    UINT8                   GuestType;
> >    UINT8                   Reserved1[3];
> >
> >    SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
> > } OVMF_WORK_AREA;
> >
> > A common patter is to define each individual structure, then use UNION.
> >
> > For example,
> >
> > typedef struct {
> >    UINT8                   GuestType;
> >    UINT8                   Reserved1[3];
> >
> >    SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
> > } SEV_WORK_AREA;
> >
> > typedef union {
> >    CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER Header;
> >    SEV_WORK_AREA  Sev;
> > } OVMF_WORK_AREA;
> >
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> >> Singh via groups.io
> >> Sent: Thursday, August 5, 2021 4:20 AM
> >> To: devel@edk2.groups.io
> >> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M
> <min.m.xu@intel.com>;
> >> Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> >> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> >> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
> >> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>;
> Brijesh
> >> Singh <brijesh.singh@amd.com>
> >> Subject: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area
> >>
> >> Based on the discussion on the mailing list, we agreed that instead
> >> of wasting extra page in the MEMFD, we can reuse the SevEsWorkArea
> >> buffer for the TDX. To avoid any confusion, lets introduce a OvmfWorkArea
> >> that will contains 32 bytes of header followed by the actual workarea.
> >>
> >> While at it, move the code to clear the GHCB page from PageTable build
> >> to AmdSev.asm.
> >>
> >> I have used the existing TDX BZ for it because the request came
> >> during the TDX patch review. if anyone have concern please let me know
> >> and I will happily create a new BZ.
> >>
> >> Full tree is at:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2FAMDESE%2Fovmf%2Ftree%2Fsev-new-work-
> area&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C4c55a642f1804a80
> 3c4e08d957b75e61%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6
> 37637267367225365%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi
> LCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NSs
> UVfQodJMDUcpLCsHSpTaRDHM8et%2BWZJOS8lCS3Kw%3D&amp;reserved=0
> >>
> >> Brijesh Singh (3):
> >>    OvmfPkg: introduce a common work area
> >>    OvmfPkg/ResetVector: update SEV support to use new work area format
> >>    OvmfPkg/ResetVector: move the GHCB page setup in AmdSev.asm
> >>
> >> Cc: James Bottomley <jejb@linux.ibm.com>
> >> Cc: Min Xu <min.m.xu@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> >> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> Cc: Erdem Aktas <erdemaktas@google.com>
> >>
> >>   OvmfPkg/OvmfPkg.dec                        |   6 ++
> >>   OvmfPkg/OvmfPkgX64.fdf                     |   9 +-
> >>   OvmfPkg/PlatformPei/PlatformPei.inf        |   4 +-
> >>   OvmfPkg/ResetVector/ResetVector.inf        |   1 +
> >>   OvmfPkg/Sec/SecMain.inf                    |   1 +
> >>   OvmfPkg/Include/Library/MemEncryptSevLib.h |  21 +---
> >>   OvmfPkg/Include/WorkArea.h                 |  53 ++++++++++
> >>   OvmfPkg/PlatformPei/MemDetect.c            |  32 +++---
> >>   OvmfPkg/Sec/SecMain.c                      |  25 ++++-
> >>   OvmfPkg/ResetVector/Ia32/AmdSev.asm        | 111 +++++++++++++++++----
> >>   OvmfPkg/ResetVector/Ia32/PageTables64.asm  |  57 ++---------
> >>   OvmfPkg/ResetVector/ResetVector.nasmb      |   1 +
> >>   12 files changed, 213 insertions(+), 108 deletions(-)
> >>   create mode 100644 OvmfPkg/Include/WorkArea.h
> >>
> >> --
> >> 2.17.1
> >>
> >>
> >>
> >>
> >>
> >
> 
> 
> 
>