Laszlo, the comments couldn't be better! Thanks!! Reviewed-by: Ray Ni <ray...@intel.com>
> -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Thursday, October 10, 2019 7:30 PM > To: edk2-devel-groups-io <devel@edk2.groups.io> > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com> > Subject: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot > CPU count in AP detection > > - If a platform boots such that the boot CPU count is smaller than > PcdCpuMaxLogicalProcessorNumber, then the platform cannot use the > "fast > AP detection" logic added in commit 6e1987f19af7. (Which has been > documented as a subset of use case (2) in the previous patch.) > > Said logic depends on the boot CPU count being equal to > PcdCpuMaxLogicalProcessorNumber. If the equality does not hold, the > platform either has to wait too long, or risk missing APs due to an > early timeout. > > - The platform may not be able to use the variant added in commit > 0594ec417c89 either. (Which has been documented as use case (1) in the > previous patch.) > > See commit 861218740d6d. When OVMF runs on QEMU/KVM, APs may > check in > with the BSP in arbitrary order, plus the individual AP may take > arbitrarily long to check-in. If "NumApsExecuting" falls to zero > mid-enumeration, APs will be missed. > > Allow platforms to specify the exact boot CPU count, independently of > PcdCpuMaxLogicalProcessorNumber. In this mode, the BSP waits for all APs > to check-in regardless of timeout. If at least one AP fails to check-in, then > the > AP enumeration hangs forever. That is the desired behavior when the exact > boot CPU count is known in advance. (A hung boot is better than an AP > checking-in after timeout, and executing code from released > storage.) > > Cc: Eric Dong <eric.d...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515 > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > > Notes: > v2: > - update commit message > - update docs in DEC file > - add code comments > - no functional changes > > UefiCpuPkg/UefiCpuPkg.dec | 13 +++ > UefiCpuPkg/UefiCpuPkg.uni | 4 + > UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 + > UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 3 +- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 99 ++++++++++++-------- > 5 files changed, 80 insertions(+), 40 deletions(-) > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec > index 031a2ccd680a..12f4413ea5b0 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dec > +++ b/UefiCpuPkg/UefiCpuPkg.dec > @@ -227,6 +227,19 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, > PcdsDynamic, PcdsDynamicEx] > ## Specifies timeout value in microseconds for the BSP to detect all APs > for > the first time. > # @Prompt Timeout for the BSP to detect all APs for the first time. > > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000| > UINT32|0x00000004 > + ## Specifies the number of Logical Processors that are available in > + the # preboot environment after platform reset, including BSP and > + APs. Possible # values:<BR><BR> # zero (default) - > + PcdCpuBootLogicalProcessorNumber is ignored, and > + # PcdCpuApInitTimeOutInMicroSeconds limits the initial AP > + # detection by the BSP.<BR> > + # nonzero - PcdCpuApInitTimeOutInMicroSeconds is ignored. The > initial > + # AP detection finishes only when the detected CPU count > + # (BSP plus APs) reaches the value of > + # PcdCpuBootLogicalProcessorNumber, regardless of how > long > + # that takes.<BR> > + # @Prompt Number of Logical Processors available after platform reset. > + > + > gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT > 32|0x > + 00000008 > ## Specifies the base address of the first microcode Patch in the microcode > Region. > # @Prompt Microcode Region base address. > > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64| > 0x00000005 > diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni index > fbf768072668..a7e279c5cb14 100644 > --- a/UefiCpuPkg/UefiCpuPkg.uni > +++ b/UefiCpuPkg/UefiCpuPkg.uni > @@ -37,6 +37,10 @@ > > #string > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApInitTimeOutInMicroSeconds_ > HELP #language en-US "Specifies timeout value in microseconds for the BSP > to detect all APs for the first time." > > +#string > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_PR > OMPT #language en-US "Number of Logical Processors available after > platform reset." > + > +#string > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_HE > LP #language en-US "Specifies the number of Logical Processors that are > available in the preboot environment after platform reset, including BSP and > APs." > + > #string > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_PROMP > T #language en-US "Microcode Region base address." > > #string > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_HELP > #language en-US "Specifies the base address of the first microcode Patch in > the microcode Region." > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > index 37b3f64e578a..cd912ab0c5ee 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > @@ -61,6 +61,7 @@ [Guids] > > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## > CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber ## > CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## > SOMETIMES_CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## > CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## > CONSUMES > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > index 82b77b63ea87..1538185ef99a 100644 > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > @@ -53,7 +53,8 @@ [LibraryClasses] > > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## > CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber ## > CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## > SOMETIMES_CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## > CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## > CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ## > CONSUMES > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 594a035d8b92..622b70ca3c4e 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1044,46 +1044,67 @@ WakeUpAP ( > SendInitSipiSipiAllExcludingSelf ((UINT32) ExchangeInfo->BufferStart); > } > if (CpuMpData->InitFlag == ApInitConfig) { > - // > - // The AP enumeration algorithm below is suitable for two use cases. > - // > - // (1) The check-in time for an individual AP is bounded, and APs run > - // through their initialization routines strongly concurrently. In > - // particular, the number of concurrently running APs > - // ("NumApsExecuting") is never expected to fall to zero > - // *temporarily* -- it is expected to fall to zero only when all > - // APs have checked-in. > - // > - // In this case, the platform is supposed to set > - // PcdCpuApInitTimeOutInMicroSeconds to a low-ish value (just long > - // enough for one AP to start initialization). The timeout will be > - // reached soon, and remaining APs are collected by watching > - // NumApsExecuting fall to zero. If NumApsExecuting falls to zero > - // mid-process, while some APs have not completed initialization, > - // the behavior is undefined. > - // > - // (2) The check-in time for an individual AP is unbounded, and/or APs > - // may complete their initializations widely spread out. In > - // particular, some APs may finish initialization before some APs > - // even start. > - // > - // In this case, the platform is supposed to set > - // PcdCpuApInitTimeOutInMicroSeconds to a high-ish value. The AP > - // enumeration will always take that long (except when the boot CPU > - // count happens to be maximal, that is, > - // PcdCpuMaxLogicalProcessorNumber). All APs are expected to > - // check-in before the timeout, and NumApsExecuting is assumed zero > - // at timeout. APs that miss the time-out may cause undefined > - // behavior. > - // > - TimedWaitForApFinish ( > - CpuMpData, > - PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1, > - PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds) > - ); > + if (PcdGet32 (PcdCpuBootLogicalProcessorNumber) > 0) { > + // > + // The AP enumeration algorithm below is suitable only when the > + // platform can tell us the *exact* boot CPU count in advance. > + // > + // The wait below finishes only when the detected AP count reaches > + // (PcdCpuBootLogicalProcessorNumber - 1), regardless of how long > that > + // takes. If at least one AP fails to check in (meaning a platform > + // hardware bug), the detection hangs forever, by design. If the > actual > + // boot CPU count in the system is higher than > + // PcdCpuBootLogicalProcessorNumber (meaning a platform > + // misconfiguration), then some APs may complete initialization after > + // the wait finishes, and cause undefined behavior. > + // > + TimedWaitForApFinish ( > + CpuMpData, > + PcdGet32 (PcdCpuBootLogicalProcessorNumber) - 1, > + MAX_UINT32 // approx. 71 minutes > + ); > + } else { > + // > + // The AP enumeration algorithm below is suitable for two use cases. > + // > + // (1) The check-in time for an individual AP is bounded, and APs run > + // through their initialization routines strongly concurrently. > In > + // particular, the number of concurrently running APs > + // ("NumApsExecuting") is never expected to fall to zero > + // *temporarily* -- it is expected to fall to zero only when all > + // APs have checked-in. > + // > + // In this case, the platform is supposed to set > + // PcdCpuApInitTimeOutInMicroSeconds to a low-ish value (just > long > + // enough for one AP to start initialization). The timeout will > be > + // reached soon, and remaining APs are collected by watching > + // NumApsExecuting fall to zero. If NumApsExecuting falls to zero > + // mid-process, while some APs have not completed initialization, > + // the behavior is undefined. > + // > + // (2) The check-in time for an individual AP is unbounded, and/or > APs > + // may complete their initializations widely spread out. In > + // particular, some APs may finish initialization before some APs > + // even start. > + // > + // In this case, the platform is supposed to set > + // PcdCpuApInitTimeOutInMicroSeconds to a high-ish value. The AP > + // enumeration will always take that long (except when the boot > CPU > + // count happens to be maximal, that is, > + // PcdCpuMaxLogicalProcessorNumber). All APs are expected to > + // check-in before the timeout, and NumApsExecuting is assumed > zero > + // at timeout. APs that miss the time-out may cause undefined > + // behavior. > + // > + TimedWaitForApFinish ( > + CpuMpData, > + PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1, > + PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds) > + ); > > - while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) { > - CpuPause(); > + while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) { > + CpuPause(); > + } > } > } else { > // > -- > 2.19.1.3.g30247aa5d201 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48806): https://edk2.groups.io/g/devel/message/48806 Mute This Topic: https://groups.io/mt/34473663/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
