OvmfPkg: Refine SmmAccess implementation

This patch refines the SmmAccess implementation:
1. SmramMap will be retrieved from the
gEfiSmmSmramMemoryGuid instead of original from
the TSEG Memory Base register.
2. Remove the gEfiAcpiVariableGuid creation, thus
the DESCRIPTOR_INDEX definition can be also cleaned.
3. The gEfiAcpiVariableGuid HOB is moved to the
OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

OvmfPkg/Library/PlatformInitLib/MemDetect.c

@@ -42,6 +42,8 @@ Module Name:
 #include <Library/TdxLib.h>
 
 #include <Library/PlatformInitLib.h>
+
+#include <Guid/AcpiS3Context.h>
 #include <Guid/SmramMemoryReserve.h>
 
 #define MEGABYTE_SHIFT  20
@@ -1003,6 +1005,7 @@ CreateSmmSmramMemoryHob (
   UINT8                           SmramRanges;
   EFI_PEI_HOB_POINTERS            Hob;
   EFI_SMRAM_HOB_DESCRIPTOR_BLOCK  *SmramHobDescriptorBlock;
+  VOID                            *GuidHob;
 
   SmramRanges = 2;
   BufferSize  = sizeof (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK) + (SmramRanges - 1) * sizeof (EFI_SMRAM_DESCRIPTOR);
@@ -1025,6 +1028,13 @@ CreateSmmSmramMemoryHob (
   SmramHobDescriptorBlock->Descriptor[0].PhysicalSize  = EFI_PAGE_SIZE;
   SmramHobDescriptorBlock->Descriptor[0].RegionState   = EFI_SMRAM_CLOSED | EFI_CACHEABLE | EFI_ALLOCATED;
 
+  //
+  // 1.1 Create gEfiAcpiVariableGuid according SmramHobDescriptorBlock->Descriptor[0] since it's used in S3 resume.
+  //
+  GuidHob = BuildGuidHob (&gEfiAcpiVariableGuid, sizeof (EFI_SMRAM_DESCRIPTOR));
+  ASSERT (GuidHob != NULL);
+  CopyMem (GuidHob, &SmramHobDescriptorBlock->Descriptor[0], sizeof (EFI_SMRAM_DESCRIPTOR));
+
   //
   // 2. Create second SMRAM descriptor, which is free and will be used by SMM foundation.
   //

OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf

@@ -58,6 +58,7 @@
 
 [Guids]
   gEfiSmmSmramMemoryGuid
+  gEfiAcpiVariableGuid
 
 [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress

OvmfPkg/SmmAccess/SmmAccess2Dxe.c

@@ -6,7 +6,7 @@
   driver.
 
   Copyright (C) 2013, 2015, Red Hat, Inc.<BR>
-  Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2009 - 2024, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -115,8 +115,6 @@ SmmAccess2DxeGetCapabilities (
   )
 {
   return SmramAccessGetCapabilities (
-           This->LockState,
-           This->OpenState,
            SmramMapSize,
            SmramMap
            );

OvmfPkg/SmmAccess/SmmAccess2Dxe.inf

@@ -5,6 +5,7 @@
 # driver.
 #
 # Copyright (C) 2013, 2015, Red Hat, Inc.
+# Copyright (c) 2024 Intel Corporation.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -41,6 +42,7 @@
   PciLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
+  HobLib
 
 [Protocols]
   gEfiSmmAccess2ProtocolGuid   ## PRODUCES
@@ -48,6 +50,9 @@
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
 
+[Guids]
+  gEfiSmmSmramMemoryGuid             # ALWAYS_CONSUMED
+
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes

OvmfPkg/SmmAccess/SmmAccessPei.c

@@ -3,25 +3,21 @@
   A PEIM with the following responsibilities:
 
   - verify & configure the Q35 TSEG in the entry point,
-  - provide SMRAM access by producing PEI_SMM_ACCESS_PPI,
+  - provide SMRAM access by producing PEI_SMM_ACCESS_PPI
-  - set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and expose
-    it via the gEfiAcpiVariableGuid GUID HOB.
 
   This PEIM runs from RAM, so we can write to variables with static storage
   duration.
 
   Copyright (C) 2013, 2015, Red Hat, Inc.<BR>
-  Copyright (c) 2010, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2010 - 2024, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
-#include <Guid/AcpiS3Context.h>
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
-#include <Library/HobLib.h>
 #include <Library/IoLib.h>
 #include <Library/PcdLib.h>
 #include <Library/PciLib.h>
@@ -64,7 +60,17 @@ SmmAccessPeiOpen (
   IN UINTN               DescriptorIndex
   )
 {
-  if (DescriptorIndex >= DescIdxCount) {
+  EFI_HOB_GUID_TYPE               *GuidHob;
+  EFI_SMRAM_HOB_DESCRIPTOR_BLOCK  *DescriptorBlock;
+
+  //
+  // Get the number of regions in the system that can be usable for SMRAM
+  //
+  GuidHob         = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid);
+  DescriptorBlock = GET_GUID_HOB_DATA (GuidHob);
+  ASSERT (DescriptorBlock);
+
+  if (DescriptorIndex >= DescriptorBlock->NumberOfSmmReservedRegions) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -102,7 +108,17 @@ SmmAccessPeiClose (
   IN UINTN               DescriptorIndex
   )
 {
-  if (DescriptorIndex >= DescIdxCount) {
+  EFI_HOB_GUID_TYPE               *GuidHob;
+  EFI_SMRAM_HOB_DESCRIPTOR_BLOCK  *DescriptorBlock;
+
+  //
+  // Get the number of regions in the system that can be usable for SMRAM
+  //
+  GuidHob         = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid);
+  DescriptorBlock = GET_GUID_HOB_DATA (GuidHob);
+  ASSERT (DescriptorBlock);
+
+  if (DescriptorIndex >= DescriptorBlock->NumberOfSmmReservedRegions) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -139,7 +155,17 @@ SmmAccessPeiLock (
   IN UINTN               DescriptorIndex
   )
 {
-  if (DescriptorIndex >= DescIdxCount) {
+  EFI_HOB_GUID_TYPE               *GuidHob;
+  EFI_SMRAM_HOB_DESCRIPTOR_BLOCK  *DescriptorBlock;
+
+  //
+  // Get the number of regions in the system that can be usable for SMRAM
+  //
+  GuidHob         = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid);
+  DescriptorBlock = GET_GUID_HOB_DATA (GuidHob);
+  ASSERT (DescriptorBlock);
+
+  if (DescriptorIndex >= DescriptorBlock->NumberOfSmmReservedRegions) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -178,8 +204,6 @@ SmmAccessPeiGetCapabilities (
   )
 {
   return SmramAccessGetCapabilities (
-           This->LockState,
-           This->OpenState,
            SmramMapSize,
            SmramMap
            );
@@ -244,10 +268,6 @@ SmmAccessPeiEntryPoint (
   UINT8   EsmramcVal;
   UINT8   RegMask8;
   UINT32  TopOfLowRam, TopOfLowRamMb;
-  EFI_STATUS            Status;
-  UINTN                 SmramMapSize;
-  EFI_SMRAM_DESCRIPTOR  SmramMap[DescIdxCount];
-  VOID                  *GuidHob;
 
   //
   // This module should only be included if SMRAM support is required.
@@ -356,65 +376,7 @@ SmmAccessPeiEntryPoint (
     MCH_SMRAM_G_SMRAME
     );
 
-  //
-  // Create the GUID HOB and point it to the first SMRAM range.
-  //
   GetStates (&mAccess.LockState, &mAccess.OpenState);
-  SmramMapSize = sizeof SmramMap;
-  Status       = SmramAccessGetCapabilities (
-                   mAccess.LockState,
-                   mAccess.OpenState,
-                   &SmramMapSize,
-                   SmramMap
-                   );
-  ASSERT_EFI_ERROR (Status);
-
-  DEBUG_CODE_BEGIN ();
-  {
-    UINTN  Count;
-    UINTN  Idx;
-
-    Count = SmramMapSize / sizeof SmramMap[0];
-    DEBUG ((
-      DEBUG_VERBOSE,
-      "%a: SMRAM map follows, %d entries\n",
-      __func__,
-      (INT32)Count
-      ));
-    DEBUG ((
-      DEBUG_VERBOSE,
-      "% 20a % 20a % 20a % 20a\n",
-      "PhysicalStart(0x)",
-      "PhysicalSize(0x)",
-      "CpuStart(0x)",
-      "RegionState(0x)"
-      ));
-    for (Idx = 0; Idx < Count; ++Idx) {
-      DEBUG ((
-        DEBUG_VERBOSE,
-        "% 20Lx % 20Lx % 20Lx % 20Lx\n",
-        SmramMap[Idx].PhysicalStart,
-        SmramMap[Idx].PhysicalSize,
-        SmramMap[Idx].CpuStart,
-        SmramMap[Idx].RegionState
-        ));
-    }
-  }
-  DEBUG_CODE_END ();
-
-  GuidHob = BuildGuidHob (
-              &gEfiAcpiVariableGuid,
-              sizeof SmramMap[DescIdxSmmS3ResumeState]
-              );
-  if (GuidHob == NULL) {
-    return EFI_OUT_OF_RESOURCES;
-  }
-
-  CopyMem (
-    GuidHob,
-    &SmramMap[DescIdxSmmS3ResumeState],
-    sizeof SmramMap[DescIdxSmmS3ResumeState]
-    );
 
   //
   // SmramAccessLock() depends on "mQ35SmramAtDefaultSmbase"; init the latter

OvmfPkg/SmmAccess/SmmAccessPei.inf

@@ -2,11 +2,10 @@
 # A PEIM with the following responsibilities:
 #
 # - provide SMRAM access by producing PEI_SMM_ACCESS_PPI,
-# - verify & configure the Q35 TSEG in the entry point,
+# - verify & configure the Q35 TSEG in the entry point.
-# - set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and expose
-#   it via the gEfiAcpiVariableGuid GUIDed HOB.
 #
 # Copyright (C) 2013, 2015, Red Hat, Inc.
+# Copyright (c) 2024 Intel Corporation.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -36,9 +35,6 @@
   MdePkg/MdePkg.dec
   OvmfPkg/OvmfPkg.dec
 
-[Guids]
-  gEfiAcpiVariableGuid
-
 [LibraryClasses]
   BaseLib
   BaseMemoryLib
@@ -57,6 +53,9 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
 
+[Guids]
+  gEfiSmmSmramMemoryGuid             # ALWAYS_CONSUMED
+
 [Ppis]
   gPeiSmmAccessPpiGuid           ## PRODUCES
 

OvmfPkg/SmmAccess/SmramInternal.c

@@ -3,12 +3,11 @@
   Functions and types shared by the SMM accessor PEI and DXE modules.
 
   Copyright (C) 2015, Red Hat, Inc.
+  Copyright (c) 2024 Intel Corporation.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
-
-#include <Guid/AcpiS3Context.h>
 #include <IndustryStandard/Q35MchIch9.h>
 #include <Library/DebugLib.h>
 #include <Library/PcdLib.h>
@@ -166,68 +165,43 @@ SmramAccessLock (
 
 EFI_STATUS
 SmramAccessGetCapabilities (
-  IN BOOLEAN                   LockState,
-  IN BOOLEAN                   OpenState,
   IN OUT UINTN                 *SmramMapSize,
   IN OUT EFI_SMRAM_DESCRIPTOR  *SmramMap
   )
 {
-  UINTN   OriginalSize;
+  UINTN                           BufferSize;
-  UINT32  TsegMemoryBaseMb, TsegMemoryBase;
+  EFI_HOB_GUID_TYPE               *GuidHob;
-  UINT64  CommonRegionState;
+  EFI_SMRAM_HOB_DESCRIPTOR_BLOCK  *DescriptorBlock;
-  UINT8   TsegSizeBits;
+  UINTN                           Index;
 
-  OriginalSize  = *SmramMapSize;
+  //
-  *SmramMapSize = DescIdxCount * sizeof *SmramMap;
+  // Get Hob list
-  if (OriginalSize < *SmramMapSize) {
+  //
+  GuidHob         = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid);
+  DescriptorBlock = GET_GUID_HOB_DATA (GuidHob);
+  ASSERT (DescriptorBlock);
+
+  BufferSize = DescriptorBlock->NumberOfSmmReservedRegions * sizeof (EFI_SMRAM_DESCRIPTOR);
+
+  if (*SmramMapSize < BufferSize) {
+    *SmramMapSize = BufferSize;
     return EFI_BUFFER_TOO_SMALL;
   }
 
   //
-  // Read the TSEG Memory Base register.
+  // Update SmramMapSize to real return SMRAM map size
   //
-  TsegMemoryBaseMb = PciRead32 (DRAMC_REGISTER_Q35 (MCH_TSEGMB));
+  *SmramMapSize = BufferSize;
-  TsegMemoryBase   = (TsegMemoryBaseMb >> MCH_TSEGMB_MB_SHIFT) << 20;
 
   //
-  // Precompute the region state bits that will be set for all regions.
+  // Use the hob to publish SMRAM capabilities
   //
-  CommonRegionState = (OpenState ? EFI_SMRAM_OPEN : EFI_SMRAM_CLOSED) |
+  for (Index = 0; Index < DescriptorBlock->NumberOfSmmReservedRegions; Index++) {
-                      (LockState ? EFI_SMRAM_LOCKED : 0) |
+    SmramMap[Index].PhysicalStart = DescriptorBlock->Descriptor[Index].PhysicalStart;
-                      EFI_CACHEABLE;
+    SmramMap[Index].CpuStart      = DescriptorBlock->Descriptor[Index].CpuStart;
-
+    SmramMap[Index].PhysicalSize  = DescriptorBlock->Descriptor[Index].PhysicalSize;
-  //
+    SmramMap[Index].RegionState   = DescriptorBlock->Descriptor[Index].RegionState;
-  // The first region hosts an SMM_S3_RESUME_STATE object. It is located at the
+  }
-  // start of TSEG. We round up the size to whole pages, and we report it as
-  // EFI_ALLOCATED, so that the SMM_CORE stays away from it.
-  //
-  SmramMap[DescIdxSmmS3ResumeState].PhysicalStart = TsegMemoryBase;
-  SmramMap[DescIdxSmmS3ResumeState].CpuStart      = TsegMemoryBase;
-  SmramMap[DescIdxSmmS3ResumeState].PhysicalSize  =
-    EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (sizeof (SMM_S3_RESUME_STATE)));
-  SmramMap[DescIdxSmmS3ResumeState].RegionState =
-    CommonRegionState | EFI_ALLOCATED;
-
-  //
-  // Get the TSEG size bits from the ESMRAMC register.
-  //
-  TsegSizeBits = PciRead8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC)) &
-                 MCH_ESMRAMC_TSEG_MASK;
-
-  //
-  // The second region is the main one, following the first.
-  //
-  SmramMap[DescIdxMain].PhysicalStart =
-    SmramMap[DescIdxSmmS3ResumeState].PhysicalStart +
-    SmramMap[DescIdxSmmS3ResumeState].PhysicalSize;
-  SmramMap[DescIdxMain].CpuStart     = SmramMap[DescIdxMain].PhysicalStart;
-  SmramMap[DescIdxMain].PhysicalSize =
-    (TsegSizeBits == MCH_ESMRAMC_TSEG_8MB ? SIZE_8MB :
-     TsegSizeBits == MCH_ESMRAMC_TSEG_2MB ? SIZE_2MB :
-     TsegSizeBits == MCH_ESMRAMC_TSEG_1MB ? SIZE_1MB :
-     mQ35TsegMbytes * SIZE_1MB) -
-    SmramMap[DescIdxSmmS3ResumeState].PhysicalSize;
-  SmramMap[DescIdxMain].RegionState = CommonRegionState;
 
   return EFI_SUCCESS;
 }

OvmfPkg/SmmAccess/SmramInternal.h

@@ -3,6 +3,7 @@
   Functions and types shared by the SMM accessor PEI and DXE modules.
 
   Copyright (C) 2015, Red Hat, Inc.
+  Copyright (c) 2024 Intel Corporation.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -10,20 +11,8 @@
 
 #include <Pi/PiMultiPhase.h>
 
-//
+#include <Guid/SmramMemoryReserve.h>
-// We'll have two SMRAM ranges.
+#include <Library/HobLib.h>
-//
-// The first is a tiny one that hosts an SMM_S3_RESUME_STATE object, to be
-// filled in by the CPU SMM driver during normal boot, for the PEI instance of
-// the LockBox library (which will rely on the object during S3 resume).
-//
-// The other SMRAM range is the main one, for the SMM core and the SMM drivers.
-//
-typedef enum {
-  DescIdxSmmS3ResumeState = 0,
-  DescIdxMain             = 1,
-  DescIdxCount            = 2
-} DESCRIPTOR_INDEX;
 
 //
 // The value of PcdQ35TsegMbytes is saved into this variable at module startup.
@@ -97,8 +86,6 @@ SmramAccessLock (
 
 EFI_STATUS
 SmramAccessGetCapabilities (
-  IN BOOLEAN                   LockState,
-  IN BOOLEAN                   OpenState,
   IN OUT UINTN                 *SmramMapSize,
   IN OUT EFI_SMRAM_DESCRIPTOR  *SmramMap
   );