From 90e11edd16c7a97e3d5fd79f67acaab7c2777d79 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Wed, 26 Feb 2020 23:11:42 +0100 Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Cc: Eric Dong Cc: Igor Mammedov Cc: Jiewen Yao Cc: Jordan Justen Cc: Michael Kinney Cc: Philippe Mathieu-Daudé Cc: Ray Ni Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512 Signed-off-by: Laszlo Ersek Message-Id: <20200226221156.29589-3-lersek@redhat.com> Acked-by: Ard Biesheuvel Reviewed-by: Eric Dong Tested-by: Boris Ostrovsky [lersek@redhat.com: shut up UINTN->UINT32 warning from Windows VS2019 PR] --- 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 ba5cc0194c..29e9ba92b4 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c @@ -616,7 +616,12 @@ InitializeCpuBeforeRebase ( PrepareApStartupVector (mAcpiCpuData.StartupVector); - mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1; + if (FeaturePcdGet (PcdCpuHotPlugSupport)) { + ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus); + } else { + ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus); + } + mNumberToFinish = (UINT32)(mNumberOfCpus - 1); mExchangeInfo->ApFunction = (VOID *) (UINTN) InitializeAp; // @@ -646,7 +651,12 @@ InitializeCpuAfterRebase ( VOID ) { - mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1; + if (FeaturePcdGet (PcdCpuHotPlugSupport)) { + ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus); + } else { + ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus); + } + mNumberToFinish = (UINT32)(mNumberOfCpus - 1); // // Signal that SMM base relocation is complete and to continue initialization for all APs. -- 2.39.2