Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
On 10/11/19 10:22, Ni, Ray wrote: > Laszlo, the comments couldn't be better! Thanks!! > > Reviewed-by: Ray Ni Thanks, Ray :) Series pushed as commit range a7e2d20193e8..778832bcad33. Cheers! Laszlo > >> -Original Message- >> From: Laszlo Ersek >> Sent: Thursday, October 10, 2019 7:30 PM >> To: edk2-devel-groups-io >> Cc: Dong, Eric ; Ni, Ray >> 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 >> Cc: Ray Ni >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515 >> Signed-off-by: Laszlo Ersek >> --- >> >> 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|5| >> UINT32|0x0004 >> + ## Specifies the number of Logical Processors that are available in >> + the # preboot environment after platform reset, including BSP and >> + APs. Possible # values: # zero (default) - >> + PcdCpuBootLogicalProcessorNumber is ignored, and >> + # PcdCpuApInitTimeOutInMicroSeconds limits the initial >> AP >> + # detection by the BSP. >> + # 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. >> + # @Prompt Number of Logical Processors available after platform reset. >> + >> + >> gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT >> 32|0x >> + 0008 >>## Specifies the base address of the first microcode Patch in the >> microcode >> Region. >># @Prompt Microcode Region base address. >> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64| >> 0x0005 >> 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
Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
Laszlo, the comments couldn't be better! Thanks!! Reviewed-by: Ray Ni > -Original Message- > From: Laszlo Ersek > Sent: Thursday, October 10, 2019 7:30 PM > To: edk2-devel-groups-io > Cc: Dong, Eric ; Ni, Ray > 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 > Cc: Ray Ni > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515 > Signed-off-by: Laszlo Ersek > --- > > 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|5| > UINT32|0x0004 > + ## Specifies the number of Logical Processors that are available in > + the # preboot environment after platform reset, including BSP and > + APs. Possible # values: # zero (default) - > + PcdCpuBootLogicalProcessorNumber is ignored, and > + # PcdCpuApInitTimeOutInMicroSeconds limits the initial AP > + # detection by the BSP. > + # 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. > + # @Prompt Number of Logical Processors available after platform reset. > + > + > gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT > 32|0x > + 0008 >## Specifies the base address of the first microcode Patch in the microcode > Region. ># @Prompt Microcode Region base address. > > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64| > 0x0005 > 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_gUefiCpuPkgToke
