[PATCH 0/3] Enable Secure-Boot in Tdx guest

[PATCH 0/3] Enable Secure-Boot in Tdx guest

[Min Xu]

Secure-Boot related variables include the PK/KEK/DB/DBX and they are
stored in NvVarStore (OVMF_VARS.fd). But QEMU command option -pflash is
not supported in Tdx guest. So when Tdx guest is booted,
EmuVariableFvbRuntimeDxe driver is loaded and the NvVarStore is
initialized with empty content. This patch-set is to initialize the
NvVarStore with the content of Configuration FV (CFV).

Before the NvVarStore is initialized with the content of CFV, CFV's
integrity should be validated. So patch #1/2 are imported to do such
validation.

Code: https://github.com/mxu9/edk2/tree/secure-boot.v1

Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>

*** BLURB HERE ***

Min M Xu (3):
  OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib
  OvmfPkg: Validate Cfv integrity in Tdx guest
  OvmfPkg: Initialize NvVarStore with Configuration FV in Td guest

 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c        |  19 +++
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf      |   2 +
 OvmfPkg/Include/Library/PlatformInitLib.h     |  17 ++
 OvmfPkg/Library/PeilessStartupLib/IntelTdx.c  | 153 ------------------
 .../PeilessStartupInternal.h                  |  17 --
 OvmfPkg/Library/PlatformInitLib/IntelTdx.c    | 153 ++++++++++++++++++
 OvmfPkg/Sec/SecMain.c                         |   8 +
 OvmfPkg/Sec/SecMain.inf                       |   2 +
 8 files changed, 201 insertions(+), 170 deletions(-)

-- 
2.29.2.windows.2

[PATCH 1/3] OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

TdxValidateCfv validates the integrity of Configuration FV (CFV). It was
implemented in PeilessStartupLib which is included in IntelTdxX64.

In OvmfPkgX64 we should validate CFV as well. So it is moved from
PeilessStartupLib to PlatformInitLib so that it can be called in both
OvmfPkgX64 and IntelTdxX64.

Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/Include/Library/PlatformInitLib.h     |  17 ++
 OvmfPkg/Library/PeilessStartupLib/IntelTdx.c  | 153 ------------------
 .../PeilessStartupInternal.h                  |  17 --
 OvmfPkg/Library/PlatformInitLib/IntelTdx.c    | 153 ++++++++++++++++++
 4 files changed, 170 insertions(+), 170 deletions(-)

diff --git a/OvmfPkg/Include/Library/PlatformInitLib.h b/OvmfPkg/Include/Library/PlatformInitLib.h
index 2987a367cc9c..a3acfb1fb196 100644
--- a/OvmfPkg/Include/Library/PlatformInitLib.h
+++ b/OvmfPkg/Include/Library/PlatformInitLib.h
@@ -234,4 +234,21 @@ PlatformTdxPublishRamRegions (
   VOID
   );
 
+/**
+  Check the integrity of CFV data.
+
+  @param[in] TdxCfvBase - A pointer to CFV header
+  @param[in] TdxCfvSize - CFV data size
+
+  @retval  TRUE   - The CFV data is valid.
+  @retval  FALSE  - The CFV data is invalid.
+
+**/
+BOOLEAN
+EFIAPI
+TdxValidateCfv (
+  IN UINT8   *TdxCfvBase,
+  IN UINT32  TdxCfvSize
+  );
+
 #endif // PLATFORM_INIT_LIB_H_
diff --git a/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c b/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c
index 484fd21057c8..216c413caad5 100644
--- a/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c
+++ b/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c
@@ -7,8 +7,6 @@
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
-#include <Guid/VariableFormat.h>
-#include <Guid/SystemNvDataGuid.h>
 #include <IndustryStandard/Tpm20.h>
 #include <IndustryStandard/UefiTcgPlatform.h>
 #include <Library/HobLib.h>
