[edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug

[Laszlo Ersek]

The "ACPI_CPU_DATA.NumberOfCpus" field is specified as follows, in
"UefiCpuPkg/Include/AcpiCpuData.h" (rewrapped for this commit message):

  //
  // The number of CPUs.  If a platform does not support hot plug CPUs,
  // then this is the number of CPUs detected when the platform is booted,
  // regardless of being enabled or disabled.  If a platform does support
  // hot plug CPUs, then this is the maximum number of CPUs that the
  // platform supports.
  //

The InitializeCpuBeforeRebase() and InitializeCpuAfterRebase() functions
in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c" try to restore CPU configuration on
the S3 Resume path for *all* CPUs accounted for in
"ACPI_CPU_DATA.NumberOfCpus". This is wrong, as with CPU hotplug, not all
of the possible CPUs may be present at the time of S3 Suspend / Resume.
The symptom is an infinite wait.

Instead, the "mNumberOfCpus" variable should be used, which is properly
maintained through the EFI_SMM_CPU_SERVICE_PROTOCOL implementation (see
SmmAddProcessor(), SmmRemoveProcessor(), SmmCpuUpdate() in
"UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c").

When CPU hotplug is disabled, "mNumberOfCpus" is constant, and equals
"ACPI_CPU_DATA.NumberOfCpus" at all times.

Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
Cc: Eric Dong <eric.dong at intel.com>
Cc: Igor Mammedov <imammedo at redhat.com>
Cc: Jiewen Yao <jiewen.yao at intel.com>
Cc: Jordan Justen <jordan.l.justen at intel.com>
Cc: Michael Kinney <michael.d.kinney at intel.com>
Cc: Philippe Mathieu-Daudé <philmd at redhat.com>
Cc: Ray Ni <ray.ni at intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
---

Notes:
    v2:
    
    - Pick up Ard's Acked-by, which is conditional on approval from Intel
      reviewers on Cc. (I'd like to save Ard the churn of re-acking
      unmodified patches.)

 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index ba5cc0194c2d..1e0840119724 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -597,75 +597,85 @@ PrepareApStartupVector (
 }
 
 /**
   The function is invoked before SMBASE relocation in S3 path to restores CPU status.
 
   The function is invoked before SMBASE relocation in S3 path. It does first time microcode load
   and restores MTRRs for both BSP and APs.
 
 **/
 VOID
 InitializeCpuBeforeRebase (
   VOID
   )
 {
   LoadMtrrData (mAcpiCpuData.MtrrTable);
 
   SetRegister (TRUE);
 
   ProgramVirtualWireMode ();
 
   PrepareApStartupVector (mAcpiCpuData.StartupVector);
 
-  mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
+  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
+  } else {
+    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
+  }
+  mNumberToFinish = mNumberOfCpus - 1;
   mExchangeInfo->ApFunction  = (VOID *) (UINTN) InitializeAp;
 
   //
   // Execute code for before SmmBaseReloc. Note: This flag is maintained across S3 boots.
   //
   mInitApsAfterSmmBaseReloc = FALSE;
 
   //
   // Send INIT IPI - SIPI to all APs
   //
   SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
 
   while (mNumberToFinish > 0) {
     CpuPause ();
   }
 }
 
 /**
   The function is invoked after SMBASE relocation in S3 path to restores CPU status.
 
   The function is invoked after SMBASE relocation in S3 path. It restores configuration according to
   data saved by normal boot path for both BSP and APs.
 
 **/
 VOID
 InitializeCpuAfterRebase (
   VOID
   )
 {
-  mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
+  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
+  } else {
+    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
+  }
+  mNumberToFinish = mNumberOfCpus - 1;
 
   //
   // Signal that SMM base relocation is complete and to continue initialization for all APs.
   //
   mInitApsAfterSmmBaseReloc = TRUE;
 
   //
   // Must begin set register after all APs have continue their initialization.
   // This is a requirement to support semaphore mechanism in register table.
   // Because if semaphore's dependence type is package type, semaphore will wait
   // for all Aps in one package finishing their tasks before set next register
   // for all APs. If the Aps not begin its task during BSP doing its task, the
   // BSP thread will hang because it is waiting for other Aps in the same
   // package finishing their task.
   //
   SetRegister (FALSE);
 
   while (mNumberToFinish > 0) {
     CpuPause ();
   }
 }
 
-- 
2.19.1.3.g30247aa5d201



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54946): https://edk2.groups.io/g/devel/message/54946
Mute This Topic: https://groups.io/mt/71575170/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-