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.
The gEfiAcpiVariableGuid HOB will be created in 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>
---
OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 4 +-
OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 5 +++
OvmfPkg/SmmAccess/SmmAccessPei.c | 88 +++----------------------------------
OvmfPkg/SmmAccess/SmmAccessPei.inf | 7 +--
OvmfPkg/SmmAccess/SmramInternal.c | 73 +++++++++++-------------------
OvmfPkg/SmmAccess/SmramInternal.h | 18 +-------
6 files changed, 41 insertions(+), 154 deletions(-)

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
@@ -4,11 +4,11 @@

 Q35 TSEG is expected to have been verified and set up by the SmmAccessPei
 driver.

 Copyright (C) 2013, 2015, Red Hat, Inc. 
- Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved. 
+ Copyright (c) 2009 - 2024, Intel Corporation. All rights reserved. 

 SPDX-License-Identifier: BSD-2-Clause-Patent

**/

@@ -113,12 +113,10 @@ SmmAccess2DxeGetCapabilities (
 IN OUT UINTN *SmramMapSize,
 IN OUT EFI_SMRAM_DESCRIPTOR *SmramMap
 )
{
 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
@@ -3,10 +3,11 @@
#
# Q35 TSEG is expected to have been verified and set up by the SmmAccessPei
# driver.
#
# Copyright (C) 2013, 2015, Red Hat, Inc.
+# Copyright (c) 2024 Intel Corporation.
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##

@@ -39,17 +40,21 @@
 DebugLib
 PcdLib
 PciLib
 UefiBootServicesTableLib
 UefiDriverEntryPoint
+ HobLib

[Protocols]
 gEfiSmmAccess2ProtocolGuid ## PRODUCES

[FeaturePcd]
 gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire

+[Guids]
+ gEfiSmmSmramMemoryGuid # ALWAYS_CONSUMED
+
[Pcd]
 gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
 gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes

[Depex]
diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c b/OvmfPkg/SmmAccess/SmmAccessPei.c
index 0e57b7804c..9459bcc989 100644
--- a/OvmfPkg/SmmAccess/SmmAccessPei.c
+++ b/OvmfPkg/SmmAccess/SmmAccessPei.c
@@ -9,21 +9,19 @@

 This PEIM runs from RAM, so we can write to variables with static storage
 duration.

 Copyright (C) 2013, 2015, Red Hat, Inc. 
- Copyright (c) 2010, Intel Corporation. All rights reserved. 
+ Copyright (c) 2010 - 2024, Intel Corporation. All rights reserved. 

 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>
#include <Library/PeiServicesLib.h>
#include <Ppi/SmmAccess.h>
@@ -62,14 +60,10 @@ SmmAccessPeiOpen (
 IN EFI_PEI_SERVICES **PeiServices,
 IN PEI_SMM_ACCESS_PPI *This,
 IN UINTN DescriptorIndex
 )
{
- if (DescriptorIndex >= DescIdxCount) {
- return EFI_INVALID_PARAMETER;
- }
-
 //
 // According to current practice, DescriptorIndex is not considered at all,
 // beyond validating it.
 //
 return SmramAccessOpen (&This->LockState, &This->OpenState);
@@ -100,14 +94,10 @@ SmmAccessPeiClose (
 IN EFI_PEI_SERVICES **PeiServices,
 IN PEI_SMM_ACCESS_PPI *This,
 IN UINTN DescriptorIndex
 )
{
- if (DescriptorIndex >= DescIdxCount) {
- return EFI_INVALID_PARAMETER;
- }
-
 //
 // According to current practice, DescriptorIndex is not considered at all,
 // beyond validating it.
 //
 return SmramAccessClose (&This->LockState, &This->OpenState);
@@ -137,14 +127,10 @@ SmmAccessPeiLock (
 IN EFI_PEI_SERVICES **PeiServices,
 IN PEI_SMM_ACCESS_PPI *This,
 IN UINTN DescriptorIndex
 )
{
- if (DescriptorIndex >= DescIdxCount) {
- return EFI_INVALID_PARAMETER;
- }
-
 //
 // According to current practice, DescriptorIndex is not considered at all,
 // beyond validating it.
 //
 return SmramAccessLock (&This->LockState, &This->OpenState);
@@ -176,12 +162,10 @@ SmmAccessPeiGetCapabilities (
 IN OUT UINTN *SmramMapSize,
 IN OUT EFI_SMRAM_DESCRIPTOR *SmramMap
 )
{
 return SmramAccessGetCapabilities (
- This->LockState,
- This->OpenState,
 SmramMapSize,
 SmramMap
 );
}

@@ -238,18 +222,14 @@ EFIAPI
SmmAccessPeiEntryPoint (
 IN EFI_PEI_FILE_HANDLE FileHandle,
 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.
 //
 ASSERT (FeaturePcdGet (PcdSmmSmramRequire));
@@ -354,69 +334,11 @@ SmmAccessPeiEntryPoint (
 DRAMC_REGISTER_Q35 (MCH_SMRAM),
 (UINT8)((~(UINT32)MCH_SMRAM_D_LCK) & 0xff),
 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
 // just before exposing the former via PEI_SMM_ACCESS_PPI.Lock().
 //
diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.inf b/OvmfPkg/SmmAccess/SmmAccessPei.inf
index 1698c4ce6c..98bbda20e3 100644
--- a/OvmfPkg/SmmAccess/SmmAccessPei.inf
+++ b/OvmfPkg/SmmAccess/SmmAccessPei.inf
@@ -5,10 +5,11 @@
# - 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
#
##

@@ -34,13 +35,10 @@
[Packages]
 MdeModulePkg/MdeModulePkg.dec
 MdePkg/MdePkg.dec
 OvmfPkg/OvmfPkg.dec

-[Guids]
- gEfiAcpiVariableGuid
-
[LibraryClasses]
 BaseLib
 BaseMemoryLib
 DebugLib
 HobLib
@@ -55,10 +53,13 @@

[Pcd]
 gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
 gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes

+[Guids]
+ gEfiSmmSmramMemoryGuid # ALWAYS_CONSUMED
+
[Ppis]
 gPeiSmmAccessPpiGuid ## PRODUCES

[Depex]
 gEfiPeiMemoryDiscoveredPpiGuid
diff --git a/OvmfPkg/SmmAccess/SmramInternal.c b/OvmfPkg/SmmAccess/SmramInternal.c
index d391ddc9ae..312f67b304 100644
--- a/OvmfPkg/SmmAccess/SmramInternal.c
+++ b/OvmfPkg/SmmAccess/SmramInternal.c
@@ -1,20 +1,22 @@
/** @file

 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 <Guid/SmramMemoryReserve.h>
#include <IndustryStandard/Q35MchIch9.h>
#include <Library/DebugLib.h>
#include <Library/PcdLib.h>
#include <Library/PciLib.h>
+#include <Library/HobLib.h>

#include "SmramInternal.h"

//
// The value of PcdQ35TsegMbytes is saved into this variable at module startup.
@@ -164,70 +166,45 @@ SmramAccessLock (
 return EFI_SUCCESS;
}

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..4dd1c6dc9b 100644
--- a/OvmfPkg/SmmAccess/SmramInternal.h
+++ b/OvmfPkg/SmmAccess/SmramInternal.h
@@ -1,32 +1,18 @@
/** @file

 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 <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;
-
//
// The value of PcdQ35TsegMbytes is saved into this variable at module startup.
//
extern UINT16 mQ35TsegMbytes;

@@ -95,10 +81,8 @@ SmramAccessLock (
 IN OUT BOOLEAN *OpenState
 );

EFI_STATUS
SmramAccessGetCapabilities (
- IN BOOLEAN LockState,
- IN BOOLEAN OpenState,
 IN OUT UINTN *SmramMapSize,
 IN OUT EFI_SMRAM_DESCRIPTOR *SmramMap
 );
--
2.16.2.windows.1