[PATCH v4 0/5] Simplify SMM Relocation Process

[PATCH v4 0/5] Simplify SMM Relocation Process

[Wu, Jiaxin]

Existing SMBASE Relocation is in the PiSmmCpuDxeSmm driver, which
will relocate the SMBASE of each processor by setting the SMBASE
field in the saved state map (at offset 7EF8h) to a new value.
The RSM instruction reloads the internal SMBASE register with the
value in SMBASE field when each time it exits SMM. All subsequent
SMI requests will use the new SMBASE to find the starting address
for the SMI handler (at SMBASE + 8000h).

Due to the default SMBASE for all x86 processors is 0x30000, the
APs' 1st SMI for rebase has to be executed one by one to avoid
the CPUs over-writing each other's SMM Save State Area (see
existing SmmRelocateBases() function), which means the next AP has
to wait for the previous AP to finish its 1st SMI, then it can call
into its 1st SMI for rebase via Smi Ipi command, thus leading the
existing SMBASE Relocation has to be running in series. Besides, it
needs very complex code to handle the AP exit semaphore
(mRebased[Index]), which will hook return address of SMM Save State
so that semaphore code can be executed immediately after AP exits
SMM for SMBASE relocation (see existing SemaphoreHook() function).

This series is to add the new SMM Base HOB for any PEI module to do
the SmBase relocation ahead of PiSmmCpuDxeSmm driver and store the
relocated SmBase address in array for each Processors. When the
SMBASE relocation happens in a PEI module, the PEI module shall
produce the SMM_BASE_HOB in HOB database which tells the
PiSmmCpuDxeSmm driver (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 relocate and
program the new SMBASE itself (keep existing SMBASE Relocation way).

With SMM Base Hob support, PiSmmCpuDxeSmm does not need the RSM
instruction to do the SMBASE Relocation. 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 in PiSmmCpuDxeSmm driver. This
way makes the first SMI init can be executed in parallel and save
boot time on multi-core system. Besides, Semaphore Hook code logic
is also not required, which will greatly simplify the SMBASE
Relocation flow.

Note:
This is the new way that firmware can program the SMBASE
independently of the RSM instruction. The PEI code performing
this logic will not be open sourced, similarly to other things
that are kept binary-only in the FSP. Due to the register
difference in different vender, and it has not been documented
in the Intel SDM yet, we need a new binary-only interface for
SMM Base HOB.

Jiaxin Wu (5):
  UefiCpuPkg/PiSmmCpuDxeSmm: Fix invalid InitializeMpSyncData call
  UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data
  UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info
  UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE configuration
  OvmfPkg/SmmCpuFeaturesLib: Check SmBase relocation supported or not

 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |   8 +
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
 UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  64 +++++++
 .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
 .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  23 ++-
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
 UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
 .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  29 +++-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  23 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 191 ++++++++++++++++-----
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  24 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
 UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
 15 files changed, 332 insertions(+), 50 deletions(-)
 create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h

-- 
2.16.2.windows.1

[PATCH v4 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Fix invalid InitializeMpSyncData call

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4338

No need call InitializeMpSyncData during normal boot SMI init,
because mSmmMpSyncData is NULL at that time. mSmmMpSyncData is
allocated in InitializeMpServiceData, which is invoked after
normal boot SMI init (SmmRelocateBases).

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/PiSmmCpuDxeSmm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 655175a2c6..f723b1d253 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -369,13 +369,11 @@ SmmInitHandler (
       if (!mSmmS3Flag) {
         //
         // Check XD and BTS features on each processor on normal boot
         //
         CheckFeatureSupported ();
-      }
-
-      if (mIsBsp) {
+      } else if (mIsBsp) {
         //
         // BSP rebase is already done above.
         // Initialize private data during S3 resume
         //
         InitializeMpSyncData ();
-- 
2.16.2.windows.1

[PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4337

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 relocate
and program the new SMBASE (in TSEG range) for each CPU thread. When
the SMBASE relocation happens in a PEI module, the PEI module shall
produce the SMM_BASE_HOB in HOB database which tells the
PiSmmCpuDxeSmm driver (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 relocate and
program the new SMBASE itself.

This patch adds the SMM Base HOB for any PEI module to do
the SmBase relocation ahead of PiSmmCpuDxeSmm driver and
store the relocated SmBase address in array for reach
Processors.

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 | 64 ++++++++++++++++++++++++++++++++++++
 UefiCpuPkg/UefiCpuPkg.dec            |  3 ++
 2 files changed, 67 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..d22b3b942c
--- /dev/null
+++ b/UefiCpuPkg/Include/Guid/SmmBaseHob.h
@@ -0,0 +1,64 @@
+/** @file
+  The Smm Base HOB is used to store the information of:
+  * The relocated SmBase address in array for each Processors.
+
+  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 relocate and program
+  the new SMBASE (in TSEG range) for each CPU thread. When the SMBASE relocation
+  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 relocate and program
+  the new SMBASE itself.
+
+  Note:
+  SMBASE relocation process needs to program the vender specific hardware
+  interface to set SMBASE, it should be in the thread scope. It's doable to
+  program the hardware interface using DXE MP service protocol in PiSmmCpuDxeSmm
+  entry point. But, considering the standalone MM environment where the CpuMm
+  driver runs in a isolated environment and it cannot invoke any DXE or PEI MP
+  service, we recommend to put the hardware interface programming in a separate
+  PEI module instead of in the PiSmmCpuDxeSmm driver.
+
+  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_
+
+#define SMM_BASE_HOB_DATA_GUID \
+  { \
+    0xc2217ba7, 0x03bb, 0x4f63, {0xa6, 0x47, 0x7c, 0x25, 0xc5, 0xfc, 0x9d, 0x73}  \
+  }
+
+#pragma pack(1)
+typedef struct {
+  ///
+  /// CpuIndex tells which CPU range this specific HOB instance described.
+  /// If CpuIndex is set to 0, it indicats the HOB describes the CPU from 0 to
+  /// NumberOfCpus - 1. The HOB list may contains multiple this HOB instances.
+  /// Each HOB instances describe the information for CPU from CpuIndex to
+  /// CpuIndex + NumberOfCpus - 1. The instance order in the HOB list is random
+  /// so consumer can not assume the CpuIndex of first instance is 0.
+  ///
+  UINT32    CpuIndex;
+  ///
+  /// Describes the Number of all max supported processors.
+  ///
+  UINT32    NumberOfProcessors;
+  ///
+  /// Pointer to SmBase address for each Processors.
+  ///
+  UINT64    SmBase[1];
+} 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 v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4337

Existing SMBASE Relocation is in the PiSmmCpuDxeSmm driver, which
will relocate the SMBASE of each processor by setting the SMBASE
field in the saved state map (at offset 7EF8h) to a new value.
The RSM instruction reloads the internal SMBASE register with the
value in SMBASE field when each time it exits SMM. All subsequent
SMI requests will use the new SMBASE to find the starting address
for the SMI handler (at SMBASE + 8000h).

Due to the default SMBASE for all x86 processors is 0x30000, the
APs' 1st SMI for rebase has to be executed one by one to avoid
the CPUs over-writing each other's SMM Save State Area (see
existing SmmRelocateBases() function), which means the next AP has
to wait for the previous AP to finish its 1st SMI, then it can call
into its 1st SMI for rebase via Smi Ipi command, thus leading the
existing SMBASE Relocation has to be running in series. Besides, it
needs very complex code to handle the AP exit semaphore
(mRebased[Index]), which will hook return address of SMM Save State
so that semaphore code can be executed immediately after AP exits
SMM for SMBASE relocation (see existing SemaphoreHook() function).

With SMM Base Hob support, PiSmmCpuDxeSmm does not need the RSM
instruction to do the SMBASE Relocation. 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 in PiSmmCpuDxeSmm driver. This
way makes the first SMI init can be executed in parallel and save
boot time on multi-core system. Besides, Semaphore Hook code logic
is also not required, which will greatly simplify the SMBASE
Relocation flow.

Mainly changes as below:
* Assume the biggest possibility of tile size is 8k.
* 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 first
SMI 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            |  29 +++-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        |  23 ++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 189 +++++++++++++++++++++------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  24 ++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   1 +
 5 files changed, 222 insertions(+), 44 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index fb4a44eab6..39c0b002f0 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -822,13 +822,38 @@ SmmRestoreCpu (
     //
     InitializeCpuBeforeRebase ();
   }
 
   //
-  // Restore SMBASE for BSP and all APs
+  // Make sure the gSmmBaseHobGuid existence status is the same between normal and S3 boot.
   //
-  SmmRelocateBases ();
+  ASSERT (mSmmRelocated == (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL));
+  if (mSmmRelocated != (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "gSmmBaseHobGuid %a produced in normal boot but %a in S3 boot!",
+      mSmmRelocated ? "is" : "is not",
+      mSmmRelocated ? "is not" : "is"
+      ));
+    CpuDeadLoop ();
+  }
+
+  //
+  // Check whether Smm Relocation is done or not.
+  // If not, will do the SmmBases Relocation here!!!
+  //
+  if (!mSmmRelocated) {
+    //
+    // Restore SMBASE for BSP and all APs
+    //
+    SmmRelocateBases ();
+  } else {
+    //
+    // Issue SMI IPI (All Excluding  Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+    //
+    ExecuteFirstSmiInit ();
+  }
 
   //
   // 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..3744f35214 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1721,17 +1721,40 @@ SmiRendezvous (
   UINTN       Index;
   UINTN       Cr2;
 
   ASSERT (CpuIndex < mMaxNumberOfCpus);
 
+  if (mSmmRelocated) {
+    ASSERT (mSmmInitialized != NULL);
+  }
+
   //
   // Save Cr2 because Page Fault exception in SMM may override its value,
   // 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);
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index f723b1d253..950b5f4c72 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -57,11 +57,10 @@ SMM_CPU_PRIVATE_DATA  *gSmmCpuPrivate = &mSmmCpuPrivateData;
 
 //
 // SMM Relocation variables
 //
 volatile BOOLEAN  *mRebased;
-volatile BOOLEAN  mIsBsp;
 
 ///
 /// Handle for the SMM CPU Protocol
 ///
 EFI_HANDLE  mSmmCpuHandle = NULL;
@@ -83,10 +82,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;
@@ -341,58 +344,108 @@ VOID
 EFIAPI
 SmmInitHandler (
   VOID
   )
 {
-  UINT32  ApicId;
-  UINTN   Index;
+  UINT32   ApicId;
+  UINTN    Index;
+  BOOLEAN  IsBsp;
 
   //
   // Update SMM IDT entries' code segment and load IDT
   //
   AsmWriteIdtr (&gcSmiIdtr);
   ApicId = GetApicId ();
 
+  IsBsp = (BOOLEAN)(mBspApicId == ApicId);
+
   ASSERT (mNumberOfCpus <= mMaxNumberOfCpus);
 
   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,
+        IsBsp,
         gSmmCpuPrivate->ProcessorInfo,
         &mCpuHotPlugData
         );
 
       if (!mSmmS3Flag) {
         //
         // Check XD and BTS features on each processor on normal boot
         //
         CheckFeatureSupported ();
-      } else if (mIsBsp) {
+      } else if (IsBsp) {
         //
         // BSP rebase is already done above.
         // Initialize private data during S3 resume
         //
         InitializeMpSyncData ();
       }
 
-      //
-      // Hook return after RSM to set SMM re-based flag
-      //
-      SemaphoreHook (Index, &mRebased[Index]);
+      if (!mSmmRelocated) {
+        //
+        // Hook return after RSM to set SMM re-based flag
+        //
+        SemaphoreHook (Index, &mRebased[Index]);
+      }
 
       return;
     }
   }
 
   ASSERT (FALSE);
 }
 
+/**
+  Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+
+**/
+VOID
+ExecuteFirstSmiInit (
+  VOID
+  )
+{
+  UINTN  Index;
+
+  if (mSmmInitialized == NULL) {
+    mSmmInitialized = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
+  }
+
+  ASSERT (mSmmInitialized != NULL);
+  if (mSmmInitialized == NULL) {
+    return;
+  }
+
+  //
+  // Reset the mSmmInitialized to false.
+  //
+  ZeroMem (mSmmInitialized, sizeof (BOOLEAN) * mMaxNumberOfCpus);
+
+  //
+  // Get the BSP ApicId.
+  //
+  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) {
+    }
+  }
+}
+
 /**
   Relocate SmmBases for each processor.
 
   Execute on first boot and all S3 resumes
 
@@ -405,11 +458,10 @@ SmmRelocateBases (
 {
   UINT8                 BakBuf[BACK_BUF_SIZE];
   SMRAM_SAVE_STATE_MAP  BakBuf2;
   SMRAM_SAVE_STATE_MAP  *CpuStatePtr;
   UINT8                 *U8Ptr;
-  UINT32                ApicId;
   UINTN                 Index;
   UINTN                 BspIndex;
 
   //
   // Make sure the reserved size is large enough for procedure SmmInitTemplate.
@@ -446,21 +498,20 @@ SmmRelocateBases (
   CopyMem (U8Ptr, gcSmmInitTemplate, gcSmmInitSize);
 
   //
   // Retrieve the local APIC ID of current processor
   //
-  ApicId = GetApicId ();
+  mBspApicId = GetApicId ();
 
   //
   // Relocate SM bases for all APs
   // This is APs' 1st SMI - rebase will be done here, and APs' default SMI handler will be overridden by gcSmmInitTemplate
   //
-  mIsBsp   = FALSE;
   BspIndex = (UINTN)-1;
   for (Index = 0; Index < mNumberOfCpus; Index++) {
     mRebased[Index] = FALSE;
-    if (ApicId != (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
+    if (mBspApicId != (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
       SendSmiIpi ((UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId);
       //
       // Wait for this AP to finish its 1st SMI
       //
       while (!mRebased[Index]) {
@@ -475,12 +526,11 @@ SmmRelocateBases (
 
   //
   // Relocate BSP's SMM base
   //
   ASSERT (BspIndex != (UINTN)-1);
-  mIsBsp = TRUE;
-  SendSmiIpi (ApicId);
+  SendSmiIpi (mBspApicId);
   //
   // Wait for the BSP to finish its 1st SMI
   //
   while (!mRebased[BspIndex]) {
   }
@@ -559,10 +609,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 ();
@@ -787,30 +842,58 @@ 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.
-  //
-  // 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.
+  // Retrive the allocated SmmBase from gSmmBaseHobGuid. If found,
+  // means the SmBase relocation has been done.
   //
-  // 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);
+  GuidHob = GetFirstGuidHob (&gSmmBaseHobGuid);
+  if (GuidHob != NULL) {
+    //
+    // Check whether the Required TileSize is enough.
+    //
+    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));
+      CpuDeadLoop ();
+      return RETURN_BUFFER_TOO_SMALL;
+    }
+
+    SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
+
+    //
+    // Assume single instance of HOB produced, expect the HOB.NumberOfProcessors equals to the mMaxNumberOfCpus.
+    //
+    ASSERT (SmmBaseHobData->NumberOfProcessors == (UINT32)mMaxNumberOfCpus && SmmBaseHobData->CpuIndex == 0);
+    mSmmRelocated = TRUE;
   } else {
-    Buffer = AllocateAlignedCodePages (BufferPages, SIZE_4KB);
-  }
+    //
+    // When the HOB doesn't exist, allocate new SMBASE itself.
+    //
+    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);
@@ -841,11 +924,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)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) {
@@ -954,21 +1038,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
@@ -995,10 +1085,25 @@ 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) to execute first SMI init.
+  //
+  if (mSmmRelocated) {
+    ExecuteFirstSmiInit ();
+
+    //
+    // 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..50c9695c4f 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,29 @@ 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
+  );
+
+/**
+  Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+
+**/
+VOID
+ExecuteFirstSmiInit (
+  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 +420,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;
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 v4 4/5] UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4337

This patch is to avoid 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     | 23 +++++++++++++++++++---
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  4 ++++
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |  1 +
 UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |  1 -
 .../StandaloneMmCpuFeaturesLib.inf                 |  4 ++++
 6 files changed, 31 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..7c3836286b 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  mSmmCpuFeaturesSmmRelocated;
+
 //
 // Set default value to assume MTRRs need to be configured on each SMI
 //
 BOOLEAN  mNeedConfigureMtrrs = TRUE;
 
@@ -142,10 +148,16 @@ 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.
+  //
+  mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL);
 }
 
 /**
   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 +197,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 (!mSmmCpuFeaturesSmmRelocated) {
+    //
+    // 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 v4 5/5] OvmfPkg/SmmCpuFeaturesLib: Check SmBase relocation supported or not

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4337

This patch is to check SmBase relocation supported or not.
If gSmmBaseHobGuid found, means SmBase info has been relocated
and recorded in the SmBase array. ASSERT it's not supported in OVMF.

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>
---
 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c   | 8 ++++++++
 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 6693666d04..ec918cad06 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -15,14 +15,16 @@
 #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
@@ -41,10 +43,16 @@ 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. ASSERT it's not supported in OVMF.
+  //
+  ASSERT (GetFirstGuidHob (&gSmmBaseHobGuid) == NULL);
+
   //
   // No need to program SMRRs on our virtual platform.
   //
   return EFI_SUCCESS;
 }
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 v4 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Fix invalid InitializeMpSyncData call

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin
> Sent: Friday, February 10, 2023 2:05 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: [edk2-devel] [PATCH v4 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Fix
> invalid InitializeMpSyncData call
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4338
> 
> No need call InitializeMpSyncData during normal boot SMI init,
> because mSmmMpSyncData is NULL at that time. mSmmMpSyncData is
> allocated in InitializeMpServiceData, which is invoked after
> normal boot SMI init (SmmRelocateBases).
> 
> 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/PiSmmCpuDxeSmm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 655175a2c6..f723b1d253 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -369,13 +369,11 @@ SmmInitHandler (
>        if (!mSmmS3Flag) {
>          //
>          // Check XD and BTS features on each processor on normal boot
>          //
>          CheckFeatureSupported ();
> -      }
> -
> -      if (mIsBsp) {
> +      } else if (mIsBsp) {
>          //
>          // BSP rebase is already done above.
>          // Initialize private data during S3 resume
>          //
>          InitializeMpSyncData ();
> --
> 2.16.2.windows.1
> 
> 
> 
> 
> 

Re: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Ni, Ray]

Jiaxin,
> +  ///
> +  /// Pointer to SmBase address for each Processors.
> +  ///
> +  UINT64    SmBase[1];

Why SmBase[1] instead of SmBase[0]?
I think using SmBase[0] can better help C code to calculate the HOB size.
Simply sizeof (SMM_BASE_HOB_DATA) * sizeof (UINT64) * CpuCount,
instead of sizeof (SMM_BASE_HOB_DATA) * sizeof (UINT64) * (CpuCount - 1).

Re: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Wu, Jiaxin]

Hi Ray, 

Both is fine to me, one thought is that we must have one CPU, so change to 1 and I also checked some existing usage cases like the EFI_SMRAM_HOB_DESCRIPTOR_BLOCK.Descriptor, WIN_CERTIFICATE_UEFI_GUID. CertData, it also the same way.

Thanks,
Jiaxin 

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, February 10, 2023 3:15 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: RE: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base
> HOB Data
> 
> Jiaxin,
> > +  ///
> > +  /// Pointer to SmBase address for each Processors.
> > +  ///
> > +  UINT64    SmBase[1];
> 
> Why SmBase[1] instead of SmBase[0]?
> I think using SmBase[0] can better help C code to calculate the HOB size.
> Simply sizeof (SMM_BASE_HOB_DATA) * sizeof (UINT64) * CpuCount,
> instead of sizeof (SMM_BASE_HOB_DATA) * sizeof (UINT64) * (CpuCount -
> 1).
> 

Re: [edk2-devel] [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Marvin Häuser]

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

Hi Ray and Jiaxin,

Quick reminder that [1] invokes undefined behaviour and [0] is not legal C, but a compiler extension. The canonical way is a flexible array member using [].

Off-topic and nitpicking, but I started wondering why so many structures are packed. For UEFI, packing would only help saving RAM when there‘s actual padding (unlike this case, where there isn‘t). All architectures require natural data alignment („unless specifies otherwise“), so there should be no compatibility concerns for structures specific to UEFI and UEFI PI ever. When considering ABIs outside UEFI, even then natural alignment is the strictest realistically possible outside super niche platforms, as even executable formats don‘t pack their structures (this structure also is inherently naturally aligned).

Best regards,
Marvin

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

Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Marvin Häuser]

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

Hi Jiaxin,

1) mSmmInitialized *must* be volatile. Your current code may cause anything, from skipping waiting entirely (the loop is optimized away as the compiler considers it free from side effects) to stalling (the memory access is optimized away as the compiler considers it locally-immutable).

2) ASSERTs on memory allocation failures are one of the most terrible edk2 "paradigms".

3) Comparisons against boolean constants are not allowed by the code style spec.

Best regards,
Marvin

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

Re: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Gerd Hoffmann]

  Hi,

> +#pragma pack(1)
> +typedef struct {
> +  ///
> +  /// CpuIndex tells which CPU range this specific HOB instance described.
> +  /// If CpuIndex is set to 0, it indicats the HOB describes the CPU from 0 to
> +  /// NumberOfCpus - 1. The HOB list may contains multiple this HOB instances.
> +  /// Each HOB instances describe the information for CPU from CpuIndex to
> +  /// CpuIndex + NumberOfCpus - 1. The instance order in the HOB list is random
> +  /// so consumer can not assume the CpuIndex of first instance is 0.
> +  ///
> +  UINT32    CpuIndex;
> +  ///
> +  /// Describes the Number of all max supported processors.
> +  ///
> +  UINT32    NumberOfProcessors;
> +  ///
> +  /// Pointer to SmBase address for each Processors.
> +  ///
> +  UINT64    SmBase[1];
> +} SMM_BASE_HOB_DATA;
> +#pragma pack()

No generic chunked hobs as suggested/discussed on v3 of this series?
Why not?

take care,
  Gerd

Re: [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Gerd Hoffmann]

> Mainly changes as below:
> * Assume the biggest possibility of tile size is 8k.
> * 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 first
> SMI init before normal SMI sources happen.
> * Call SmmCpuFeaturesInitializeProcessor() in parallel.

Four changes in a single patch.  Please split them up.

thanks,
  Gerd

Re: [PATCH v4 5/5] OvmfPkg/SmmCpuFeaturesLib: Check SmBase relocation supported or not

[Gerd Hoffmann]

On Fri, Feb 10, 2023 at 02:05:19PM +0800, Jiaxin Wu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4337
> 
> This patch is to check SmBase relocation supported or not.
> If gSmmBaseHobGuid found, means SmBase info has been relocated
> and recorded in the SmBase array. ASSERT it's not supported in OVMF.
> 
> 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>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Ni, Ray]

Gerd,
That requires changing PI spec and all HOB consuming logic.
But if you have a simple idea, please advise.

Thanks,
Ray

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, February 10, 2023 7:24 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 v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB
> Data
> 
>   Hi,
> 
> > +#pragma pack(1)
> > +typedef struct {
> > +  ///
> > +  /// CpuIndex tells which CPU range this specific HOB instance described.
> > +  /// If CpuIndex is set to 0, it indicats the HOB describes the CPU from 0 to
> > +  /// NumberOfCpus - 1. The HOB list may contains multiple this HOB
> instances.
> > +  /// Each HOB instances describe the information for CPU from CpuIndex to
> > +  /// CpuIndex + NumberOfCpus - 1. The instance order in the HOB list is
> random
> > +  /// so consumer can not assume the CpuIndex of first instance is 0.
> > +  ///
> > +  UINT32    CpuIndex;
> > +  ///
> > +  /// Describes the Number of all max supported processors.
> > +  ///
> > +  UINT32    NumberOfProcessors;
> > +  ///
> > +  /// Pointer to SmBase address for each Processors.
> > +  ///
> > +  UINT64    SmBase[1];
> > +} SMM_BASE_HOB_DATA;
> > +#pragma pack()
> 
> No generic chunked hobs as suggested/discussed on v3 of this series?
> Why not?
> 
> take care,
>   Gerd

Re: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Gerd Hoffmann]

On Fri, Feb 10, 2023 at 11:56:01AM +0000, Ni, Ray wrote:
> Gerd,
> That requires changing PI spec 

Yes, but we have
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-First-Process

> and all HOB consuming logic.

There is no need to change the existing logic.  Code to handle
chunked HOBs can go to new helper functions in HobLib.

take care,
  Gerd

Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Ni, Ray]

> +  ZeroMem (mSmmInitialized, sizeof (BOOLEAN) * mMaxNumberOfCpus);
> ...
> +  for (Index = 0; Index < mNumberOfCpus; Index++) {
> +    while (mSmmInitialized[Index] == FALSE) {
> +    }

Above code sets the BOOLEAN array to all FALSE.
Then polling on the array to be all TRUE.
We need to add "volatile" to the mSmmInitialized to tell
the compiler not to optimize the code but always check the
memory content.

A better way is to use compiler barrier but there is no
edk2 lib API to wrap it for different compilers
Let's just use "volatile" for now.

Thanks,
Ray

Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Ni, Ray]

Gerd,
All the 4 are needed to make sure SMM still works.
Only having one will cause system hang.
I think Jiaxin's commit message is to describe what has been done to
enable the single feature: Early SMM rebase.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Friday, February 10, 2023 7:27 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: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm:
> Consume SMM Base Hob for SmBase info
> 
> > Mainly changes as below:
> > * Assume the biggest possibility of tile size is 8k.
> > * 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 first
> > SMI init before normal SMI sources happen.
> > * Call SmmCpuFeaturesInitializeProcessor() in parallel.
> 
> Four changes in a single patch.  Please split them up.
> 
> thanks,
>   Gerd
> 
> 
> 
> 
> 

Re: [PATCH v4 4/5] UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>
  -- but please update the copyright year.

> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Friday, February 10, 2023 2:05 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: [PATCH v4 4/5] UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE
> configuration
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4337
> 
> This patch is to avoid 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     | 23
> +++++++++++++++++++---
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  4 ++++
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |  1 +
>  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |  1 -
>  .../StandaloneMmCpuFeaturesLib.inf                 |  4 ++++
>  6 files changed, 31 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..7c3836286b 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  mSmmCpuFeaturesSmmRelocated;
> +
>  //
>  // Set default value to assume MTRRs need to be configured on each SMI
>  //
>  BOOLEAN  mNeedConfigureMtrrs = TRUE;
> 
> @@ -142,10 +148,16 @@ 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.
> +  //
> +  mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob
> (&gSmmBaseHobGuid) != NULL);
>  }
> 
>  /**
>    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 +197,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 (!mSmmCpuFeaturesSmmRelocated) {
> +    //
> +    // 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

Re: [edk2-devel] [PATCH v4 0/5] Simplify SMM Relocation Process

[Ni, Ray]

Jiaxin,
I provide separate review comments for each patch.

Can you please make sure the copy right year is updated for every file change?
I may not emphasize this for each patch.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Jiaxin
> Sent: Friday, February 10, 2023 2:05 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH v4 0/5] Simplify SMM Relocation Process
> 
> Existing SMBASE Relocation is in the PiSmmCpuDxeSmm driver, which
> will relocate the SMBASE of each processor by setting the SMBASE
> field in the saved state map (at offset 7EF8h) to a new value.
> The RSM instruction reloads the internal SMBASE register with the
> value in SMBASE field when each time it exits SMM. All subsequent
> SMI requests will use the new SMBASE to find the starting address
> for the SMI handler (at SMBASE + 8000h).
> 
> Due to the default SMBASE for all x86 processors is 0x30000, the
> APs' 1st SMI for rebase has to be executed one by one to avoid
> the CPUs over-writing each other's SMM Save State Area (see
> existing SmmRelocateBases() function), which means the next AP has
> to wait for the previous AP to finish its 1st SMI, then it can call
> into its 1st SMI for rebase via Smi Ipi command, thus leading the
> existing SMBASE Relocation has to be running in series. Besides, it
> needs very complex code to handle the AP exit semaphore
> (mRebased[Index]), which will hook return address of SMM Save State
> so that semaphore code can be executed immediately after AP exits
> SMM for SMBASE relocation (see existing SemaphoreHook() function).
> 
> This series is to add the new SMM Base HOB for any PEI module to do
> the SmBase relocation ahead of PiSmmCpuDxeSmm driver and store the
> relocated SmBase address in array for each Processors. When the
> SMBASE relocation happens in a PEI module, the PEI module shall
> produce the SMM_BASE_HOB in HOB database which tells the
> PiSmmCpuDxeSmm driver (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 relocate and
> program the new SMBASE itself (keep existing SMBASE Relocation way).
> 
> With SMM Base Hob support, PiSmmCpuDxeSmm does not need the RSM
> instruction to do the SMBASE Relocation. 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 in PiSmmCpuDxeSmm driver. This
> way makes the first SMI init can be executed in parallel and save
> boot time on multi-core system. Besides, Semaphore Hook code logic
> is also not required, which will greatly simplify the SMBASE
> Relocation flow.
> 
> Note:
> This is the new way that firmware can program the SMBASE
> independently of the RSM instruction. The PEI code performing
> this logic will not be open sourced, similarly to other things
> that are kept binary-only in the FSP. Due to the register
> difference in different vender, and it has not been documented
> in the Intel SDM yet, we need a new binary-only interface for
> SMM Base HOB.
> 
> Jiaxin Wu (5):
>   UefiCpuPkg/PiSmmCpuDxeSmm: Fix invalid InitializeMpSyncData call
>   UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data
>   UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info
>   UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE configuration
>   OvmfPkg/SmmCpuFeaturesLib: Check SmBase relocation supported or not
> 
>  .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |   8 +
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
>  UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  64 +++++++
>  .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
>  .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  23 ++-
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
>  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
>  .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  29 +++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  23 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 191
> ++++++++++++++++-----
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  24 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
>  UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
>  15 files changed, 332 insertions(+), 50 deletions(-)
>  create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h
> 
> --
> 2.16.2.windows.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Gerd Hoffmann]

On Fri, Feb 10, 2023 at 12:37:23PM +0000, Ni, Ray wrote:
> Gerd,
> All the 4 are needed to make sure SMM still works.

The new way (HOB present) of handling SMM init will surely require the
whole patch series being applied.

For the old way (HOB not present) of handling SMM init should be
possible to split up into smaller pieces, which each step being
individually testable.  Specifically the SMI handler reorganization
should be possible to split out, and I think the tiling changes too.

These are preparing changes reorganizing the code, once they are in
place the final patch adding the new code paths to handle the new SMM
init should be relatively small.

take care,
  Gerd

Re: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Ni, Ray]

Gerd,
Firstly, chunk idea allows to split the >64K hob to multiple one without using
domain knowledge, that means the split can happen in byte level. It will hurt
debuggability a lot: When I exam the HOB list during debugging, it's hard to
combine/group the chunk hobs with the same GUID into >64K big hobs.

Secondly, Code-First process requires the code be created in edk2-staging repo.
It means the code cannot be merged in edk2 trunk.

I would like to collect more use cases for >64K hobs and
if more such hobs are needed from different domains (not just CPU), I will
consider to use the chunk idea.

Thanks,
Ray

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, February 10, 2023 8:32 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: Re: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB
> Data
> 
> On Fri, Feb 10, 2023 at 11:56:01AM +0000, Ni, Ray wrote:
> > Gerd,
> > That requires changing PI spec
> 
> Yes, but we have
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-First-
> Process
> 
> > and all HOB consuming logic.
> 
> There is no need to change the existing logic.  Code to handle
> chunked HOBs can go to new helper functions in HobLib.
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Wu, Jiaxin]

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

Thanks Marvin, I will fix according the comments. For 2), I don’t see assert only code in my patch. The only allocation in this patch is below, which has already been handled by if check.

  if (mSmmInitialized == NULL) {
    mSmmInitialized = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
  }

  ASSERT (mSmmInitialized != NULL);
  if (mSmmInitialized == NULL) {
    return;
  }


From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin H?user
Sent: Friday, February 10, 2023 6:00 PM
To: Wu; Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

Hi Jiaxin,

1) mSmmInitialized *must* be volatile. Your current code may cause anything, from skipping waiting entirely (the loop is optimized away as the compiler considers it free from side effects) to stalling (the memory access is optimized away as the compiler considers it locally-immutable).

2) ASSERTs on memory allocation failures are one of the most terrible edk2 "paradigms".

3) Comparisons against boolean constants are not allowed by the code style spec.

Best regards,
Marvin


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

Re: [edk2-devel] [PATCH v4 0/5] Simplify SMM Relocation Process

[Wu, Jiaxin]

Thanks Ray, I will update all.

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, February 10, 2023 8:57 PM
> To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH v4 0/5] Simplify SMM Relocation Process
> 
> Jiaxin,
> I provide separate review comments for each patch.
> 
> Can you please make sure the copy right year is updated for every file
> change?
> I may not emphasize this for each patch.
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin
> > Sent: Friday, February 10, 2023 2:05 PM
> > To: devel@edk2.groups.io
> > Subject: [edk2-devel] [PATCH v4 0/5] Simplify SMM Relocation Process
> >
> > Existing SMBASE Relocation is in the PiSmmCpuDxeSmm driver, which
> > will relocate the SMBASE of each processor by setting the SMBASE
> > field in the saved state map (at offset 7EF8h) to a new value.
> > The RSM instruction reloads the internal SMBASE register with the
> > value in SMBASE field when each time it exits SMM. All subsequent
> > SMI requests will use the new SMBASE to find the starting address
> > for the SMI handler (at SMBASE + 8000h).
> >
> > Due to the default SMBASE for all x86 processors is 0x30000, the
> > APs' 1st SMI for rebase has to be executed one by one to avoid
> > the CPUs over-writing each other's SMM Save State Area (see
> > existing SmmRelocateBases() function), which means the next AP has
> > to wait for the previous AP to finish its 1st SMI, then it can call
> > into its 1st SMI for rebase via Smi Ipi command, thus leading the
> > existing SMBASE Relocation has to be running in series. Besides, it
> > needs very complex code to handle the AP exit semaphore
> > (mRebased[Index]), which will hook return address of SMM Save State
> > so that semaphore code can be executed immediately after AP exits
> > SMM for SMBASE relocation (see existing SemaphoreHook() function).
> >
> > This series is to add the new SMM Base HOB for any PEI module to do
> > the SmBase relocation ahead of PiSmmCpuDxeSmm driver and store the
> > relocated SmBase address in array for each Processors. When the
> > SMBASE relocation happens in a PEI module, the PEI module shall
> > produce the SMM_BASE_HOB in HOB database which tells the
> > PiSmmCpuDxeSmm driver (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 relocate and
> > program the new SMBASE itself (keep existing SMBASE Relocation way).
> >
> > With SMM Base Hob support, PiSmmCpuDxeSmm does not need the RSM
> > instruction to do the SMBASE Relocation. 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 in PiSmmCpuDxeSmm driver. This
> > way makes the first SMI init can be executed in parallel and save
> > boot time on multi-core system. Besides, Semaphore Hook code logic
> > is also not required, which will greatly simplify the SMBASE
> > Relocation flow.
> >
> > Note:
> > This is the new way that firmware can program the SMBASE
> > independently of the RSM instruction. The PEI code performing
> > this logic will not be open sourced, similarly to other things
> > that are kept binary-only in the FSP. Due to the register
> > difference in different vender, and it has not been documented
> > in the Intel SDM yet, we need a new binary-only interface for
> > SMM Base HOB.
> >
> > Jiaxin Wu (5):
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Fix invalid InitializeMpSyncData call
> >   UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase
> info
> >   UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE configuration
> >   OvmfPkg/SmmCpuFeaturesLib: Check SmBase relocation supported or
> not
> >
> >  .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |   8 +
> >  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
> >  UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  64 +++++++
> >  .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
> >  .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  23 ++-
> >  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
> >  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
> >  .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
> >  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  29 +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  23 +++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 191
> > ++++++++++++++++-----
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  24 +++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
> >  UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
> >  15 files changed, 332 insertions(+), 50 deletions(-)
> >  create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h
> >
> > --
> > 2.16.2.windows.1
> >
> >
> >
> > 
> >

Re: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Wu, Jiaxin]

Combine the situation, I prefer the case by case handling to simplify the patch series purpose for the smbase pre-relocation support.

Thanks,
Jiaxin 



> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, February 10, 2023 9:13 PM
> To: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Zimmer,
> Vincent <vincent.zimmer@intel.com>
> Subject: RE: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base
> HOB Data
> 
> Gerd,
> Firstly, chunk idea allows to split the >64K hob to multiple one without using
> domain knowledge, that means the split can happen in byte level. It will hurt
> debuggability a lot: When I exam the HOB list during debugging, it's hard to
> combine/group the chunk hobs with the same GUID into >64K big hobs.
> 
> Secondly, Code-First process requires the code be created in edk2-staging
> repo.
> It means the code cannot be merged in edk2 trunk.
> 
> I would like to collect more use cases for >64K hobs and
> if more such hobs are needed from different domains (not just CPU), I will
> consider to use the chunk idea.
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Gerd Hoffmann <kraxel@redhat.com>
> > Sent: Friday, February 10, 2023 8:32 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io; Dong, Eric
> > <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> > Subject: Re: [PATCH v4 2/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base
> HOB
> > Data
> >
> > On Fri, Feb 10, 2023 at 11:56:01AM +0000, Ni, Ray wrote:
> > > Gerd,
> > > That requires changing PI spec
> >
> > Yes, but we have
> > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-First-
> > Process
> >
> > > and all HOB consuming logic.
> >
> > There is no need to change the existing logic.  Code to handle
> > chunked HOBs can go to new helper functions in HobLib.
> >
> > take care,
> >   Gerd

Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Wu, Jiaxin]

Hi Gerd,

The change impact the old way of handling SMM init is only: refine the SmmInitHandler() to handle the code for the different execution branches (actually, it's also related to the patch purpose as I explained why removing the mIsBsp).  It's fine to do this separating work. I will do that in the next version patch.

But for below items, they all belong to the new smbase relocation flow:

>* Assume the biggest possibility of tile size is 8k.

--- for title size, I evaluated this according your comments before, I updated/moved it into the if condition check to avoid the old smm init impact. Because the old smm init should have no such assumption. So, it has to combine with new way handling.    

  GuidHob = GetFirstGuidHob (&gSmmBaseHobGuid);
  if (GuidHob != NULL) {
    //
    // Check whether the Required TileSize is enough.
    //
    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));
      CpuDeadLoop ();
      return RETURN_BUFFER_TOO_SMALL;
    }
	....
  }

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

This is about the SMI handling logic with new way. Once SMI happen, there is only one copy of code in smbase+8000 to handling both smm relocation & normal smi. Instead of original 2 copies of code in smbase+8000. We don't change original smi template code. 


>* Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for first
>SMI init before normal SMI sources happen.
>* Call SmmCpuFeaturesInitializeProcessor() in parallel.

Above 2 are the same for the new way to issue the smi init process within if condition happen. All Excluding Self SMM IPI will achieve the parallel execution (SmmCpuFeaturesInitializeProcessor).


  //
  // 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) to execute first SMI init.
  //
  if (mSmmRelocated) {
    ExecuteFirstSmiInit ();

    //
    // Call hook for BSP to perform extra actions in normal mode after all
    // SMM base addresses have been relocated on all CPUs
    //
    SmmCpuFeaturesSmmRelocationComplete ();
  }

Thanks,
Jiaxin 



> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Friday, February 10, 2023 9:02 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm:
> Consume SMM Base Hob for SmBase info
> 
> On Fri, Feb 10, 2023 at 12:37:23PM +0000, Ni, Ray wrote:
> > Gerd,
> > All the 4 are needed to make sure SMM still works.
> 
> The new way (HOB present) of handling SMM init will surely require the
> whole patch series being applied.
> 
> For the old way (HOB not present) of handling SMM init should be
> possible to split up into smaller pieces, which each step being
> individually testable.  Specifically the SMI handler reorganization
> should be possible to split out, and I think the tiling changes too.
> 
> These are preparing changes reorganizing the code, once they are in
> place the final patch adding the new code paths to handle the new SMM
> init should be relatively small.
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Ni, Ray]

Will review v6 of PiSmmCpu driver change after you two are aligned.

> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Monday, February 13, 2023 12:15 PM
> To: kraxel@redhat.com; Ni, Ray <ray.ni@intel.com>
> Cc: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: RE: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm:
> Consume SMM Base Hob for SmBase info
> 
> Hi Gerd,
> 
> The change impact the old way of handling SMM init is only: refine the
> SmmInitHandler() to handle the code for the different execution branches
> (actually, it's also related to the patch purpose as I explained why removing
> the mIsBsp).  It's fine to do this separating work. I will do that in the next
> version patch.
> 
> But for below items, they all belong to the new smbase relocation flow:
> 
> >* Assume the biggest possibility of tile size is 8k.
> 
> --- for title size, I evaluated this according your comments before, I
> updated/moved it into the if condition check to avoid the old smm init impact.
> Because the old smm init should have no such assumption. So, it has to
> combine with new way handling.
> 
>   GuidHob = GetFirstGuidHob (&gSmmBaseHobGuid);
>   if (GuidHob != NULL) {
>     //
>     // Check whether the Required TileSize is enough.
>     //
>     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));
>       CpuDeadLoop ();
>       return RETURN_BUFFER_TOO_SMALL;
>     }
> 	....
>   }
> 
> >* 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.
> 
> This is about the SMI handling logic with new way. Once SMI happen, there is
> only one copy of code in smbase+8000 to handling both smm relocation &
> normal smi. Instead of original 2 copies of code in smbase+8000. We don't
> change original smi template code.
> 
> 
> >* Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for first
> >SMI init before normal SMI sources happen.
> >* Call SmmCpuFeaturesInitializeProcessor() in parallel.
> 
> Above 2 are the same for the new way to issue the smi init process within if
> condition happen. All Excluding Self SMM IPI will achieve the parallel
> execution (SmmCpuFeaturesInitializeProcessor).
> 
> 
>   //
>   // 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) to
> execute first SMI init.
>   //
>   if (mSmmRelocated) {
>     ExecuteFirstSmiInit ();
> 
>     //
>     // Call hook for BSP to perform extra actions in normal mode after all
>     // SMM base addresses have been relocated on all CPUs
>     //
>     SmmCpuFeaturesSmmRelocationComplete ();
>   }
> 
> Thanks,
> Jiaxin
> 
> 
> 
> > -----Original Message-----
> > From: kraxel@redhat.com <kraxel@redhat.com>
> > Sent: Friday, February 10, 2023 9:02 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v4 3/5] UefiCpuPkg/PiSmmCpuDxeSmm:
> > Consume SMM Base Hob for SmBase info
> >
> > On Fri, Feb 10, 2023 at 12:37:23PM +0000, Ni, Ray wrote:
> > > Gerd,
> > > All the 4 are needed to make sure SMM still works.
> >
> > The new way (HOB present) of handling SMM init will surely require the
> > whole patch series being applied.
> >
> > For the old way (HOB not present) of handling SMM init should be
> > possible to split up into smaller pieces, which each step being
> > individually testable.  Specifically the SMI handler reorganization
> > should be possible to split out, and I think the tiling changes too.
> >
> > These are preparing changes reorganizing the code, once they are in
> > place the final patch adding the new code paths to handle the new SMM
> > init should be relatively small.
> >
> > take care,
> >   Gerd