[edk2-devel] [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
Laszlo Ersek
lersek at redhat.com
Fri Oct 11 21:31:24 UTC 2019
On 10/11/19 10:22, Ni, Ray wrote:
> Laszlo, the comments couldn't be better! Thanks!!
>
> Reviewed-by: Ray Ni <ray.ni at intel.com>
Thanks, Ray :)
Series pushed as commit range a7e2d20193e8..778832bcad33.
Cheers!
Laszlo
>
>> -----Original Message-----
>> From: Laszlo Ersek <lersek at redhat.com>
>> Sent: Thursday, October 10, 2019 7:30 PM
>> To: edk2-devel-groups-io <devel at edk2.groups.io>
>> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at 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.dong at intel.com>
>> Cc: Ray Ni <ray.ni at intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>> Signed-off-by: Laszlo Ersek <lersek at 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 (#48854): https://edk2.groups.io/g/devel/message/48854
Mute This Topic: https://groups.io/mt/34473663/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list