[PATCH v1 0/4] Support SMM Relocated SmBase handling

[PATCH v1 0/4] Support SMM Relocated SmBase handling

[Wu, Jiaxin]

Below serial patches are to support the SMM Relocated SmBase handling.
To achieve, new hob interface is procuded, and will be consumed by SMM
CPU driver & SmmCpuFeaturesLib to do SmBase initialization:

The Smm Base HOB is used to store the relocated SmBase in
array for each Processors. If gSmmBaseHobGuid produced,
indicate SmBase for each Processors have been relocated.
The SmBase address in hob can be guaranteed the SMRAM state
save areas for all processors do not overlap.

SMM CPU driver will retrieve the SMBASE addresses from SMM Base Hob
and installs the SMI handler at [SMBASE+8000h] for each processor
instead of relocating SMM Base addresses from SMRAM again.

With SMM Base Hob, SMM CPU driver does not need the RSM instruction
to reload the SMBASE register with the new allocated value in SMBASE
field each time it exits SMM. SMBASE Register for each processors
have already been programmed in parallel since the same default
SMBASE Address(0x30000) is not used, thus the CPUs over-writing
each other's SMM Save State Area will not happen. This way will save
boot time on multi-core system.

Beside, If gSmmBaseHobGuid found, no need to do the relocation in
SmmCpuFeaturesInitializeProcessor().

Jiaxin Wu (4):
  UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data
  UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info
  UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE
  OvmfPkg/SmmCpuFeaturesLib: Skip to configure SMBASE

 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  39 ++++--
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
 UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  51 +++++++
 .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
 .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  25 +++-
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
 UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
 .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  40 +++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  25 +++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 155 ++++++++++++++++-----
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  21 ++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
 UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
 15 files changed, 318 insertions(+), 58 deletions(-)
 create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h

-- 
2.16.2.windows.1

[PATCH v1 1/4] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Wu, Jiaxin]

The Smm Base HOB is used to store the relocated SmBase in
array for each Processors. If gSmmBaseHobGuid produced,
indicate SmBase for each Processors have been relocated.
The SmBase address in hob can be guaranteed the SMRAM state
save areas for all processors do not overlap.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/Include/Guid/SmmBaseHob.h | 51 ++++++++++++++++++++++++++++++++++++
 UefiCpuPkg/UefiCpuPkg.dec            |  3 +++
 2 files changed, 54 insertions(+)
 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..090b22a274