@@ -37,157 +35,6 @@ typedef struct {
 
 #pragma pack()
 
-/**
-  Check padding data all bit should be 1.
-
-  @param[in] Buffer     - A pointer to buffer header
-  @param[in] BufferSize - Buffer size
-
-  @retval  TRUE   - The padding data is valid.
-  @retval  TRUE  - The padding data is invalid.
-
-**/
-BOOLEAN
-CheckPaddingData (
-  IN UINT8   *Buffer,
-  IN UINT32  BufferSize
-  )
-{
-  UINT32  index;
-
-  for (index = 0; index < BufferSize; index++) {
-    if (Buffer[index] != 0xFF) {
-      return FALSE;
-    }
-  }
-
-  return TRUE;
-}
-
-/**
-  Check the integrity of CFV data.
-
-  @param[in] TdxCfvBase - A pointer to CFV header
-  @param[in] TdxCfvSize - CFV data size
-
-  @retval  TRUE   - The CFV data is valid.
-  @retval  FALSE  - The CFV data is invalid.
-
-**/
-BOOLEAN
-EFIAPI
-TdxValidateCfv (
-  IN UINT8   *TdxCfvBase,
-  IN UINT32  TdxCfvSize
-  )
-{
-  UINT16                         Checksum;
-  UINTN                          VariableBase;
-  UINT32                         VariableOffset;
-  UINT32                         VariableOffsetBeforeAlign;
-  EFI_FIRMWARE_VOLUME_HEADER     *CfvFvHeader;
-  VARIABLE_STORE_HEADER          *CfvVariableStoreHeader;
-  AUTHENTICATED_VARIABLE_HEADER  *VariableHeader;
-
-  static EFI_GUID  FvHdrGUID       = EFI_SYSTEM_NV_DATA_FV_GUID;
-  static EFI_GUID  VarStoreHdrGUID = EFI_AUTHENTICATED_VARIABLE_GUID;
-
-  VariableOffset = 0;
-
-  if (TdxCfvBase == NULL) {
-    DEBUG ((DEBUG_ERROR, "TDX CFV: CFV pointer is NULL\n"));
-    return FALSE;
-  }
-
-  //
-  // Verify the header zerovetor, filesystemguid,
-  // revision, signature, attributes, fvlength, checksum
-  // HeaderLength cannot be an odd number
-  //
-  CfvFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)TdxCfvBase;
-
-  if ((!IsZeroBuffer (CfvFvHeader->ZeroVector, 16)) ||
-      (!CompareGuid (&FvHdrGUID, &CfvFvHeader->FileSystemGuid)) ||
-      (CfvFvHeader->Signature != EFI_FVH_SIGNATURE) ||
-      (CfvFvHeader->Attributes != 0x4feff) ||
-      (CfvFvHeader->Revision != EFI_FVH_REVISION) ||
-      (CfvFvHeader->FvLength != TdxCfvSize)
-      )
-  {
-    DEBUG ((DEBUG_ERROR, "TDX CFV: Basic FV headers were invalid\n"));
-    return FALSE;
-  }
-
-  //
-  // Verify the header checksum
-  //
-  Checksum = CalculateSum16 ((VOID *)CfvFvHeader, CfvFvHeader->HeaderLength);
-
-  if (Checksum != 0) {
-    DEBUG ((DEBUG_ERROR, "TDX CFV: FV checksum was invalid\n"));
-    return FALSE;
-  }
-
-  //
-  // Verify the header signature, size, format, state
-  //
-  CfvVariableStoreHeader = (VARIABLE_STORE_HEADER *)(TdxCfvBase + CfvFvHeader->HeaderLength);
-  if ((!CompareGuid (&VarStoreHdrGUID, &CfvVariableStoreHeader->Signature)) ||
-      (CfvVariableStoreHeader->Format != VARIABLE_STORE_FORMATTED) ||
-      (CfvVariableStoreHeader->State != VARIABLE_STORE_HEALTHY) ||
-      (CfvVariableStoreHeader->Size > (CfvFvHeader->FvLength - CfvFvHeader->HeaderLength)) ||
-      (CfvVariableStoreHeader->Size < sizeof (VARIABLE_STORE_HEADER))
-      )
-  {
-    DEBUG ((DEBUG_ERROR, "TDX CFV: Variable Store header was invalid\n"));
-    return FALSE;
-  }
-
-  //
-  // Verify the header startId, state
-  // Verify data to the end
-  //
-  VariableBase = (UINTN)TdxCfvBase + CfvFvHeader->HeaderLength + sizeof (VARIABLE_STORE_HEADER);
-  while (VariableOffset  < (CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER))) {
-    VariableHeader = (AUTHENTICATED_VARIABLE_HEADER *)(VariableBase + VariableOffset);
-    if (VariableHeader->StartId != VARIABLE_DATA) {
-      if (!CheckPaddingData ((UINT8 *)VariableHeader, CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER) - VariableOffset)) {
-        DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
-        return FALSE;
-      }
-
-      VariableOffset = CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER);
-    } else {
-      if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) ||
-            (VariableHeader->State == VAR_DELETED) ||
-            (VariableHeader->State == VAR_HEADER_VALID_ONLY) ||
-            (VariableHeader->State == VAR_ADDED)))
-      {
-        DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
-        return FALSE;
-      }
-
-      VariableOffset += sizeof (AUTHENTICATED_VARIABLE_HEADER) + VariableHeader->NameSize + VariableHeader->DataSize;
-      // Verify VariableOffset should be less than or equal CfvVariableStoreHeader->Size - sizeof(VARIABLE_STORE_HEADER)
-      if (VariableOffset > (CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER))) {
-        DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
-        return FALSE;
-      }
-
-      VariableOffsetBeforeAlign = VariableOffset;
-      // 4 byte align
-      VariableOffset = (VariableOffset  + 3) & (UINTN)(~3);
-
-      if (!CheckPaddingData ((UINT8 *)(VariableBase + VariableOffsetBeforeAlign), VariableOffset - VariableOffsetBeforeAlign)) {
-        DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
-        return FALSE;
-      }
-    }
-  }
-
-  return TRUE;
-}
-
 /**
   Measure the Hoblist passed from the VMM.
 
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h
index 74b5f46552c2..09cac3e26c67 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h
@@ -52,23 +52,6 @@ EFIAPI
 ConstructSecHobList (
   );
 
-/**
-  Check the integrity of CFV data.
-
-  @param[in] TdxCfvBase - A pointer to CFV header
-  @param[in] TdxCfvSize - CFV data size
-
-  @retval  TRUE   - The CFV data is valid.
-  @retval  FALSE  - The CFV data is invalid.
-
-**/
-BOOLEAN
-EFIAPI
-TdxValidateCfv (
-  IN UINT8   *TdxCfvBase,
-  IN UINT32  TdxCfvSize
-  );
-
 /**
   Measure the Hoblist passed from the VMM.
 
diff --git a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
index c6d7c8bb6e0e..626a670c9c64 100644
--- a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
+++ b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
@@ -20,6 +20,8 @@
 #include <Library/PeiServicesLib.h>
 #include <Library/TdxLib.h>
 #include <Library/SynchronizationLib.h>
+#include <Guid/SystemNvDataGuid.h>
+#include <Guid/VariableFormat.h>
 #include <WorkArea.h>
 #include <ConfidentialComputingGuestAttr.h>
 
@@ -561,3 +563,154 @@ PlatformTdxPublishRamRegions (
       );
   }
 }
+
+/**
+  Check padding data all bit should be 1.
+
+  @param[in] Buffer     - A pointer to buffer header
+  @param[in] BufferSize - Buffer size
+
+  @retval  TRUE   - The padding data is valid.
+  @retval  TRUE  - The padding data is invalid.
+
+**/
+BOOLEAN
+CheckPaddingData (
+  IN UINT8   *Buffer,
+  IN UINT32  BufferSize
+  )
+{
+  UINT32  index;
+
+  for (index = 0; index < BufferSize; index++) {
+    if (Buffer[index] != 0xFF) {
+      return FALSE;
+    }
+  }
+
+  return TRUE;
+}
+
+/**
+  Check the integrity of CFV data.
+
+  @param[in] TdxCfvBase - A pointer to CFV header
+  @param[in] TdxCfvSize - CFV data size
+
+  @retval  TRUE   - The CFV data is valid.
+  @retval  FALSE  - The CFV data is invalid.
+
+**/
+BOOLEAN
+EFIAPI
+TdxValidateCfv (
+  IN UINT8   *TdxCfvBase,
+  IN UINT32  TdxCfvSize
+  )
+{
+  UINT16                         Checksum;
+  UINTN                          VariableBase;
+  UINT32                         VariableOffset;
+  UINT32                         VariableOffsetBeforeAlign;
+  EFI_FIRMWARE_VOLUME_HEADER     *CfvFvHeader;
+  VARIABLE_STORE_HEADER          *CfvVariableStoreHeader;
+  AUTHENTICATED_VARIABLE_HEADER  *VariableHeader;
+
+  static EFI_GUID  FvHdrGUID       = EFI_SYSTEM_NV_DATA_FV_GUID;
+  static EFI_GUID  VarStoreHdrGUID = EFI_AUTHENTICATED_VARIABLE_GUID;
+
+  VariableOffset = 0;
+
+  if (TdxCfvBase == NULL) {
+    DEBUG ((DEBUG_ERROR, "TDX CFV: CFV pointer is NULL\n"));
+    return FALSE;
+  }
+
+  //
+  // Verify the header zerovetor, filesystemguid,
+  // revision, signature, attributes, fvlength, checksum
+  // HeaderLength cannot be an odd number
+  //
+  CfvFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)TdxCfvBase;
+
+  if ((!IsZeroBuffer (CfvFvHeader->ZeroVector, 16)) ||
+      (!CompareGuid (&FvHdrGUID, &CfvFvHeader->FileSystemGuid)) ||
+      (CfvFvHeader->Signature != EFI_FVH_SIGNATURE) ||
+      (CfvFvHeader->Attributes != 0x4feff) ||
+      (CfvFvHeader->Revision != EFI_FVH_REVISION) ||
+      (CfvFvHeader->FvLength != TdxCfvSize)
+      )
+  {
+    DEBUG ((DEBUG_ERROR, "TDX CFV: Basic FV headers were invalid\n"));
+    return FALSE;
+  }
+
+  //
+  // Verify the header checksum
+  //
+  Checksum = CalculateSum16 ((VOID *)CfvFvHeader, CfvFvHeader->HeaderLength);
+
+  if (Checksum != 0) {
+    DEBUG ((DEBUG_ERROR, "TDX CFV: FV checksum was invalid\n"));
+    return FALSE;
+  }
+
+  //
+  // Verify the header signature, size, format, state
+  //
+  CfvVariableStoreHeader = (VARIABLE_STORE_HEADER *)(TdxCfvBase + CfvFvHeader->HeaderLength);
+  if ((!CompareGuid (&VarStoreHdrGUID, &CfvVariableStoreHeader->Signature)) ||
+      (CfvVariableStoreHeader->Format != VARIABLE_STORE_FORMATTED) ||
+      (CfvVariableStoreHeader->State != VARIABLE_STORE_HEALTHY) ||
+      (CfvVariableStoreHeader->Size > (CfvFvHeader->FvLength - CfvFvHeader->HeaderLength)) ||
+      (CfvVariableStoreHeader->Size < sizeof (VARIABLE_STORE_HEADER))
+      )
+  {
+    DEBUG ((DEBUG_ERROR, "TDX CFV: Variable Store header was invalid\n"));
+    return FALSE;
+  }
+
+  //
+  // Verify the header startId, state
+  // Verify data to the end
+  //
+  VariableBase = (UINTN)TdxCfvBase + CfvFvHeader->HeaderLength + sizeof (VARIABLE_STORE_HEADER);
+  while (VariableOffset  < (CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER))) {
+    VariableHeader = (AUTHENTICATED_VARIABLE_HEADER *)(VariableBase + VariableOffset);
+    if (VariableHeader->StartId != VARIABLE_DATA) {
+      if (!CheckPaddingData ((UINT8 *)VariableHeader, CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER) - VariableOffset)) {
+        DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
+        return FALSE;
+      }
+
+      VariableOffset = CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER);
+    } else {
+      if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) ||
+            (VariableHeader->State == VAR_DELETED) ||
+            (VariableHeader->State == VAR_HEADER_VALID_ONLY) ||
+            (VariableHeader->State == VAR_ADDED)))
+      {
+        DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
+        return FALSE;
+      }
+
+      VariableOffset += sizeof (AUTHENTICATED_VARIABLE_HEADER) + VariableHeader->NameSize + VariableHeader->DataSize;
+      // Verify VariableOffset should be less than or equal CfvVariableStoreHeader->Size - sizeof(VARIABLE_STORE_HEADER)
+      if (VariableOffset > (CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER))) {
+        DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
+        return FALSE;
+      }
+
+      VariableOffsetBeforeAlign = VariableOffset;
+      // 4 byte align
+      VariableOffset = (VariableOffset  + 3) & (UINTN)(~3);
+
+      if (!CheckPaddingData ((UINT8 *)(VariableBase + VariableOffsetBeforeAlign), VariableOffset - VariableOffsetBeforeAlign)) {
+        DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
+        return FALSE;
+      }
+    }
+  }
+
+  return TRUE;
+}
-- 
2.29.2.windows.2

