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>

8 files changed, 87 insertions, 151 deletions

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 19abb16e79..e64c0ee324 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/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);

@@ -1026,6 +1029,13 @@ CreateSmmSmramMemoryHob (
   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.

   //

   SmramHobDescriptorBlock->Descriptor[1].PhysicalStart = SmramHobDescriptorBlock->Descriptor[0].PhysicalStart + EFI_PAGE_SIZE;

diff --git a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
index 2bb1c0296f..21e6efa5e0 100644
--- a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
+++ b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
@@ -58,6 +58,7 @@
 

 [Guids]

   gEfiSmmSmramMemoryGuid

+  gEfiAcpiVariableGuid

 

 [Pcd]

   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress

diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.c b/OvmfPkg/SmmAccess/SmmAccess2Dxe.c
index 4b9e6df37f..3371592de7 100644
--- a/OvmfPkg/SmmAccess/SmmAccess2Dxe.c
+++ b/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

            );

diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
index d86381d0fb..d9f01a13c4 100644
--- a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
+++ b/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

diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c b/OvmfPkg/SmmAccess/SmmAccessPei.c
index 0e57b7804c..ded22f035d 100644
--- a/OvmfPkg/SmmAccess/SmmAccessPei.c
+++ b/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,

-  - set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and expose

-    it via the gEfiAcpiVariableGuid GUID HOB.

+  - provide SMRAM access by producing PEI_SMM_ACCESS_PPI

 

   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

            );

@@ -240,14 +264,10 @@ SmmAccessPeiEntryPoint (
   IN CONST EFI_PEI_SERVICES     **PeiServices

   )

 {

-  UINT16                HostBridgeDevId;

-  UINT8                 EsmramcVal;

-  UINT8                 RegMask8;

-  UINT32                TopOfLowRam, TopOfLowRamMb;

-  EFI_STATUS            Status;

-  UINTN                 SmramMapSize;

-  EFI_SMRAM_DESCRIPTOR  SmramMap[DescIdxCount];

-  VOID                  *GuidHob;

+  UINT16  HostBridgeDevId;

+  UINT8   EsmramcVal;

+  UINT8   RegMask8;

+  UINT32  TopOfLowRam, TopOfLowRamMb;

 

   //

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

diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.inf b/OvmfPkg/SmmAccess/SmmAccessPei.inf
index 1698c4ce6c..1e7cedf290 100644
--- a/OvmfPkg/SmmAccess/SmmAccessPei.inf
+++ b/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,

-# - set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and expose

-#   it via the gEfiAcpiVariableGuid GUIDed HOB.

+# - verify & configure the Q35 TSEG in the entry point.

 #

 # 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

 

diff --git a/OvmfPkg/SmmAccess/SmramInternal.c b/OvmfPkg/SmmAccess/SmramInternal.c
index d391ddc9ae..ff67d302a2 100644
--- a/OvmfPkg/SmmAccess/SmramInternal.c
+++ b/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;

-  UINT32  TsegMemoryBaseMb, TsegMemoryBase;

-  UINT64  CommonRegionState;

-  UINT8   TsegSizeBits;

-

-  OriginalSize  = *SmramMapSize;

-  *SmramMapSize = DescIdxCount * sizeof *SmramMap;

-  if (OriginalSize < *SmramMapSize) {

-    return EFI_BUFFER_TOO_SMALL;

-  }

+  UINTN                           BufferSize;

+  EFI_HOB_GUID_TYPE               *GuidHob;

+  EFI_SMRAM_HOB_DESCRIPTOR_BLOCK  *DescriptorBlock;

+  UINTN                           Index;

 

   //

-  // Read the TSEG Memory Base register.

+  // Get Hob list

   //

-  TsegMemoryBaseMb = PciRead32 (DRAMC_REGISTER_Q35 (MCH_TSEGMB));

-  TsegMemoryBase   = (TsegMemoryBaseMb >> MCH_TSEGMB_MB_SHIFT) << 20;

+  GuidHob         = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid);

+  DescriptorBlock = GET_GUID_HOB_DATA (GuidHob);

+  ASSERT (DescriptorBlock);

 

-  //

-  // Precompute the region state bits that will be set for all regions.

-  //

-  CommonRegionState = (OpenState ? EFI_SMRAM_OPEN : EFI_SMRAM_CLOSED) |

-                      (LockState ? EFI_SMRAM_LOCKED : 0) |

-                      EFI_CACHEABLE;

+  BufferSize = DescriptorBlock->NumberOfSmmReservedRegions * sizeof (EFI_SMRAM_DESCRIPTOR);

 

-  //

-  // 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;

+  if (*SmramMapSize < BufferSize) {

+    *SmramMapSize = BufferSize;

+    return EFI_BUFFER_TOO_SMALL;

+  }

 

   //

-  // Get the TSEG size bits from the ESMRAMC register.

+  // Update SmramMapSize to real return SMRAM map size

   //

-  TsegSizeBits = PciRead8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC)) &

-                 MCH_ESMRAMC_TSEG_MASK;

+  *SmramMapSize = BufferSize;

 

   //

-  // The second region is the main one, following the first.

+  // Use the hob to publish SMRAM capabilities

   //

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

+  for (Index = 0; Index < DescriptorBlock->NumberOfSmmReservedRegions; Index++) {

+    SmramMap[Index].PhysicalStart = DescriptorBlock->Descriptor[Index].PhysicalStart;

+    SmramMap[Index].CpuStart      = DescriptorBlock->Descriptor[Index].CpuStart;

+    SmramMap[Index].PhysicalSize  = DescriptorBlock->Descriptor[Index].PhysicalSize;

+    SmramMap[Index].RegionState   = DescriptorBlock->Descriptor[Index].RegionState;

+  }

 

   return EFI_SUCCESS;

 }

diff --git a/OvmfPkg/SmmAccess/SmramInternal.h b/OvmfPkg/SmmAccess/SmramInternal.h
index da5b7bbca1..aa23c6dff8 100644
--- a/OvmfPkg/SmmAccess/SmramInternal.h
+++ b/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>

 

-//

-// We'll have two SMRAM ranges.

-//

-// 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;

+#include <Guid/SmramMemoryReserve.h>

+#include <Library/HobLib.h>

 

 //

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

   );