On 02/28/20 04:05, Dong, Eric wrote:
> Hi Laszlo,
>
> Thanks for your patch. The change make sense base on the comments in the data
> structure header file.
>
> I also checked all the code related to this data structure. The inputs for
> this data structure are CpuS3DataDxe and RegisterCpuFeaturesLib. Both these
> two drivers not support CPU hot plug feature, so the real inputs for
> mAcpiCpuData.NumberOfCpus is the enabled CPU number in this system. So before
> and after your code change, the CPU values are same. But the data structure
> comments said it can support CPU hot plug, so I agree your code change.
>
> Reviewed-by: Eric Dong <eric.d...@intel.com>
Thank you!
Please allow me one additional comment on CpuS3DataDxe however:
According to the comments in UefiCpuPkg/CpuS3DataDxe:
[...] this module does not support hot plug CPUs. This module can
be copied into a CPU specific package and customized if these
additional features are required [...]
in the last three patches of the series, I clone CpuS3DataDxe for
OvmfPkg, and extend it for CPU hotplug:
[edk2-devel] [PATCH v2 14/16]
OvmfPkg: clone CpuS3DataDxe from UefiCpuPkg
[edk2-devel] [PATCH v2 15/16]
OvmfPkg/CpuS3DataDxe: superficial cleanups
[edk2-devel] [PATCH v2 16/16]
OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug
(This extension occurs in a QEMU-specific way -- in other words,
OvmfPkg/CpuS3DataDxe is really a platform driver.)
What I'm trying to say is: the PiSmmCpuDxeSmm changes from the present
patch *are* utilized in a CPU hotplug situation too.
In other words, PiSmmCpuDxeSmm is really exposed to a situation where
the following expression is TRUE:
(FeaturePcdGet (PcdCpuHotPlugSupport) &&
mNumberOfCpus < mAcpiCpuData.NumberOfCpus)
It is testable with OVMF, after this series is applied.
Thanks
Laszlo
>
> Thanks,
> Eric
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Thursday, February 27, 2020 6:12 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>; Dong, Eric
> <eric.d...@intel.com>; Igor Mammedov <imamm...@redhat.com>; Yao, Jiewen
> <jiewen....@intel.com>; Justen, Jordan L <jordan.l.jus...@intel.com>; Kinney,
> Michael D <michael.d.kin...@intel.com>; Philippe Mathieu-Daudé
> <phi...@redhat.com>; Ni, Ray <ray...@intel.com>
> Subject: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3
> Resume for CPU hotplug
>
> 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.biesheu...@linaro.org>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Igor Mammedov <imamm...@redhat.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Michael Kinney <michael.d.kin...@intel.com>
> Cc: Philippe Mathieu-Daudé <phi...@redhat.com>
> Cc: Ray Ni <ray...@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheu...@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 (#55080): https://edk2.groups.io/g/devel/message/55080
Mute This Topic: https://groups.io/mt/71575170/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
