Re: [edk2-devel] [PATCH v1 09/13] OvmfPkg/SmmAccess: Consume gEfiSmmSmramMemoryGuid

["Ni, Ray" <ray.ni@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 15943 bytes --]

Do you think it makes sense to combine this with #10? Otherwise, with this patch the OVMF is broken.

Thanks,
Ray
________________________________
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Wednesday, April 10, 2024 21:57
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Ni, Ray <ray.ni@intel.com>
Subject: [PATCH v1 09/13] OvmfPkg/SmmAccess: Consume gEfiSmmSmramMemoryGuid

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

 **/

@@ -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.<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>
 #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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117623): https://edk2.groups.io/g/devel/message/117623
Mute This Topic: https://groups.io/mt/105442001/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 27312 bytes --]