[PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

[PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

[Wu, Jiaxin]

Mainly changes as below:
1. Add Smm Base HOB, which is used to store the information of
Smm Relocated SmBase array for each Processors;
2. Combine 2 SMIs (gcSmmInitTemplate & gcSmiHandlerTemplate) into one
(gcSmiHandlerTemplate), the new SMI handler needs to run to 2 paths: one
to SmmCpuFeaturesInitializeProcessor(), the other to SMM Core Entry Point.
3. Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
before normal SMI sources happen.
4. Call SmmCpuFeaturesInitializeProcessor() in parallel.

v2:
- Refine the coding style
- Rename hob to gSmmBaseHobGuid
- Update SmmInitHandler() to handle the SMM relocation
- Correct the S3 for SMM relocation

v1:
- Thread: https://edk2.groups.io/g/devel/message/97748

Change-Id: Iec7bf25166bfeefb44a202285465a35b5debbce4
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  36 +++++
 .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
 .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  24 +++-
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
 UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
 .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  39 +++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  25 +++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 149 ++++++++++++++++-----
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  21 ++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
 UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
 13 files changed, 261 insertions(+), 49 deletions(-)
 create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h

diff --git a/UefiCpuPkg/Include/Guid/SmmBaseHob.h b/UefiCpuPkg/Include/Guid/SmmBaseHob.h
new file mode 100644
index 0000000000..4729bbb986
--- /dev/null
+++ b/UefiCpuPkg/Include/Guid/SmmBaseHob.h
@@ -0,0 +1,36 @@
+/** @file
+  The Smm Base HOB is used to store the information of:
+  * Smm Relocated SmBase array for each Processors
+
+  Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef SMM_BASE_HOB_H_
+#define SMM_BASE_HOB_H_
+
+#include <Protocol/MpService.h>
+#include <PiPei.h>
+
+#define SMM_BASE_HOB_DATA_GUID \
+  { \
+    0xc2217ba7, 0x03bb, 0x4f63, {0xa6, 0x47, 0x7c, 0x25, 0xc5, 0xfc, 0x9d, 0x73}  \
+  }
+
+#pragma pack(1)
+typedef struct {
+  ///
+  /// Describes the Number of all max supported processors.
+  ///
+  UINT64    NumberOfProcessors;
+  ///
+  /// Pointer to SmBase array for each Processors.
+  ///
+  UINT64    SmBase[];
+} SMM_BASE_HOB_DATA;
+#pragma pack()
+
+extern EFI_GUID  gSmmBaseHobGuid;
+
+#endif
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
index fd3e902547..c2e4fbe96b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
@@ -7,15 +7,17 @@
 **/
 
 #ifndef CPU_FEATURES_LIB_H_
 #define CPU_FEATURES_LIB_H_
 
+#include <Guid/SmmBaseHob.h>
 #include <Library/SmmCpuFeaturesLib.h>
 #include <Library/BaseLib.h>
 #include <Library/PcdLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/DebugLib.h>
+#include <Library/HobLib.h>
 
 /**
   Performs library initialization.
 
   This initialization function contains common functionality shared betwen all
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
index d5eaaa7a99..9cedeee4bb 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
@@ -36,10 +36,15 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 // Set default value to assume IA-32 Architectural MSRs are used
 //
 UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
 UINT32  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
 
+//
+// Indicate Smm Relocation done or not
+//
+BOOLEAN  mSmmRelocationDone;
+
 //
 // Set default value to assume MTRRs need to be configured on each SMI
 //
 BOOLEAN  mNeedConfigureMtrrs = TRUE;
 
@@ -142,10 +147,17 @@ CpuFeaturesLibInitialization (
   //
   // Allocate array for state of SMRR enable on all CPUs
   //
   mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * GetCpuMaxLogicalProcessorNumber ());
   ASSERT (mSmrrEnabled != NULL);
+
+  //
+  // If gSmmBaseHobGuid found, means Smm Relocation has been done.
+  //
+  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
+    mSmmRelocationDone = TRUE;
+  }
 }
 
 /**
   Called during the very first SMI into System Management Mode to initialize
   CPU features, including SMBASE, for the currently executing CPU.  Since this
@@ -184,15 +196,17 @@ SmmCpuFeaturesInitializeProcessor (
   UINT32                RegEax;
   UINT32                RegEdx;
   UINTN                 FamilyId;
   UINTN                 ModelId;
 
-  //
-  // Configure SMBASE.
-  //
-  CpuState             = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
-  CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+  if (!mSmmRelocationDone) {
+    //
+    // Configure SMBASE.
+    //
+    CpuState             = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
+    CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+  }
 
   //
   // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
   // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
   //
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 9ac7dde78f..280a4b8b39 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -31,10 +31,14 @@
 [LibraryClasses]
   BaseLib
   PcdLib
   MemoryAllocationLib
   DebugLib
+  HobLib
+
+[Guids]
+  gSmmBaseHobGuid                ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
 
 [FeaturePcd]
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
index 86d367e0a0..4bb045244b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
@@ -62,10 +62,11 @@
 
 [Guids]
   gMsegSmramGuid                           ## SOMETIMES_CONSUMES ## HOB
   gEfiAcpi20TableGuid                      ## SOMETIMES_CONSUMES ## SystemTable
   gEfiAcpi10TableGuid                      ## SOMETIMES_CONSUMES ## SystemTable
+  gSmmBaseHobGuid                          ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMsegSize                         ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStmExceptionStackSize         ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
index 3cf162ada0..455fe83991 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
@@ -6,11 +6,10 @@
 
 **/
 
 #include <PiMm.h>
 #include <Library/BaseMemoryLib.h>
-#include <Library/HobLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/SmmServicesTableLib.h>
 #include <Library/TpmMeasurementLib.h>
 #include <Register/Intel/Cpuid.h>
 #include <Register/Intel/ArchitecturalMsr.h>
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
index b1f60a5505..63259e44e7 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
@@ -32,10 +32,14 @@
 [LibraryClasses]
   BaseLib
   DebugLib
   MemoryAllocationLib
   PcdLib
+  HobLib
+
+[Guids]
+  gSmmBaseHobGuid                ## CONSUMES
 
 [FixedPcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
 
 [FeaturePcd]
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index fb4a44eab6..0be869cc21 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -765,10 +765,11 @@ SmmRestoreCpu (
   SMM_S3_RESUME_STATE       *SmmS3ResumeState;
   IA32_DESCRIPTOR           Ia32Idtr;
   IA32_DESCRIPTOR           X64Idtr;
   IA32_IDT_GATE_DESCRIPTOR  IdtEntryTable[EXCEPTION_VECTOR_NUMBER];
   EFI_STATUS                Status;
+  UINTN                     Index;
 
   DEBUG ((DEBUG_INFO, "SmmRestoreCpu()\n"));
 
   mSmmS3Flag = TRUE;
 
@@ -822,13 +823,47 @@ SmmRestoreCpu (
     //
     InitializeCpuBeforeRebase ();
   }
 
   //
-  // Restore SMBASE for BSP and all APs
+  // Retrive the allocated SmmBase from gSmmBaseHobGuid
+  //
+  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
+    mSmmRelocated = TRUE;
+  } else {
+    mSmmRelocated = FALSE;
+  }
+
+  //
+  // Check whether Smm Relocation is done or not.
+  // If not, will do the SmmBases Relocation here!!!
   //
-  SmmRelocateBases ();
+  if (!mSmmRelocated) {
+    //
+    // Restore SMBASE for BSP and all APs
+    //
+    SmmRelocateBases ();
+  } else {
+    mSmmInitialized = (BOOLEAN*)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
+    ASSERT (mSmmInitialized != NULL);
+
+    mBspApicId = GetApicId ();
+
+    //
+    // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
+    //
+    SendSmiIpi (mBspApicId);
+    SendSmiIpiAllExcludingSelf ();
+
+    //
+    // Wait for all processors to finish its 1st SMI
+    //
+    for (Index = 0; Index < mNumberOfCpus; Index++) {
+      while (mSmmInitialized[Index] == FALSE) {
+      }
+    }
+  }
 
   //
   // Skip initialization if mAcpiCpuData is not valid
   //
   if (mAcpiCpuData.NumberOfCpus > 0) {
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index a0967eb69c..09bf3dfe74 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1728,10 +1728,29 @@ SmiRendezvous (
   // when using on-demand paging for above 4G memory.
   //
   Cr2 = 0;
   SaveCr2 (&Cr2);
 
+  if (mSmmRelocated && !mSmmInitialized[CpuIndex]) {
+    //
+    // Perform SmmInitHandler for CpuIndex
+    //
+    SmmInitHandler ();
+
+    //
+    // Restore Cr2
+    //
+    RestoreCr2 (Cr2);
+
+    //
+    // Mark the first SMI init for CpuIndex has been done so as to avoid the reentry.
+    //
+    mSmmInitialized[CpuIndex] = TRUE;
+
+    return;
+  }
+
   //
   // Call the user register Startup function first.
   //
   if (mSmmMpSyncData->StartupProcedure != NULL) {
     mSmmMpSyncData->StartupProcedure (mSmmMpSyncData->StartupProcArgs);
@@ -1882,13 +1901,13 @@ Exit:
   //
   RestoreCr2 (Cr2);
 }
 
 /**
-  Initialize PackageBsp Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
-  will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which
-  means not specified yet.
+  Initialize mPackageFirstThreadIndex Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
+  will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which means not
+  specified yet.
 
 **/
 VOID
 InitPackageFirstThreadIndexInfo (
   VOID
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 655175a2c6..7c624decd5 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -83,10 +83,14 @@ EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL  mSmmMemoryAttribute = {
   EdkiiSmmClearMemoryAttributes
 };
 
 EFI_CPU_INTERRUPT_HANDLER  mExternalVectorTable[EXCEPTION_VECTOR_NUMBER];
 
+BOOLEAN  mSmmRelocated    = FALSE;
+BOOLEAN  *mSmmInitialized = NULL;
+UINT32   mBspApicId       = 0;
+
 //
 // SMM stack information
 //
 UINTN  mSmmStackArrayBase;
 UINTN  mSmmStackArrayEnd;
@@ -343,27 +347,36 @@ SmmInitHandler (
   VOID
   )
 {
   UINT32  ApicId;
   UINTN   Index;
+  BOOLEAN IsMonarch;
+
+  IsMonarch = FALSE;
 
   //
   // Update SMM IDT entries' code segment and load IDT
   //
   AsmWriteIdtr (&gcSmiIdtr);
   ApicId = GetApicId ();
 
   ASSERT (mNumberOfCpus <= mMaxNumberOfCpus);
 
+  if (!mSmmRelocated) {
+    IsMonarch = mIsBsp;
+  } else if (mBspApicId == ApicId) {
+    IsMonarch = TRUE;
+  }
+
   for (Index = 0; Index < mNumberOfCpus; Index++) {
     if (ApicId == (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
       //
       // Initialize SMM specific features on the currently executing CPU
       //
       SmmCpuFeaturesInitializeProcessor (
         Index,
-        mIsBsp,
+        IsMonarch,
         gSmmCpuPrivate->ProcessorInfo,
         &mCpuHotPlugData
         );
 
       if (!mSmmS3Flag) {
@@ -371,23 +384,25 @@ SmmInitHandler (
         // Check XD and BTS features on each processor on normal boot
         //
         CheckFeatureSupported ();
       }
 
-      if (mIsBsp) {
+      if (!mSmmRelocated) {
+        if (mIsBsp) {
+          //
+          // BSP rebase is already done above.
+          // Initialize private data during S3 resume
+          //
+          InitializeMpSyncData ();
+        }
+
         //
-        // BSP rebase is already done above.
-        // Initialize private data during S3 resume
+        // Hook return after RSM to set SMM re-based flag
         //
-        InitializeMpSyncData ();
+        SemaphoreHook (Index, &mRebased[Index]);
       }
 
-      //
-      // Hook return after RSM to set SMM re-based flag
-      //
-      SemaphoreHook (Index, &mRebased[Index]);
-
       return;
     }
   }
 
   ASSERT (FALSE);
@@ -561,11 +576,15 @@ PiCpuSmmEntry (
   UINT32                    RegEcx;
   UINT32                    RegEdx;
   UINTN                     FamilyId;
   UINTN                     ModelId;
   UINT32                    Cr3;
+  EFI_HOB_GUID_TYPE         *GuidHob;
+  SMM_BASE_HOB_DATA         *SmmRelocationInfoHobData;
 
+  GuidHob                  = NULL;
+  SmmRelocationInfoHobData = NULL;
   //
   // Initialize address fixup
   //
   PiSmmCpuSmmInitFixupAddress ();
   PiSmmCpuSmiEntryFixupAddress ();
@@ -789,30 +808,51 @@ PiCpuSmmEntry (
   // context must be reduced.
   //
   ASSERT (TileSize <= (SMRAM_SAVE_STATE_MAP_OFFSET + sizeof (SMRAM_SAVE_STATE_MAP) - SMM_HANDLER_OFFSET));
 
   //
-  // Allocate buffer for all of the tiles.
+  // Check whether the Required TileSize is enough.
   //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 34.11 SMBASE Relocation
-  //   For Pentium and Intel486 processors, the SMBASE values must be
-  //   aligned on a 32-KByte boundary or the processor will enter shutdown
-  //   state during the execution of a RSM instruction.
+  if (TileSize > SIZE_8KB) {
+    DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not enough -- Required TileSize = 0x%08x, Actual TileSize = 0x%08x\n", TileSize, SIZE_8KB));
+    ASSERT (TileSize <= SIZE_8KB);
+    return RETURN_BUFFER_TOO_SMALL;
+  }
+
   //
-  // Intel486 processors: FamilyId is 4
-  // Pentium processors : FamilyId is 5
+  // Retrive the allocated SmmBase from gSmmBaseHobGuid
   //
-  BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * (mMaxNumberOfCpus - 1));
-  if ((FamilyId == 4) || (FamilyId == 5)) {
-    Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);
+  GuidHob = GetFirstGuidHob (&gSmmBaseHobGuid);
+  if (GuidHob != NULL) {
+    SmmRelocationInfoHobData = GET_GUID_HOB_DATA (GuidHob);
+
+    ASSERT (SmmRelocationInfoHobData->NumberOfProcessors == mMaxNumberOfCpus);
+    mSmmRelocated = TRUE;
   } else {
-    Buffer = AllocateAlignedCodePages (BufferPages, SIZE_4KB);
-  }
+    DEBUG ((DEBUG_INFO, "PiCpuSmmEntry: gSmmBaseHobGuid not found!\n"));
+    //
+    // Allocate buffer for all of the tiles.
+    //
+    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
+    // Volume 3C, Section 34.11 SMBASE Relocation
+    //   For Pentium and Intel486 processors, the SMBASE values must be
+    //   aligned on a 32-KByte boundary or the processor will enter shutdown
+    //   state during the execution of a RSM instruction.
+    //
+    // Intel486 processors: FamilyId is 4
+    // Pentium processors : FamilyId is 5
+    //
+    BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * (mMaxNumberOfCpus - 1));
+    if ((FamilyId == 4) || (FamilyId == 5)) {
+      Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);
+    } else {
+      Buffer = AllocateAlignedCodePages (BufferPages, SIZE_4KB);
+    }
 
-  ASSERT (Buffer != NULL);
-  DEBUG ((DEBUG_INFO, "SMRAM SaveState Buffer (0x%08x, 0x%08x)\n", Buffer, EFI_PAGES_TO_SIZE (BufferPages)));
+    ASSERT (Buffer != NULL);
+    DEBUG ((DEBUG_INFO, "New Allcoated SMRAM SaveState Buffer (0x%08x, 0x%08x)\n", Buffer, EFI_PAGES_TO_SIZE (BufferPages)));
+  }
 
   //
   // Allocate buffer for pointers to array in  SMM_CPU_PRIVATE_DATA.
   //
   gSmmCpuPrivate->ProcessorInfo = (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof (EFI_PROCESSOR_INFORMATION) * mMaxNumberOfCpus);
@@ -843,11 +883,12 @@ PiCpuSmmEntry (
   // Retrieve APIC ID of each enabled processor from the MP Services protocol.
   // Also compute the SMBASE address, CPU Save State address, and CPU Save state
   // size for each CPU in the platform
   //
   for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
-    mCpuHotPlugData.SmBase[Index]           = (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+    mCpuHotPlugData.SmBase[Index] = mSmmRelocated ? (UINTN)SmmRelocationInfoHobData->SmBase[Index] : (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+
     gSmmCpuPrivate->CpuSaveStateSize[Index] = sizeof (SMRAM_SAVE_STATE_MAP);
     gSmmCpuPrivate->CpuSaveState[Index]     = (VOID *)(mCpuHotPlugData.SmBase[Index] + SMRAM_SAVE_STATE_MAP_OFFSET);
     gSmmCpuPrivate->Operation[Index]        = SmmCpuNone;
 
     if (Index < mNumberOfCpus) {
@@ -956,21 +997,27 @@ PiCpuSmmEntry (
   // Initialize IDT
   //
   InitializeSmmIdt ();
 
   //
-  // Relocate SMM Base addresses to the ones allocated from SMRAM
+  // Check whether Smm Relocation is done or not.
+  // If not, will do the SmmBases Relocation here!!!
   //
-  mRebased = (BOOLEAN *)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
-  ASSERT (mRebased != NULL);
-  SmmRelocateBases ();
+  if (!mSmmRelocated) {
+    //
+    // Relocate SMM Base addresses to the ones allocated from SMRAM
+    //
+    mRebased = (BOOLEAN *)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
+    ASSERT (mRebased != NULL);
+    SmmRelocateBases ();
 
-  //
-  // Call hook for BSP to perform extra actions in normal mode after all
-  // SMM base addresses have been relocated on all CPUs
-  //
-  SmmCpuFeaturesSmmRelocationComplete ();
+    //
+    // Call hook for BSP to perform extra actions in normal mode after all
+    // SMM base addresses have been relocated on all CPUs
+    //
+    SmmCpuFeaturesSmmRelocationComplete ();
+  }
 
   DEBUG ((DEBUG_INFO, "mXdSupported - 0x%x\n", mXdSupported));
 
   //
   // SMM Time initialization
@@ -997,10 +1044,42 @@ PiCpuSmmEntry (
           );
       }
     }
   }
 
+  //
+  // For relocated SMBASE, some MSRs & CSRs are still required to be configured in SMM Mode for SMM Initialization.
+  // Those MSRs & CSRs must be configured before normal SMI sources happen.
+  // So, here is to issue SMI IPI (All Excluding  Self SMM IPI + BSP SMM IPI) for SMM init
+  //
+  if (mSmmRelocated) {
+    mSmmInitialized = (BOOLEAN *)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
+    ASSERT (mSmmInitialized != NULL);
+
+    mBspApicId = GetApicId ();
+
+    //
+    // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
+    //
+    SendSmiIpi (mBspApicId);
+    SendSmiIpiAllExcludingSelf ();
+
+    //
+    // Wait for all processors to finish its 1st SMI
+    //
+    for (Index = 0; Index < mNumberOfCpus; Index++) {
+      while (mSmmInitialized[Index] == FALSE) {
+      }
+    }
+
+    //
+    // Call hook for BSP to perform extra actions in normal mode after all
+    // SMM base addresses have been relocated on all CPUs
+    //
+    SmmCpuFeaturesSmmRelocationComplete ();
+  }
+
   //
   // Fill in SMM Reserved Regions
   //
   gSmmCpuPrivate->SmmReservedSmramRegion[0].SmramReservedStart = 0;
   gSmmCpuPrivate->SmmReservedSmramRegion[0].SmramReservedSize  = 0;
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 5f0a38e400..78a88e6f3c 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -23,10 +23,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Protocol/MmMp.h>
 
 #include <Guid/AcpiS3Context.h>
 #include <Guid/MemoryAttributesTable.h>
 #include <Guid/PiSmmMemoryAttributesTable.h>
+#include <Guid/SmmBaseHob.h>
 
 #include <Library/BaseLib.h>
 #include <Library/IoLib.h>
 #include <Library/TimerLib.h>
 #include <Library/SynchronizationLib.h>
@@ -346,10 +347,20 @@ SmmWriteSaveState (
   IN EFI_SMM_SAVE_STATE_REGISTER  Register,
   IN UINTN                        CpuIndex,
   IN CONST VOID                   *Buffer
   );
 
+/**
+  C function for SMI handler. To change all processor's SMMBase Register.
+
+**/
+VOID
+EFIAPI
+SmmInitHandler (
+  VOID
+);
+
 /**
 Read a CPU Save State register on the target processor.
 
 This function abstracts the differences that whether the CPU Save State register is in the
 IA32 CPU Save State Map or X64 CPU Save State Map.
@@ -400,10 +411,14 @@ WriteSaveStateRegister (
   IN EFI_SMM_SAVE_STATE_REGISTER  Register,
   IN UINTN                        Width,
   IN CONST VOID                   *Buffer
   );
 
+extern BOOLEAN  mSmmRelocated;
+extern BOOLEAN  *mSmmInitialized;
+extern UINT32   mBspApicId;
+
 extern CONST UINT8        gcSmmInitTemplate[];
 extern CONST UINT16       gcSmmInitSize;
 X86_ASSEMBLY_PATCH_LABEL  gPatchSmmCr0;
 extern UINT32             mSmmCr0;
 X86_ASSEMBLY_PATCH_LABEL  gPatchSmmCr3;
@@ -1486,13 +1501,13 @@ RegisterStartupProcedure (
   IN     EFI_AP_PROCEDURE  Procedure,
   IN OUT VOID              *ProcedureArguments OPTIONAL
   );
 
 /**
-  Initialize PackageBsp Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
-  will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which
-  means not specified yet.
+  Initialize mPackageFirstThreadIndex Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
+  will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which means not
+  specified yet.
 
 **/
 VOID
 InitPackageFirstThreadIndexInfo (
   VOID
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index b4b327f60c..6dbed17b96 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -112,10 +112,11 @@
 
 [Guids]
   gEfiAcpiVariableGuid                     ## SOMETIMES_CONSUMES ## HOB # it is used for S3 boot.
   gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
   gEfiMemoryAttributesTableGuid            ## CONSUMES ## SystemTable
+  gSmmBaseHobGuid                          ## CONSUMES
 
 [FeaturePcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp            ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection             ## CONSUMES
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index cff239d528..2afd08cdd2 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -76,10 +76,13 @@
   gEdkiiCpuFeaturesInitDoneGuid  = { 0xc77c3a41, 0x61ab, 0x4143, { 0x98, 0x3e, 0x33, 0x39, 0x28, 0x6, 0x28, 0xe5 }}
 
   ## Include/Guid/MicrocodePatchHob.h
   gEdkiiMicrocodePatchHobGuid    = { 0xd178f11d, 0x8716, 0x418e, { 0xa1, 0x31, 0x96, 0x7d, 0x2a, 0xc4, 0x28, 0x43 }}
 
+  ## Include/Guid/SmmBaseHob.h
+  gSmmBaseHobGuid      = { 0xc2217ba7, 0x03bb, 0x4f63, {0xa6, 0x47, 0x7c, 0x25, 0xc5, 0xfc, 0x9d, 0x73 }}
+
 [Protocols]
   ## Include/Protocol/SmmCpuService.h
   gEfiSmmCpuServiceProtocolGuid   = { 0x1d202cab, 0xc8ab, 0x4d5c, { 0x94, 0xf7, 0x3c, 0xfc, 0xc0, 0xd3, 0xd3, 0x35 }}
   gEdkiiSmmCpuRendezvousProtocolGuid = { 0xaa00d50b, 0x4911, 0x428f, { 0xb9, 0x1a, 0xa5, 0x9d, 0xdb, 0x13, 0xe2, 0x4c }}
 
-- 
2.16.2.windows.1

Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

[Laszlo Ersek]

On 1/11/23 09:35, Wu, Jiaxin wrote:
> Mainly changes as below:
> 1. Add Smm Base HOB, which is used to store the information of
> Smm Relocated SmBase array for each Processors;
> 2. Combine 2 SMIs (gcSmmInitTemplate & gcSmiHandlerTemplate) into one
> (gcSmiHandlerTemplate), the new SMI handler needs to run to 2 paths: one
> to SmmCpuFeaturesInitializeProcessor(), the other to SMM Core Entry Point.
> 3. Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
> before normal SMI sources happen.
> 4. Call SmmCpuFeaturesInitializeProcessor() in parallel.
> 
> v2:
> - Refine the coding style
> - Rename hob to gSmmBaseHobGuid
> - Update SmmInitHandler() to handle the SMM relocation
> - Correct the S3 for SMM relocation
> 
> v1:
> - Thread: https://edk2.groups.io/g/devel/message/97748
> 
> Change-Id: Iec7bf25166bfeefb44a202285465a35b5debbce4

(1) Please don't include this in upstream patches.

> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>

(2) This patch is for UefiCpuPkg, but Gerd has not been CC'd, as far as
I can tell. CC'ing him now. (Please refer to commit 0aca5901e344,
"Maintainers.txt: designate Gerd Hoffmann as UefiCpuPkg reviewer",
2023-01-06).

> ---
>  UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  36 +++++
>  .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
>  .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  24 +++-
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
>  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
>  .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  39 +++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  25 +++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 149 ++++++++++++++++-----
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  21 ++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
>  UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
>  13 files changed, 261 insertions(+), 49 deletions(-)
>  create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h

(3) The patch extends the interface between PiSmmCpuDxeSmm and multipe
SmmCpuFeaturesLib instances. There is no concise and complete design
description, either in the commit message, or in a TianoCore bugzilla
ticket (no reference in your commit message), or in the new header file
"SmmBaseHob.h".

(4) The commit modifies multiple modules at once. In a producer-consumer
scenario (which is usually characteristic of HOBs), we tend to extend
the producer first (if there are multiple producers, then each in
separation), and then the consumers. Usually the consumers are expected
to keep compatibility with the lack of the HOB, if possible.

The compatibility aspects are not explained, and the modifications are
squashed together in a single patch. Unacceptable.

(5) OvmfPkg includes its own SmmCpuFeaturesLib instance, but there's not
a peep about OvmfPkg in the patch (code or commit message), not even an
explanation why OvmfPkg is supposed to be unaffected. Unacceptable.

Nacked-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

[Laszlo Ersek]

On 1/12/23 13:37, Laszlo Ersek wrote:
> On 1/11/23 09:35, Wu, Jiaxin wrote:
>> Mainly changes as below:
>> 1. Add Smm Base HOB, which is used to store the information of
>> Smm Relocated SmBase array for each Processors;
>> 2. Combine 2 SMIs (gcSmmInitTemplate & gcSmiHandlerTemplate) into one
>> (gcSmiHandlerTemplate), the new SMI handler needs to run to 2 paths: one
>> to SmmCpuFeaturesInitializeProcessor(), the other to SMM Core Entry Point.
>> 3. Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
>> before normal SMI sources happen.
>> 4. Call SmmCpuFeaturesInitializeProcessor() in parallel.
>>
>> v2:
>> - Refine the coding style
>> - Rename hob to gSmmBaseHobGuid
>> - Update SmmInitHandler() to handle the SMM relocation
>> - Correct the S3 for SMM relocation
>>
>> v1:
>> - Thread: https://edk2.groups.io/g/devel/message/97748
>>
>> Change-Id: Iec7bf25166bfeefb44a202285465a35b5debbce4
> 
> (1) Please don't include this in upstream patches.
> 
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Zeng Star <star.zeng@intel.com>
>> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> 
> (2) This patch is for UefiCpuPkg, but Gerd has not been CC'd, as far as
> I can tell. CC'ing him now. (Please refer to commit 0aca5901e344,
> "Maintainers.txt: designate Gerd Hoffmann as UefiCpuPkg reviewer",
> 2023-01-06).
> 
>> ---
>>  UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  36 +++++
>>  .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
>>  .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  24 +++-
>>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
>>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
>>  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
>>  .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
>>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  39 +++++-
>>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  25 +++-
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 149 ++++++++++++++++-----
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  21 ++-
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
>>  UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
>>  13 files changed, 261 insertions(+), 49 deletions(-)
>>  create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h
> 
> (3) The patch extends the interface between PiSmmCpuDxeSmm and multipe
> SmmCpuFeaturesLib instances. There is no concise and complete design
> description, either in the commit message, or in a TianoCore bugzilla
> ticket (no reference in your commit message), or in the new header file
> "SmmBaseHob.h".
> 
> (4) The commit modifies multiple modules at once. In a producer-consumer
> scenario (which is usually characteristic of HOBs), we tend to extend
> the producer first (if there are multiple producers, then each in
> separation), and then the consumers. Usually the consumers are expected
> to keep compatibility with the lack of the HOB, if possible.
> 
> The compatibility aspects are not explained, and the modifications are
> squashed together in a single patch. Unacceptable.
> 
> (5) OvmfPkg includes its own SmmCpuFeaturesLib instance, but there's not
> a peep about OvmfPkg in the patch (code or commit message), not even an
> explanation why OvmfPkg is supposed to be unaffected. Unacceptable.
> 
> Nacked-by: Laszlo Ersek <lersek@redhat.com>
> 

(6) BTW, does this patch conflict with (or at least require coordination
with):

[edk2-devel] [PATCH v2 0/6] Adds AmdSmmCpuFeaturesLib
https://edk2.groups.io/g/devel/message/98270

?

Cc'ing Abdul.

Laszlo

Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

[Wu, Jiaxin]

> >> v1:
> >> - Thread: https://edk2.groups.io/g/devel/message/97748
> >>
> >> Change-Id: Iec7bf25166bfeefb44a202285465a35b5debbce4
> >
> > (1) Please don't include this in upstream patches.
> >


I will resubmit the series patches according your below comments.

> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Ray Ni <ray.ni@intel.com>
> >> Cc: Zeng Star <star.zeng@intel.com>
> >> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> >
> > (2) This patch is for UefiCpuPkg, but Gerd has not been CC'd, as far as
> > I can tell. CC'ing him now. (Please refer to commit 0aca5901e344,
> > "Maintainers.txt: designate Gerd Hoffmann as UefiCpuPkg reviewer",
> > 2023-01-06).
> >
> >> ---
> >>  UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  36 +++++
> >>  .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
> >>  .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  24 +++-
> >>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
> >>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
> >>  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
> >>  .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
> >>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  39 +++++-
> >>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  25 +++-
> >>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 149
> ++++++++++++++++-----
> >>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  21 ++-
> >>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
> >>  UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
> >>  13 files changed, 261 insertions(+), 49 deletions(-)
> >>  create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h
> >
> > (3) The patch extends the interface between PiSmmCpuDxeSmm and
> multipe
> > SmmCpuFeaturesLib instances. There is no concise and complete design
> > description, either in the commit message, or in a TianoCore bugzilla
> > ticket (no reference in your commit message), or in the new header file
> > "SmmBaseHob.h".
> >

Agree, thanks Laszlo, I will add more description to explain the hob usage and design detailed.


> > (4) The commit modifies multiple modules at once. In a producer-consumer
> > scenario (which is usually characteristic of HOBs), we tend to extend
> > the producer first (if there are multiple producers, then each in
> > separation), and then the consumers. Usually the consumers are expected
> > to keep compatibility with the lack of the HOB, if possible.
> >
> > The compatibility aspects are not explained, and the modifications are
> > squashed together in a single patch. Unacceptable.

Agree, I will separate the patch to the new series patches for the producer-consumer scenario.


> >
> > (5) OvmfPkg includes its own SmmCpuFeaturesLib instance, but there's not
> > a peep about OvmfPkg in the patch (code or commit message), not even
> an
> > explanation why OvmfPkg is supposed to be unaffected. Unacceptable.
> >
> > Nacked-by: Laszlo Ersek <lersek@redhat.com>
> >

Good catch for the missed lib instance. Thanks Laszlo. 

> 
> (6) BTW, does this patch conflict with (or at least require coordination
> with):
> 
> [edk2-devel] [PATCH v2 0/6] Adds AmdSmmCpuFeaturesLib
> https://edk2.groups.io/g/devel/message/98270
> 
> ?
> 

That's depends on which patch merged first, then the later patch need consider the case as the new design.

> Cc'ing Abdul.
> 
> Laszlo

Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

[Wu, Jiaxin]

Drop this patch and refine the format according Laszlo's comment.

Please help review the series patches @ 
https://edk2.groups.io/g/devel/message/98446
https://edk2.groups.io/g/devel/message/98447
https://edk2.groups.io/g/devel/message/98448
https://edk2.groups.io/g/devel/message/98449
https://edk2.groups.io/g/devel/message/98450

Thanks,
Jiaxin


> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Friday, January 13, 2023 11:05 AM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Abdul Lateef
> Attar <abdattar@amd.com>
> Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg: Support SMM Relocated
> SmBase handling
> 
> > >> v1:
> > >> - Thread: https://edk2.groups.io/g/devel/message/97748
> > >>
> > >> Change-Id: Iec7bf25166bfeefb44a202285465a35b5debbce4
> > >
> > > (1) Please don't include this in upstream patches.
> > >
> 
> 
> I will resubmit the series patches according your below comments.
> 
> > >> Cc: Eric Dong <eric.dong@intel.com>
> > >> Cc: Ray Ni <ray.ni@intel.com>
> > >> Cc: Zeng Star <star.zeng@intel.com>
> > >> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> > >
> > > (2) This patch is for UefiCpuPkg, but Gerd has not been CC'd, as far as
> > > I can tell. CC'ing him now. (Please refer to commit 0aca5901e344,
> > > "Maintainers.txt: designate Gerd Hoffmann as UefiCpuPkg reviewer",
> > > 2023-01-06).
> > >
> > >> ---
> > >>  UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  36 +++++
> > >>  .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
> > >>  .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  24 +++-
> > >>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
> > >>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
> > >>  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
> > >>  .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
> > >>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  39 +++++-
> > >>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  25 +++-
> > >>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 149
> > ++++++++++++++++-----
> > >>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  21 ++-
> > >>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
> > >>  UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
> > >>  13 files changed, 261 insertions(+), 49 deletions(-)
> > >>  create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h
> > >
> > > (3) The patch extends the interface between PiSmmCpuDxeSmm and
> > multipe
> > > SmmCpuFeaturesLib instances. There is no concise and complete design
> > > description, either in the commit message, or in a TianoCore bugzilla
> > > ticket (no reference in your commit message), or in the new header file
> > > "SmmBaseHob.h".
> > >
> 
> Agree, thanks Laszlo, I will add more description to explain the hob usage and
> design detailed.
> 
> 
> > > (4) The commit modifies multiple modules at once. In a producer-
> consumer
> > > scenario (which is usually characteristic of HOBs), we tend to extend
> > > the producer first (if there are multiple producers, then each in
> > > separation), and then the consumers. Usually the consumers are
> expected
> > > to keep compatibility with the lack of the HOB, if possible.
> > >
> > > The compatibility aspects are not explained, and the modifications are
> > > squashed together in a single patch. Unacceptable.
> 
> Agree, I will separate the patch to the new series patches for the producer-
> consumer scenario.
> 
> 
> > >
> > > (5) OvmfPkg includes its own SmmCpuFeaturesLib instance, but there's
> not
> > > a peep about OvmfPkg in the patch (code or commit message), not even
> > an
> > > explanation why OvmfPkg is supposed to be unaffected. Unacceptable.
> > >
> > > Nacked-by: Laszlo Ersek <lersek@redhat.com>
> > >
> 
> Good catch for the missed lib instance. Thanks Laszlo.
> 
> >
> > (6) BTW, does this patch conflict with (or at least require coordination
> > with):
> >
> > [edk2-devel] [PATCH v2 0/6] Adds AmdSmmCpuFeaturesLib
> > https://edk2.groups.io/g/devel/message/98270
> >
> > ?
> >
> 
> That's depends on which patch merged first, then the later patch need
> consider the case as the new design.
> 
> > Cc'ing Abdul.
> >
> > Laszlo

Re: [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

[Ni, Ray]

Jiaxin,
I know that you will submit v3 patch set to address Laszlo's comments.
Let me put my review comments on code logic here so you could take them into account also.


1. I suggest we describe the HOB structure in this header file in file header comments.
For example:

The default SMBASE for the x86 processor is 0x30000. When SMI happens, CPU runs the
SMI handler at SMBASE+0x8000. Also, the SMM save state area is within SMBASE+0x10000.

One of the SMM initialization from CPU perspective is to program the new SMBASE (in TSEG range)
for each CPU thread. When the SMBASE update happens in a PEI module, the PEI module shall
produce the SMM_BASE_HOB in HOB database which tells the PiSmmCpuDxeSmm driver which runs
at a later phase about the new SMBASE for each CPU thread. PiSmmCpuDxeSmm driver installs
the SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000 for CPU thread Index.
When the HOB doesn't exist, PiSmmCpuDxeSmm driver shall program the new SMBASE itself.

> +
> +#pragma pack(1)
> +typedef struct {
> +  ///
> +  /// Describes the Number of all max supported processors.
> +  ///
> +  UINT64    NumberOfProcessors;
> +  ///
> +  /// Pointer to SmBase array for each Processors.
> +  ///
> +  UINT64    SmBase[];
> +} SMM_BASE_HOB_DATA;
> +#pragma pack()
> +
> +extern EFI_GUID  gSmmBaseHobGuid;
> +


> +  //
> +  // If gSmmBaseHobGuid found, means Smm Relocation has been done.
> +  //
> +  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
> +    mSmmRelocationDone = TRUE;
> +  }

2. Can you write the code as "mSmmRelocationDone = (BOOLEAN) (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL)"?
    It removes the assumption that the initial value of mSmmRelocationDone is FALSE.
    I understand it's TRUE usually. But it depends on the PE loader.

> +  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
> +    mSmmRelocated = TRUE;
> +  } else {
> +    mSmmRelocated = FALSE;

3. The above code doesn't assume the initial value of global variable. Good.
    Can you align the variable name between SmmCpuFeaturesLib and the PiSmmCpuDxeSmm driver?
    If the two names are chosen to fix link error, there are two ways to avoid the error:
    a. Add "static" and both component use mSmmRelocated.
    b. One can be renamed as mSmmCpuFeaturesSmmRelocated. No change to the one in driver.

> +  }
> +
> +  //
> +  // Check whether Smm Relocation is done or not.
> +  // If not, will do the SmmBases Relocation here!!!
>    //
> -  SmmRelocateBases ();
> +  if (!mSmmRelocated) {
> +    //
> +    // Restore SMBASE for BSP and all APs
> +    //
> +    SmmRelocateBases ();
> +  } else {
> +    mSmmInitialized = (BOOLEAN*)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
4. I guess mSmmInitialized is already allocated in normal boot phase. Here what we need is only to set all
elements to FALSE. Will keep reviewing following changes and confirm my guess.
    But we still cannot call Allocate in every S3 boot without freeing. It may cause all SMRAM be allocated.


> +    ASSERT (mSmmInitialized != NULL);
> +
> +    mBspApicId = GetApicId ();
> +
> +    //
> +    // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
> +    //
> +    SendSmiIpi (mBspApicId);
> +    SendSmiIpiAllExcludingSelf ();
> +
> +    //
> +    // Wait for all processors to finish its 1st SMI
> +    //
> +    for (Index = 0; Index < mNumberOfCpus; Index++) {
> +      while (mSmmInitialized[Index] == FALSE) {
> +      }
> +    }
5. I am not sure if the same logic is also needed in normal boot. So, maybe a local function can be created to
reduce the code duplication?


> +  if (!mSmmRelocated) {
> +    IsMonarch = mIsBsp;
> +  } else if (mBspApicId == ApicId) {
> +    IsMonarch = TRUE;
> +  }

6. How about removing mIsBsp and always use mBspApicId even when mSmmRelocated==FALSE?
7. How about renaming IsMonarch to IsBsp?

> 
> -      if (mIsBsp) {
> +      if (!mSmmRelocated) {
> +        if (mIsBsp) {
> +          //
> +          // BSP rebase is already done above.
> +          // Initialize private data during S3 resume
> +          //
> +          InitializeMpSyncData ();
8. Because the same function is already called in driver entrypoint, here the call is for s3 path.
     Can you check if we need to call it as well in S3 path when SmmRelocated is TRUE?

> +  if (TileSize > SIZE_8KB) {
> +    DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not enough -- Required TileSize = 0x%08x, Actual TileSize
> = 0x%08x\n", TileSize, SIZE_8KB));
> +    ASSERT (TileSize <= SIZE_8KB);

9. I suggest we add CpuDeadLoop() here to capture the error in release build.

Re: [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

[Wu, Jiaxin]

Thanks ray, below are really great feedback. I have checked all and response as below:

> The default SMBASE for the x86 processor is 0x30000. When SMI happens,
> CPU runs the
> SMI handler at SMBASE+0x8000. Also, the SMM save state area is within
> SMBASE+0x10000.
> 
> One of the SMM initialization from CPU perspective is to program the new
> SMBASE (in TSEG range)
> for each CPU thread. When the SMBASE update happens in a PEI module,
> the PEI module shall
> produce the SMM_BASE_HOB in HOB database which tells the
> PiSmmCpuDxeSmm driver which runs
> at a later phase about the new SMBASE for each CPU thread.
> PiSmmCpuDxeSmm driver installs
> the SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000 for CPU
> thread Index.
> When the HOB doesn't exist, PiSmmCpuDxeSmm driver shall program the
> new SMBASE itself.
> 
Agree, added!

> 
> 2. Can you write the code as "mSmmRelocationDone = (BOOLEAN)
> (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL)"?
>     It removes the assumption that the initial value of mSmmRelocationDone is
> FALSE.
>     I understand it's TRUE usually. But it depends on the PE loader.

Agree.

> 
> > +  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
> > +    mSmmRelocated = TRUE;
> > +  } else {
> > +    mSmmRelocated = FALSE;
> 
> 3. The above code doesn't assume the initial value of global variable. Good.
>     Can you align the variable name between SmmCpuFeaturesLib and the
> PiSmmCpuDxeSmm driver?
>     If the two names are chosen to fix link error, there are two ways to avoid
> the error:
>     a. Add "static" and both component use mSmmRelocated.
>     b. One can be renamed as mSmmCpuFeaturesSmmRelocated. No change
> to the one in driver.
> 

Agree. 

> > +  }
> > +
> > +  //
> > +  // Check whether Smm Relocation is done or not.
> > +  // If not, will do the SmmBases Relocation here!!!
> >    //
> > -  SmmRelocateBases ();
> > +  if (!mSmmRelocated) {
> > +    //
> > +    // Restore SMBASE for BSP and all APs
> > +    //
> > +    SmmRelocateBases ();
> > +  } else {
> > +    mSmmInitialized = (BOOLEAN*)AllocateZeroPool (sizeof (BOOLEAN) *
> mMaxNumberOfCpus);
> 4. I guess mSmmInitialized is already allocated in normal boot phase. Here
> what we need is only to set all
> elements to FALSE. Will keep reviewing following changes and confirm my
> guess.
>     But we still cannot call Allocate in every S3 boot without freeing. It may
> cause all SMRAM be allocated.
> 

Yes, I checked the detailed, we don't need allocate that.


> 
> > +    ASSERT (mSmmInitialized != NULL);
> > +
> > +    mBspApicId = GetApicId ();
> > +
> > +    //
> > +    // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
> > +    //
> > +    SendSmiIpi (mBspApicId);
> > +    SendSmiIpiAllExcludingSelf ();
> > +
> > +    //
> > +    // Wait for all processors to finish its 1st SMI
> > +    //
> > +    for (Index = 0; Index < mNumberOfCpus; Index++) {
> > +      while (mSmmInitialized[Index] == FALSE) {
> > +      }
> > +    }
> 5. I am not sure if the same logic is also needed in normal boot. So, maybe a
> local function can be created to
> reduce the code duplication?

Ok, will define new API to reduce the duplication.

> 
> 
> > +  if (!mSmmRelocated) {
> > +    IsMonarch = mIsBsp;
> > +  } else if (mBspApicId == ApicId) {
> > +    IsMonarch = TRUE;
> > +  }
> 
> 6. How about removing mIsBsp and always use mBspApicId even when
> mSmmRelocated==FALSE?
> 7. How about renaming IsMonarch to IsBsp?

Agree.

> 
> >
> > -      if (mIsBsp) {
> > +      if (!mSmmRelocated) {
> > +        if (mIsBsp) {
> > +          //
> > +          // BSP rebase is already done above.
> > +          // Initialize private data during S3 resume
> > +          //
> > +          InitializeMpSyncData ();
> 8. Because the same function is already called in driver entrypoint, here the
> call is for s3 path.
>      Can you check if we need to call it as well in S3 path when SmmRelocated is
> TRUE?

Yes, we need that in S3, will handle it.

> 
> > +  if (TileSize > SIZE_8KB) {
> > +    DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not
> enough -- Required TileSize = 0x%08x, Actual TileSize
> > = 0x%08x\n", TileSize, SIZE_8KB));
> > +    ASSERT (TileSize <= SIZE_8KB);
> 
> 9. I suggest we add CpuDeadLoop() here to capture the error in release build.

Agree.