Re: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection

["Ni, Ray" <ray.ni@intel.com>]

Laszlo, the comments couldn't be better! Thanks!!

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, October 10, 2019 7:30 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@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@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
> Signed-off-by: Laszlo Ersek <lersek@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