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            |  31 ++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        |  25 +++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 166 ++++++++++++++++++++++-----
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  26 ++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   3 +-
 5 files changed, 214 insertions(+), 37 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index fb4a44eab6..d408b3f9f7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -1,9 +1,9 @@
 /** @file
 Code for Processor S3 restoration
 
-Copyright (c) 2006 - 2022, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2023, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
 #include "PiSmmCpuDxeSmm.h"
@@ -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..baf827cf9d 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1,9 +1,9 @@
 /** @file
 SMM MP service implementation
 
-Copyright (c) 2009 - 2022, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.<BR>
 Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -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 6e795d1756..12e2d1579b 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -82,10 +82,12 @@ EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL  mSmmMemoryAttribute = {
   EdkiiSmmClearMemoryAttributes
 };
 
 EFI_CPU_INTERRUPT_HANDLER  mExternalVectorTable[EXCEPTION_VECTOR_NUMBER];
 
+BOOLEAN           mSmmRelocated    = FALSE;
+volatile BOOLEAN  *mSmmInitialized = NULL;
 UINT32            mBspApicId       = 0;
 
 //
 // SMM stack information
 //
@@ -381,22 +383,69 @@ SmmInitHandler (
         // 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 ((VOID *)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]) {
+    }
+  }
+}
+
 /**
   Relocate SmmBases for each processor.
 
   Execute on first boot and all S3 resumes
 
@@ -560,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 ();
@@ -788,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.
-  //
-  // 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) {
+    //
+    // 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);
@@ -842,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) {
@@ -955,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
@@ -996,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..c3731f174b 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1,9 +1,9 @@
 /** @file
 Agent Module to load other modules to deploy SMM Entry Vector for X86 CPU.
 
-Copyright (c) 2009 - 2022, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.<BR>
 Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -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 volatile  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..9bfa8c1a76 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -2,11 +2,11 @@
 # CPU SMM driver.
 #
 # This SMM driver performs SMM initialization, deploy SMM Entry Vector,
 # provides CPU specific services in SMM.
 #
-# Copyright (c) 2009 - 2022, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.<BR>
 # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100065): https://edk2.groups.io/g/devel/message/100065
Mute This Topic: https://groups.io/mt/96932003/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> +  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;
> +    }

Ok, so TileSize is what the firmware needs to store code and state.
Where does the SIZE_8KB come from?  I assume this is the amount of
per-cpu memory allocated by the PEI module?  Shouldn't this be passed
in the HOB instead of being hard-coded?

> +    // 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));

I think correct is:
	BufferPages = EFI_SIZE_TO_PAGES(TileSize * mMaxNumberOfCpus);

> +    if ((FamilyId == 4) || (FamilyId == 5)) {
> +      Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);

Does that actually matter still?  I'm pretty sure we can safely use
"ASSERT(FamilyId > 5)" here.  Pentium processors have been built in
the last century, predating x64.

Beside that the code is broken for SMP, only cpu0 will get a properly
aligned smbase.  Not sure penium processors support SMP in the first
place though ...

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

For Index = 0 this evaluates to "Buffer - SMM_HANDLER_OFFSET", which looks
wrong to me.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100083): https://edk2.groups.io/g/devel/message/100083
Mute This Topic: https://groups.io/mt/96932003/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Gerd,


> 
> Ok, so TileSize is what the firmware needs to store code and state.
> Where does the SIZE_8KB come from?  I assume this is the amount of
> per-cpu memory allocated by the PEI module?  Shouldn't this be passed
> in the HOB instead of being hard-coded?
> 

Yes, TileSize is for firmware store code and data, including 3 parts:
1. CPU SMRAM Save State Map starts at SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET(0xfc00),
2. extra CPU specific context start starts at SMBASE + SMM_PSD_OFFSET (PROCESSOR SMM DESCRIPTO, 0xfb00),
3. SMI entry point starts at SMBASE + SMM_HANDLER_OFFSET (0x8000).
This size is rounded up to nearest power of 2. So, you can refer the below existing algorithm:

  TileCodeSize = GetSmiHandlerSize ();
  TileCodeSize = ALIGN_VALUE (TileCodeSize, SIZE_4KB);
  TileDataSize = (SMRAM_SAVE_STATE_MAP_OFFSET - SMM_PSD_OFFSET) + sizeof (SMRAM_SAVE_STATE_MAP);
  TileDataSize = ALIGN_VALUE (TileDataSize, SIZE_4KB);
  TileSize     = TileDataSize + TileCodeSize - 1;
  TileSize     = 2 * GetPowerOfTwo32 ((UINT32)TileSize);
  DEBUG ((DEBUG_INFO, "SMRAM TileSize = 0x%08x (0x%08x, 0x%08x)\n", TileSize, TileCodeSize, TileDataSize));

Based on above, we hardcode the size to 8k, because It's almost impossible to exceed 8k for total code & data size. So, there is the hard requirement that SMI Entry Size <= 0x1000, data Size < 0x1000 in pi smm cpu driver. To simplify the usage, we add the size check as below to catch this very little possibility case instead of passing or defining the new interface for that, which means we add such rigorous processed assumption to avoid define the new interface that may not be changed and used. 

In PEI module, it also has such assumption, so we don't pass in the HOB for the resolved smbase mem size, because we have avoided the possibility of error in the reference pi smm cpu driver.

    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;
    }

> > +    // 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));
> 
> I think correct is:
> 	BufferPages = EFI_SIZE_TO_PAGES(TileSize * mMaxNumberOfCpus);
> 

This is the existing code logic & it's correct, not wrong, I don't change it. To understand that, we need understand the algorithm of smbase:

The SIZE_32KB covers the *several* SMI Entry and Save State of CPU 0, while TileSize * (mMaxNumberOfCpus - 1) to cover Save State of CPU 1+, not include the cpu0, so, it's the mMaxNumberOfCpus - 1. 


> > +    if ((FamilyId == 4) || (FamilyId == 5)) {
> > +      Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);
> 
> Does that actually matter still?  I'm pretty sure we can safely use
> "ASSERT(FamilyId > 5)" here.  Pentium processors have been built in
> the last century, predating x64.
> 

This is the existing code logic. I don't change it. If you think we don't need it, please file Bugzilla for change.

> Beside that the code is broken for SMP, only cpu0 will get a properly
> aligned smbase.  Not sure penium processors support SMP in the first
> place though ...
> 

I don't understand why "only cpu0 will get a properly aligned smbase"


> >    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;
> 
> For Index = 0 this evaluates to "Buffer - SMM_HANDLER_OFFSET", which
> looks
> wrong to me.
> 

No, it's correct, we don't allocate the buffer for [smbase 0, smbase + smi handler), we just record the address of smbase 0, there is no need for the   [smbase 0, smbase + smi handler) usage.


Thanks,
Jiaxin




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100151): https://edk2.groups.io/g/devel/message/100151
Mute This Topic: https://groups.io/mt/96932003/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> In PEI module, it also has such assumption, so we don't pass in the
> HOB for the resolved smbase mem size, because we have avoided the
> possibility of error in the reference pi smm cpu driver.

So you essentially are hoping this will never ever change and hard-code
the 8k in both PEI module and PiSmmCpuDxeSmm.  I'd suggest to add a
field to the HOB struct instead.  If you want stick to the hardcoded 8k
please add a note saying so to the HOB struct description.

> > > +    BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize *
> > (mMaxNumberOfCpus - 1));
> > 
> > I think correct is:
> > 	BufferPages = EFI_SIZE_TO_PAGES(TileSize * mMaxNumberOfCpus);
> > 
> 
> This is the existing code logic & it's correct, not wrong, I don't change it. To understand that, we need understand the algorithm of smbase:
> 
> The SIZE_32KB covers the *several* SMI Entry and Save State of CPU 0, while TileSize * (mMaxNumberOfCpus - 1) to cover Save State of CPU 1+, not include the cpu0, so, it's the mMaxNumberOfCpus - 1. 

Ok, there is a longish comment in the source code explaining the tiling
(starting at line 639).

smram is 64k (16 pages), with pages 0-7 being unused, page 8 being the
smi handler, 9-14 unused again, page 15 holding cpu state.  Due to the
smi handler having an even page index and the cpu state page having a
odd page index you can use that tiling trick so you need only 8k not 32k
per additional cpu.

I agree, the existing code is correct.

> > > +    if ((FamilyId == 4) || (FamilyId == 5)) {
> > > +      Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);
> > 
> > Does that actually matter still?  I'm pretty sure we can safely use
> > "ASSERT(FamilyId > 5)" here.  Pentium processors have been built in
> > the last century, predating x64.
> 
> This is the existing code logic. I don't change it. If you think we don't need it, please file Bugzilla for change.
> 
> > Beside that the code is broken for SMP, only cpu0 will get a properly
> > aligned smbase.  Not sure penium processors support SMP in the first
> > place though ...
> 
> I don't understand why "only cpu0 will get a properly aligned smbase"

When the cpu expects the smbase being 32k-aligned (as the comment in the
code explains) the tiling trick just doesn't work.  The whole buffer and
the smbase for cpu0 are properly aligned to 32k, but the smbase for cpu1
is not.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100166): https://edk2.groups.io/g/devel/message/100166
Mute This Topic: https://groups.io/mt/96932003/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> 
> So you essentially are hoping this will never ever change and hard-code
> the 8k in both PEI module and PiSmmCpuDxeSmm.  I'd suggest to add a

Yes, 8k is bigger than the real usage case.

> field to the HOB struct instead.  If you want stick to the hardcoded 8k
> please add a note saying so to the HOB struct description.
> 

I will add the reserved mem requirement for each cpu.





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100232): https://edk2.groups.io/g/devel/message/100232
Mute This Topic: https://groups.io/mt/96932003/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-