--- /dev/null
+++ b/UefiCpuPkg/Include/Guid/SmmBaseHob.h
@@ -0,0 +1,51 @@
+/** @file
+  The Smm Base HOB is used to store the information of:
+  * The relocated SmBase in array for each Processors.
+
+  If gSmmBaseHobGuid produced, indicate SmBase for each Processors
+  have been relocated and SmBase in HOB can be guaranteed the SMRAM
+  state save areas for all processors do not overlap. SMM CPU driver
+  should retrieve the SMBASE addresses from this HOB and installs the
+  SMI handler at [SMBASE+8000h] for each processor instead of relocating
+  SMM Base addresses from SMRAM again.
+
+  With SMM Base Hob, SMM CPU driver doesn't need the RSM instruction
+  to reload the SMBASE register with the new allocated value in SMBASE
+  field each time it exits SMM. SMBASE Register for each processors
+  have already been programmed in parallel since the same default
+  SMBASE Address (0x30000) is not used, thus the CPUs over-writing
+  each other's SMM Save State Area will not happen. This way will save
+  boot time on multi-core system.
+
+  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 address for each Processors.
+  ///
+  UINT64    SmBase[];
+} SMM_BASE_HOB_DATA;
+#pragma pack()
+
+extern EFI_GUID  gSmmBaseHobGuid;
+
+#endif
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

[PATCH v1 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Wu, Jiaxin]

SMM CPU driver will retrieve the SMBASE addresses from SMM Base Hob
and installs the SMI handler at [SMBASE+8000h] for each processor
instead of relocating SMM Base addresses from SMRAM again.

With SMM Base Hob, SMM CPU driver does not need the RSM instruction
to reload the SMBASE register with the new allocated value in SMBASE
field each time it exits SMM. SMBASE Register for each processors
have already been programmed in parallel since the same default
SMBASE Address(0x30000) is not used, thus the CPUs over-writing
each other's SMM Save State Area will not happen. This way will save
boot time on multi-core system.

Mainly changes as below:
*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.
*Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM
init before normal SMI sources happen.
*Call SmmCpuFeaturesInitializeProcessor() in parallel.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c            |  40 ++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        |  25 ++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 155 ++++++++++++++++++++-------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  21 +++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   1 +
 5 files changed, 197 insertions(+), 45 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index fb4a44eab6..f7479fc74d 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,48 @@ SmmRestoreCpu (
     //
     InitializeCpuBeforeRebase ();
   }
 
   //
-  // Restore SMBASE for BSP and all APs
+  // Retrive the allocated SmmBase from gSmmBaseHobGuid. If found,
+  // means the SmBase relocation has been done.
+  //
+  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
+    mSmBaseRelocationDone = TRUE;
+  } else {
+    mSmBaseRelocationDone = FALSE;
+  }
+
+  //
+  // Check whether Smm Relocation is done or not.
+  // If not, will do the SmmBases Relocation here!!!
   //
-  SmmRelocateBases ();
+  if (!mSmBaseRelocationDone) {
+    //
+    // 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..b4339eef48 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 (mSmBaseRelocationDone && !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..aa1700ba58 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  mSmBaseRelocationDone    = FALSE;
+BOOLEAN  *mSmmInitialized = NULL;
+UINT32   mBspApicId       = 0;
+
 //
 // SMM stack information
 //
 UINTN  mSmmStackArrayBase;
 UINTN  mSmmStackArrayEnd;
@@ -341,29 +345,38 @@ VOID
 EFIAPI
 SmmInitHandler (
   VOID
   )
 {
-  UINT32  ApicId;
-  UINTN   Index;
+  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 (!mSmBaseRelocationDone) {
+    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 (!mSmBaseRelocationDone) {
+        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,10 +576,15 @@ PiCpuSmmEntry (
   UINT32                    RegEcx;
   UINT32                    RegEdx;
   UINTN                     FamilyId;
   UINTN                     ModelId;
   UINT32                    Cr3;
+  EFI_HOB_GUID_TYPE         *GuidHob;
+  SMM_BASE_HOB_DATA         *SmmBaseHobData;
+
+  GuidHob                  = NULL;
+  SmmBaseHobData           = NULL;
 
   //
   // Initialize address fixup
   //
   PiSmmCpuSmmInitFixupAddress ();
@@ -789,30 +809,52 @@ 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. If found,
+  // means the SmBase relocation has been done.
   //
-  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) {
+    SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
+
+    ASSERT (SmmBaseHobData->NumberOfProcessors == mMaxNumberOfCpus);
+    mSmBaseRelocationDone = 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 +885,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] = mSmBaseRelocationDone ? (UINTN)SmmBaseHobData->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 +999,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 (!mSmBaseRelocationDone) {
+    //
+    // 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 +1046,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 (mSmBaseRelocationDone) {
+    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..da6804c58b 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  mSmBaseRelocationDone;
+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
-- 
2.16.2.windows.1

[PATCH v1 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE

[Wu, Jiaxin]

This patch is to aviod configure SMBASE if SmBase relocation has been
done. If gSmmBaseHobGuid found, means SmBase info has been relocated
and recorded in the SmBase array. No need to do the relocation in
SmmCpuFeaturesInitializeProcessor().

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |  2 ++
 .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     | 25 +++++++++++++++++++---
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  4 ++++
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |  1 +
 UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |  1 -
 .../StandaloneMmCpuFeaturesLib.inf                 |  4 ++++
 6 files changed, 33 insertions(+), 4 deletions(-)

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..c82bf9ad37 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
@@ -36,10 +36,16 @@ 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 SmBase for each Processors has been relocated or not. If TRUE,
+// means no need to do the relocation in SmmCpuFeaturesInitializeProcessor().
+//
+BOOLEAN  mSmBaseRelocated;
+
 //
 // Set default value to assume MTRRs need to be configured on each SMI
 //
 BOOLEAN  mNeedConfigureMtrrs = TRUE;
 
@@ -142,10 +148,18 @@ CpuFeaturesLibInitialization (
   //
   // Allocate array for state of SMRR enable on all CPUs
   //
   mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * GetCpuMaxLogicalProcessorNumber ());
   ASSERT (mSmrrEnabled != NULL);
+
+  //
+  // If gSmmBaseHobGuid found, means SmBase info has been relocated and recorded
+  // in the SmBase array.
+  //
+  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
+    mSmBaseRelocated = TRUE;
+  }
 }
 
 /**
   Called during the very first SMI into System Management Mode to initialize
   CPU features, including SMBASE, for the currently executing CPU.  Since this
@@ -185,14 +199,19 @@ SmmCpuFeaturesInitializeProcessor (
   UINT32                RegEdx;
   UINTN                 FamilyId;
   UINTN                 ModelId;
 
   //
-  // Configure SMBASE.
+  // No need to configure SMBASE if SmBase relocation has been done.
   //
-  CpuState             = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
-  CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+  if (!mSmBaseRelocated) {
+    //
+    // 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]
-- 
2.16.2.windows.1

[PATCH v1 4/4] OvmfPkg/SmmCpuFeaturesLib: Skip to configure SMBASE

[Wu, Jiaxin]

This patch is to aviod configure SMBASE if SmBase relocation has been
done. If gSmmBaseHobGuid found, means SmBase info has been relocated
and recorded in the SmBase array. No need to do the relocation in
SmmCpuFeaturesInitializeProcessor().

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  | 39 +++++++++++++++++-----
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  4 +++
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 6693666d04..00c84877ed 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -15,20 +15,28 @@
 #include <Library/PcdLib.h>
 #include <Library/SafeIntLib.h>
 #include <Library/SmmCpuFeaturesLib.h>
 #include <Library/SmmServicesTableLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/HobLib.h>
 #include <Pcd/CpuHotEjectData.h>
 #include <PiSmm.h>
 #include <Register/Intel/SmramSaveStateMap.h>
 #include <Register/QemuSmramSaveStateMap.h>
+#include <Guid/SmmBaseHob.h>
 
 //
 // EFER register LMA bit
 //
 #define LMA  BIT10
 
+//
+// Indicate SmBase for each Processors has been relocated or not. If TRUE,
+// means no need to do the relocation in SmmCpuFeaturesInitializeProcessor().
+//
+BOOLEAN  mSmBaseRelocated;
+
 /**
   The constructor function
 
   @param[in]  ImageHandle  The firmware allocated handle for the EFI image.
   @param[in]  SystemTable  A pointer to the EFI System Table.
@@ -41,10 +49,18 @@ EFIAPI
 SmmCpuFeaturesLibConstructor (
   IN EFI_HANDLE        ImageHandle,
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
+  //
+  // If gSmmBaseHobGuid found, means SmBase info has been relocated and recorded
+  // in the SmBase array.
+  //
+  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
+    mSmBaseRelocated = TRUE;
+  }
+
   //
   // No need to program SMRRs on our virtual platform.
   //
   return EFI_SUCCESS;
 }
@@ -83,20 +99,25 @@ SmmCpuFeaturesInitializeProcessor (
   )
 {
   QEMU_SMRAM_SAVE_STATE_MAP  *CpuState;
 
   //
-  // Configure SMBASE.
+  // No need to configure SMBASE if SmBase relocation has been done.
   //
-  CpuState = (QEMU_SMRAM_SAVE_STATE_MAP *)(UINTN)(
-                                                  SMM_DEFAULT_SMBASE +
-                                                  SMRAM_SAVE_STATE_MAP_OFFSET
-                                                  );
-  if ((CpuState->x86.SMMRevId & 0xFFFF) == 0) {
-    CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
-  } else {
-    CpuState->x64.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+  if (!mSmBaseRelocated) {
+    //
+    // Configure SMBASE.
+    //
+    CpuState = (QEMU_SMRAM_SAVE_STATE_MAP *)(UINTN)(
+                                                    SMM_DEFAULT_SMBASE +
+                                                    SMRAM_SAVE_STATE_MAP_OFFSET
+                                                    );
+    if ((CpuState->x86.SMMRevId & 0xFFFF) == 0) {
+      CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+    } else {
+      CpuState->x64.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+    }
   }
 
   //
   // No need to program SMRRs on our virtual platform.
   //
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 8a426a4c10..6a281518f5 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -33,10 +33,14 @@
   MemoryAllocationLib
   PcdLib
   SafeIntLib
   SmmServicesTableLib
   UefiBootServicesTableLib
+  HobLib
+
+[Guids]
+  gSmmBaseHobGuid                ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
   gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
-- 
2.16.2.windows.1

Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase handling

[Gerd Hoffmann]

On Fri, Jan 13, 2023 at 03:17:34PM +0800, Wu, Jiaxin wrote:
> Below serial patches are to support the SMM Relocated SmBase handling.
> To achieve, new hob interface is procuded, and will be consumed by SMM
> CPU driver & SmmCpuFeaturesLib to do SmBase initialization:

Who produces that HOB?  I see only consumers in this patch series.

The patch series doesn't even pass BaseTools/Scripts/PatchCheck.py,
it will surely not pass CI.

> With SMM Base Hob, SMM CPU driver does not need the RSM instruction
> to reload the SMBASE register with the new allocated value in SMBASE
> field each time it exits SMM. SMBASE Register for each processors
> have already been programmed in parallel since the same default
> SMBASE Address(0x30000) is not used, thus the CPUs over-writing
> each other's SMM Save State Area will not happen.

Care to explain how this works?  SMBASE setup has to happen somewhere
(probably in the HOB producer), how does that work without using the
default smbase address?

take care,
  Gerd

Re: [PATCH v1 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Gerd Hoffmann]

  Hi,

> +  } else {
> +    mSmmInitialized = (BOOLEAN *)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
> +    ASSERT (mSmmInitialized != NULL);

ASSERT is not sufficient error handling.

take care,
  Gerd

Re: [PATCH v1 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Wu, Jiaxin]

Thanks Gerd, I will refine the code logic.


> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, January 13, 2023 5:51 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: Re: [PATCH v1 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM
> Base Hob for SmBase info
> 
>   Hi,
> 
> > +  } else {
> > +    mSmmInitialized = (BOOLEAN *)AllocateZeroPool (sizeof (BOOLEAN) *
> mMaxNumberOfCpus);
> > +    ASSERT (mSmmInitialized != NULL);
> 
> ASSERT is not sufficient error handling.
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase handling

[Wu, Jiaxin]

Thanks Comments Gerd, I will resolve the CI check issue (I have noticed that in https://github.com/tianocore/edk2/pull/3884) & refine the usage description in next version.

Anyway, Ray also helped explain the usage as below, I will integrate it in the hob definition .h file, help below can help understand the design.:

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.

I also explain the in the patch 2 for PiSmmCpuDxeSmm changes:

SMM CPU driver will retrieve the SMBASE addresses from SMM Base Hob
and installs the SMI handler at [SMBASE+8000h] for each processor
instead of relocating SMM Base addresses from SMRAM again.

With SMM Base Hob, SMM CPU driver does not need the RSM instruction
to reload the SMBASE register with the new allocated value in SMBASE
field each time it exits SMM. SMBASE Register for each processors
have already been programmed in parallel since the same default
SMBASE Address(0x30000) is not used, thus the CPUs over-writing
each other's SMM Save State Area will not happen. This way will save
boot time on multi-core system

  
thanks,
Jiaxin 


> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, January 13, 2023 5:50 PM
> To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase
> handling
> 
> On Fri, Jan 13, 2023 at 03:17:34PM +0800, Wu, Jiaxin wrote:
> > Below serial patches are to support the SMM Relocated SmBase handling.
> > To achieve, new hob interface is procuded, and will be consumed by SMM
> > CPU driver & SmmCpuFeaturesLib to do SmBase initialization:
> 
> Who produces that HOB?  I see only consumers in this patch series.
> 
> The patch series doesn't even pass BaseTools/Scripts/PatchCheck.py,
> it will surely not pass CI.
> 
> > With SMM Base Hob, SMM CPU driver does not need the RSM instruction
> > to reload the SMBASE register with the new allocated value in SMBASE
> > field each time it exits SMM. SMBASE Register for each processors
> > have already been programmed in parallel since the same default
> > SMBASE Address(0x30000) is not used, thus the CPUs over-writing
> > each other's SMM Save State Area will not happen.
> 
> Care to explain how this works?  SMBASE setup has to happen somewhere
> (probably in the HOB producer), how does that work without using the
> default smbase address?
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase handling

[Wu, Jiaxin]

For simple, this change will keep compatibility with the lack of the HOB. When the HOB doesn't exist, PiSmmCpuDxeSmm driver will keep the original logic to program the new SMBASE in driver itself.


> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Friday, January 13, 2023 6:19 PM
> To: Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io
> Subject: RE: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase
> handling
> 
> Thanks Comments Gerd, I will resolve the CI check issue (I have noticed that
> in https://github.com/tianocore/edk2/pull/3884) & refine the usage
> description in next version.
> 
> Anyway, Ray also helped explain the usage as below, I will integrate it in the
> hob definition .h file, help below can help understand the design.:
> 
> 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.
> 
> I also explain the in the patch 2 for PiSmmCpuDxeSmm changes:
> 
> SMM CPU driver will retrieve the SMBASE addresses from SMM Base Hob
> and installs the SMI handler at [SMBASE+8000h] for each processor
> instead of relocating SMM Base addresses from SMRAM again.
> 
> With SMM Base Hob, SMM CPU driver does not need the RSM instruction
> to reload the SMBASE register with the new allocated value in SMBASE
> field each time it exits SMM. SMBASE Register for each processors
> have already been programmed in parallel since the same default
> SMBASE Address(0x30000) is not used, thus the CPUs over-writing
> each other's SMM Save State Area will not happen. This way will save
> boot time on multi-core system
> 
> 
> thanks,
> Jiaxin
> 
> 
> > -----Original Message-----
> > From: Gerd Hoffmann <kraxel@redhat.com>
> > Sent: Friday, January 13, 2023 5:50 PM
> > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase
> > handling
> >
> > On Fri, Jan 13, 2023 at 03:17:34PM +0800, Wu, Jiaxin wrote:
> > > Below serial patches are to support the SMM Relocated SmBase handling.
> > > To achieve, new hob interface is procuded, and will be consumed by
> SMM
> > > CPU driver & SmmCpuFeaturesLib to do SmBase initialization:
> >
> > Who produces that HOB?  I see only consumers in this patch series.
> >
> > The patch series doesn't even pass BaseTools/Scripts/PatchCheck.py,
> > it will surely not pass CI.
> >
> > > With SMM Base Hob, SMM CPU driver does not need the RSM instruction
> > > to reload the SMBASE register with the new allocated value in SMBASE
> > > field each time it exits SMM. SMBASE Register for each processors
> > > have already been programmed in parallel since the same default
> > > SMBASE Address(0x30000) is not used, thus the CPUs over-writing
> > > each other's SMM Save State Area will not happen.
> >
> > Care to explain how this works?  SMBASE setup has to happen somewhere
> > (probably in the HOB producer), how does that work without using the
> > default smbase address?
> >
> > take care,
> >   Gerd

Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase handling

[Gerd Hoffmann]

On Fri, Jan 13, 2023 at 10:18:59AM +0000, Wu, Jiaxin wrote:
> Thanks Comments Gerd, I will resolve the CI check issue (I have noticed that in https://github.com/tianocore/edk2/pull/3884) & refine the usage description in next version.
> 
> Anyway, Ray also helped explain the usage as below, I will integrate it in the hob definition .h file, help below can help understand the design.:
> 
> 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.

There are no PEI module changes in this patch series.

So where does the HOB come from?

And what are the reasons for setting SMBASE in a PEI module instead
of PiSmmCpuDxeSmm?

take care,
  Gerd

Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase handling

[Gerd Hoffmann]

On Fri, Jan 13, 2023 at 03:17:34PM +0800, Wu, Jiaxin wrote:
> Below serial patches are to support the SMM Relocated SmBase handling.
> To achieve, new hob interface is procuded, and will be consumed by SMM
> CPU driver & SmmCpuFeaturesLib to do SmBase initialization:

The series breaks s3 suspend in ovmf.

take care,
  Gerd

Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase handling

[Wu, Jiaxin]

> 
> There are no PEI module changes in this patch series.
> 
> So where does the HOB come from?
> 

Who use, who produce. Here, just provide the interface capability.   

> And what are the reasons for setting SMBASE in a PEI module instead
> of PiSmmCpuDxeSmm?
> 

Do the smbase relocated ahead of SMM cpu driver can bring the benefit as I explain in the patch 2:

PiSmmCpuDxeSmm will retrieve the SMBASE addresses from SMM Base Hob
and installs the SMI handler at [SMBASE+8000h] for each processor
instead of relocating SMM Base addresses from SMRAM again.

With SMM Base Hob, PiSmmCpuDxeSmm does not need the RSM
instruction to reload the SMBASE register with the new allocated
SMBASE each time when it exits SMM. SMBASE Register for each
processors have already been programmed and all SMBASE address
have recorded in SMM Base Hob. So the same default SMBASE Address
(0x30000) will not be used, thus the CPUs over-writing
each other's SMM Save State Area will not happen. This way makes
the first SMI init can be executed in parallel and save boot
time on multi-core system.


> take care,
>   Gerd

Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase handling

[Wu, Jiaxin]

Gerd, 

Could you share me the steps to build OVMF & run the test? I did the OVMF related test almost 5 year ago, I forgot how to enable the OVFM in windows☹. 

Thanks,
Jiaxin 


> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, January 13, 2023 8:45 PM
> To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase
> handling
> 
> On Fri, Jan 13, 2023 at 03:17:34PM +0800, Wu, Jiaxin wrote:
> > Below serial patches are to support the SMM Relocated SmBase handling.
> > To achieve, new hob interface is procuded, and will be consumed by SMM
> > CPU driver & SmmCpuFeaturesLib to do SmBase initialization:
> 
> The series breaks s3 suspend in ovmf.
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase handling

[Wu, Jiaxin]

Hi Gerd,

I made some fix for the s3 flow with patch set v2, and now pass the CI check, you can review directly on PR https://github.com/tianocore/edk2/pull/3884

Thanks,
Jiaxin

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, January 13, 2023 8:45 PM
> To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase
> handling
> 
> On Fri, Jan 13, 2023 at 03:17:34PM +0800, Wu, Jiaxin wrote:
> > Below serial patches are to support the SMM Relocated SmBase handling.
> > To achieve, new hob interface is procuded, and will be consumed by SMM
> > CPU driver & SmmCpuFeaturesLib to do SmBase initialization:
> 
> The series breaks s3 suspend in ovmf.
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH v1 0/4] Support SMM Relocated SmBase handling

[Gerd Hoffmann]

On Fri, Jan 13, 2023 at 12:47:54PM +0000, Wu, Jiaxin wrote:
> > 
> > There are no PEI module changes in this patch series.
> > 
> > So where does the HOB come from?
> 
> Who use, who produce. Here, just provide the interface capability.   

Well, yes.  This series adds the interface (i.e. the HOB guid
and struct).  It also updates some modules which consume that
new interface.

It does *not* add a producer of that new interface.

> > And what are the reasons for setting SMBASE in a PEI module instead
> > of PiSmmCpuDxeSmm?
> 
> Do the smbase relocated ahead of SMM cpu driver can bring the benefit
> as I explain in the patch 2:

> PiSmmCpuDxeSmm will retrieve the SMBASE addresses from SMM Base Hob
> and installs the SMI handler at [SMBASE+8000h] for each processor
> instead of relocating SMM Base addresses from SMRAM again.

That doesn't answer my question.

Sure, if you do the SMM Base relocation somewhere else PiSmmCpuDxeSmm
shouldn't do it again.

It is not clear why you do the SMM Base relocation somewhere else.
The code for that is also not present in this patch series.

take care,
  Gerd