[PATCH 2/3] OvmfPkg: Validate Cfv integrity in Tdx guest

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

Validate Configurtion FV (CFV) in Tdx guest.

Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/Sec/SecMain.c   | 8 ++++++++
 OvmfPkg/Sec/SecMain.inf | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index 1167d22a68cc..f6c00b8dab96 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -768,6 +768,14 @@ SecCoreStartupWithStack (
     if (ProcessTdxHobList () != EFI_SUCCESS) {
       CpuDeadLoop ();
     }
+
+    //
+    // Config FV (Cfv) contains the configuration information and its integrity
+    // should be validated.
+    //
+    if (!TdxValidateCfv ((UINT8 *)(UINTN)FixedPcdGet32 (PcdCfvBase), FixedPcdGet32 (PcdCfvRawDataSize))) {
+      CpuDeadLoop ();
+    }
   }
 
  #endif
diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
index 561a840f29c5..ae0094a15eda 100644
--- a/OvmfPkg/Sec/SecMain.inf
+++ b/OvmfPkg/Sec/SecMain.inf
@@ -84,6 +84,8 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
   gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
-- 
2.29.2.windows.2

[PATCH 3/3] OvmfPkg: Initialize NvVarStore with Configuration FV in Td guest

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

QEMU command option -pflash is not supported in Tdx guest. When Tdx guest
is booted, EmuVariableFvbRuntimeDxe driver is loaded and the NvVarStore
is initialized with empty content. This patch is to initialize the
NvVarStore with the content of Configuration FV (CFV).

Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c   | 19 +++++++++++++++++++
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf |  2 ++
 2 files changed, 21 insertions(+)

diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
index 4fc715dc3681..96895272d806 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
@@ -717,6 +717,8 @@ FvbInitialize (
   EFI_HANDLE            Handle;
   EFI_PHYSICAL_ADDRESS  Address;
   RETURN_STATUS         PcdStatus;
+  UINT8                 *CfvBase;
+  UINT32                CfvSize;
 
   DEBUG ((DEBUG_INFO, "EMU Variable FVB Started\n"));
 
@@ -774,6 +776,23 @@ FvbInitialize (
 
   mEmuVarsFvb.BufferPtr = Ptr;
 
+  //
+  // In Tdx guest the VarNvStore content should be initialized by the Configuration FV (CFV).
+  // Integrity of the CFV has been validated by TdxValidateCfv (@PlatformInitLib)
+  //
+  if (TdIsEnabled ()) {
+    CfvBase = (UINT8 *)(UINTN)PcdGet32 (PcdCfvBase);
+    CfvSize = (UINT32)PcdGet32 (PcdCfvRawDataSize);
+
+    if (CfvSize > mEmuVarsFvb.Size) {
+      DEBUG ((DEBUG_ERROR, "Size of CFV is larger than the EMU Variable FVB.\n"));
+      ASSERT (FALSE);
+    } else {
+      CopyMem (Ptr, CfvBase, CfvSize);
+      Initialize = FALSE;
+    }
+  }
+
   //
   // Initialize the main FV header and variable store header
   //
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
index 0811545cf7b3..15e8e673e8a0 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
@@ -56,6 +56,8 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
-- 
2.29.2.windows.2

Re: [PATCH 3/3] OvmfPkg: Initialize NvVarStore with Configuration FV in Td guest

[Gerd Hoffmann]

> +  UINT8                 *CfvBase;
> +  UINT32                CfvSize;
>  
>    DEBUG ((DEBUG_INFO, "EMU Variable FVB Started\n"));
>  
> @@ -774,6 +776,23 @@ FvbInitialize (
>  
>    mEmuVarsFvb.BufferPtr = Ptr;
>  
> +  //
> +  // In Tdx guest the VarNvStore content should be initialized by the Configuration FV (CFV).
> +  // Integrity of the CFV has been validated by TdxValidateCfv (@PlatformInitLib)
> +  //
> +  if (TdIsEnabled ()) {
> +    CfvBase = (UINT8 *)(UINTN)PcdGet32 (PcdCfvBase);
> +    CfvSize = (UINT32)PcdGet32 (PcdCfvRawDataSize);
> +
> +    if (CfvSize > mEmuVarsFvb.Size) {
> +      DEBUG ((DEBUG_ERROR, "Size of CFV is larger than the EMU Variable FVB.\n"));
> +      ASSERT (FALSE);
> +    } else {
> +      CopyMem (Ptr, CfvBase, CfvSize);
> +      Initialize = FALSE;
> +    }
> +  }

There is PcdEmuVariableNvStoreReserved for that.  How about just
copying the store to ram, then set PcdEmuVariableNvStoreReserved
to the location and let the existing logic handle it?

Also why limit this to tdx?

take care,
  Gerd

Re: [PATCH 3/3] OvmfPkg: Initialize NvVarStore with Configuration FV in Td guest

[Min Xu]

On June 20, 2022 7:01 PM, Gerd Hoffman wrote:
> 
> > +  UINT8                 *CfvBase;
> > +  UINT32                CfvSize;
> >
> >    DEBUG ((DEBUG_INFO, "EMU Variable FVB Started\n"));
> >
> > @@ -774,6 +776,23 @@ FvbInitialize (
> >
> >    mEmuVarsFvb.BufferPtr = Ptr;
> >
> > +  //
> > +  // In Tdx guest the VarNvStore content should be initialized by the
> Configuration FV (CFV).
> > +  // Integrity of the CFV has been validated by TdxValidateCfv
> > + (@PlatformInitLib)  //  if (TdIsEnabled ()) {
> > +    CfvBase = (UINT8 *)(UINTN)PcdGet32 (PcdCfvBase);
> > +    CfvSize = (UINT32)PcdGet32 (PcdCfvRawDataSize);
> > +
> > +    if (CfvSize > mEmuVarsFvb.Size) {
> > +      DEBUG ((DEBUG_ERROR, "Size of CFV is larger than the EMU Variable
> FVB.\n"));
> > +      ASSERT (FALSE);
> > +    } else {
> > +      CopyMem (Ptr, CfvBase, CfvSize);
> > +      Initialize = FALSE;
> > +    }
> > +  }
> 
> There is PcdEmuVariableNvStoreReserved for that.  How about just copying
> the store to ram, then set PcdEmuVariableNvStoreReserved to the location
> and let the existing logic handle it?
There is ReserveEmuVariableNvStore in PlatformPei/Platform.c. This function is called to allocate storage for NV Variables. PcdEmuVariableNvStoreReserved is set in that function too. So we can copy the content to that reserved storage if it is tdx guest. Then we let the exiting logic to handle it. So I would like to extract ReserveEmuVariableNvStore to PlatformReserveEmuVariableNvStore (in PlatformInitLib) and call it in both PlatformPei/Platform.c and PeilesssStartup.c.
What's your thought?
> 
> Also why limit this to tdx?
Because I am not sure if other platforms need such operation. So in current stage it is limit to tdx.

Thanks
Min

Re: [PATCH 3/3] OvmfPkg: Initialize NvVarStore with Configuration FV in Td guest

[Gerd Hoffmann]

On Wed, Jun 22, 2022 at 02:02:00AM +0000, Xu, Min M wrote:
> On June 20, 2022 7:01 PM, Gerd Hoffman wrote:
> > 
> > There is PcdEmuVariableNvStoreReserved for that.  How about just copying
> > the store to ram, then set PcdEmuVariableNvStoreReserved to the location
> > and let the existing logic handle it?

> There is ReserveEmuVariableNvStore in PlatformPei/Platform.c. This
> function is called to allocate storage for NV Variables.
> PcdEmuVariableNvStoreReserved is set in that function too. So we can
> copy the content to that reserved storage if it is tdx guest. Then we
> let the exiting logic to handle it. So I would like to extract
> ReserveEmuVariableNvStore to PlatformReserveEmuVariableNvStore (in
> PlatformInitLib) and call it in both PlatformPei/Platform.c and
> PeilesssStartup.c.

Moving the ReserveEmuVariableNvStore() function to PlatformInitLib make
sense.  Will be a bit more than pure code motion though, we probably
need a new variable in the platforminfo struct because PeilesssStartup.c
can't set PCDs.

Copying over the content in PlatformInitLib makes sense too, probably
best as separate function.

> > Also why limit this to tdx?
> Because I am not sure if other platforms need such operation. So in
> current stage it is limit to tdx.

I think the code should copy over the varstore in case the
SECURE_BOOT_ENABLE option is set.  That is the actual use case and it
makes sense without TDX too.

take care,
  Gerd

Re: [PATCH 3/3] OvmfPkg: Initialize NvVarStore with Configuration FV in Td guest

[Min Xu]

On June 22, 2022 3:01 PM, Gerd Hoffmann wrote:
> On Wed, Jun 22, 2022 at 02:02:00AM +0000, Xu, Min M wrote:
> > On June 20, 2022 7:01 PM, Gerd Hoffman wrote:
> > >
> > > There is PcdEmuVariableNvStoreReserved for that.  How about just
> > > copying the store to ram, then set PcdEmuVariableNvStoreReserved to
> > > the location and let the existing logic handle it?
> 
> > There is ReserveEmuVariableNvStore in PlatformPei/Platform.c. This
> > function is called to allocate storage for NV Variables.
> > PcdEmuVariableNvStoreReserved is set in that function too. So we can
> > copy the content to that reserved storage if it is tdx guest. Then we
> > let the exiting logic to handle it. So I would like to extract
> > ReserveEmuVariableNvStore to PlatformReserveEmuVariableNvStore (in
> > PlatformInitLib) and call it in both PlatformPei/Platform.c and
> > PeilesssStartup.c.
> 
> Moving the ReserveEmuVariableNvStore() function to PlatformInitLib make
> sense.  Will be a bit more than pure code motion though, we probably need
> a new variable in the platforminfo struct because PeilesssStartup.c can't set
> PCDs.
I check the current PlatformInfoHob and PcdEmuVariableNvStoreReserved has already been defined.
> 
> Copying over the content in PlatformInitLib makes sense too, probably best
> as separate function.
Yes, PlatformReserveEmuVariableNvStore() will be a separated function and it returns the pointer of the allocated storage. Then this pointer can be set to either the PCD (PlatformPei) or in PlatformInfoHob (PeilessStartupLib).
> 
> > > Also why limit this to tdx?
> > Because I am not sure if other platforms need such operation. So in
> > current stage it is limit to tdx.
> 
> I think the code should copy over the varstore in case the
> SECURE_BOOT_ENABLE option is set.  That is the actual use case and it
> makes sense without TDX too.
Then we need add a build-flag in *.dsc. Do you think OvmfPkgX64.dsc and IntelTdxX64.dsc are enough?

Thanks
Min

Re: [PATCH 3/3] OvmfPkg: Initialize NvVarStore with Configuration FV in Td guest

[Gerd Hoffmann]

> > Copying over the content in PlatformInitLib makes sense too, probably best
> > as separate function.

> Yes, PlatformReserveEmuVariableNvStore() will be a separated function
> and it returns the pointer of the allocated storage. Then this pointer
> can be set to either the PCD (PlatformPei) or in PlatformInfoHob
> (PeilessStartupLib).

I mean copying over should be a separate function too, so it is up to
the caller not the library itself to decide whenever the copying should
happen or not.

> > > > Also why limit this to tdx?
> > > Because I am not sure if other platforms need such operation. So in
> > > current stage it is limit to tdx.
> > 
> > I think the code should copy over the varstore in case the
> > SECURE_BOOT_ENABLE option is set.  That is the actual use case and it
> > makes sense without TDX too.
> Then we need add a build-flag in *.dsc. Do you think OvmfPkgX64.dsc and IntelTdxX64.dsc are enough?

The flag is already there ;)

take care,
  Gerd

Re: [PATCH 3/3] OvmfPkg: Initialize NvVarStore with Configuration FV in Td guest

[Min Xu]

On June 22, 2022 5:23 PM, Gerd Hoffmann wrote:
> 
> > Yes, PlatformReserveEmuVariableNvStore() will be a separated function
> > and it returns the pointer of the allocated storage. Then this pointer
> > can be set to either the PCD (PlatformPei) or in PlatformInfoHob
> > (PeilessStartupLib).
> 
> I mean copying over should be a separate function too, so it is up to the
> caller not the library itself to decide whenever the copying should happen or
> not.
I see. I will do that.
> 
> > > > > Also why limit this to tdx?
> > > > Because I am not sure if other platforms need such operation. So
> > > > in current stage it is limit to tdx.
> > >
> > > I think the code should copy over the varstore in case the
> > > SECURE_BOOT_ENABLE option is set.  That is the actual use case and
> > > it makes sense without TDX too.
> > Then we need add a build-flag in *.dsc. Do you think OvmfPkgX64.dsc and
> IntelTdxX64.dsc are enough?
> 
> The flag is already there ;)
SECURE_BOOT_ENABLE is not a build-flag. It can only works in .dsc files. 
The build-flag(SECURE_BOOT_FEATURE_ENABLED) would be defined in .dsc:
[BuildOptions]
!if $(SECURE_BOOT_ENABLE) == TRUE
  MSFT:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
  INTEL:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
  GCC:*_*_*_CC_FLAGS = -D SECURE_BOOT_FEATURE_ENABLED
!endif

Then it will be used like:
#ifdef   SECURE_BOOT_FEATURE_ENABLED
  CopyOverVarStoreContent()
#endif

In this way, there are totally 7 dsc in OvmfPkg need updated.
 - IntelTdx/IntelTdxX64.dsc
 - CloudHv/CloudHvX64.dsc
 - Bhyve/BhyveX64.dsc
 - Microvm/MicrovmX64.dsc
 - OvmfPkgIa32.dsc 
 - OvmfPkgX64.dsc
 - OvmfPkgIa32X64.dsc

Thanks
Min

Re: [PATCH 3/3] OvmfPkg: Initialize NvVarStore with Configuration FV in Td guest

[Gerd Hoffmann]

> > The flag is already there ;)
> SECURE_BOOT_ENABLE is not a build-flag. It can only works in .dsc files. 
> The build-flag(SECURE_BOOT_FEATURE_ENABLED) would be defined in .dsc:
> [BuildOptions]
> !if $(SECURE_BOOT_ENABLE) == TRUE
>   MSFT:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
>   INTEL:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
>   GCC:*_*_*_CC_FLAGS = -D SECURE_BOOT_FEATURE_ENABLED
> !endif

Ah, I see.

I've expected that to happen automatically, similar to the the linux
kernel, where all CONFIG_* variables are available to both build system
and C source code.

take care,
  Gerd