Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Tuesday, October 8, 2019 7:27 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Dong, Eric <eric.dong@intel.com>; Igor Mammedov
> <imammedo@redhat.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's
> boot CPU count in AP detection
> 
> If a platform boots with a CPU topology that is not fully populated (that
> is, the boot CPU count is smaller than PcdCpuMaxLogicalProcessorNumber),
> then the platform cannot use the "fast AP detection" logic added in commit
> 6e1987f19af7. Said logic depends on the boot CPU count being equal to
> PcdCpuMaxLogicalProcessorNumber.
> 
> The platform may not be able to use the variant added in commit
> 0594ec417c89 either, for different reasons; see for example commit
> 861218740d6d.
> 
> Allow platforms to specify the exact boot CPU count, independently of
> PcdCpuMaxLogicalProcessorNumber.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.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>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec                     | 11 +++++
>  UefiCpuPkg/UefiCpuPkg.uni                     |  4 ++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  3 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 42 ++++++++++++--------
>  5 files changed, 43 insertions(+), 18 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 031a2ccd680a..d6b33fd9b465 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -227,6 +227,17 @@ [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|UI
> NT32|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) - This PCD is ignored, and
> +  #                   PcdCpuApInitTimeOutInMicroSeconds limits the initial AP
> +  #                   detection by the BSP.<BR>
> +  #  nonzero        - PcdCpuApInitTimeOutInMicroSeconds is ignored. The initial
> +  #                   AP detection finishes when the detected CPU count (BSP
> +  #                   plus APs) reaches the value of this PCD.<BR>
> +  # @Prompt Number of Logical Processors available after platform reset.
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT32
> |0x00000008
>    ## Specifies the base address of the first microcode Patch in the microcode
> Region.
>    # @Prompt Microcode Region base address.
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64|0x
> 00000005
> 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_HEL
> P  #language en-US "Specifies timeout value in microseconds for the BSP to
> detect all APs for the first time."
> 
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_PRO
> MPT  #language en-US "Number of Logical Processors available after platform
> reset."
> +
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_HELP
> #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_PROMPT
> #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 d6f84c6f45c0..f1bf8a7ba7cf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1044,24 +1044,32 @@ WakeUpAP (
>        SendInitSipiSipiAllExcludingSelf ((UINT32) ExchangeInfo->BufferStart);
>      }
>      if (CpuMpData->InitFlag == ApInitConfig) {
> -      //
> -      // Here support two methods to collect AP count through adjust
> -      // PcdCpuApInitTimeOutInMicroSeconds values.
> -      //
> -      // one way is set a value to just let the first AP to start the
> -      // initialization, then through the later while loop to wait all Aps
> -      // finsh the initialization.
> -      // The other way is set a value to let all APs finished the initialzation.
> -      // In this case, the later while loop is useless.
> -      //
> -      TimedWaitForApFinish (
> -        CpuMpData,
> -        PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
> -        PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
> -        );
> +      if (PcdGet32 (PcdCpuBootLogicalProcessorNumber) > 0) {
> +        TimedWaitForApFinish (
> +          CpuMpData,
> +          PcdGet32 (PcdCpuBootLogicalProcessorNumber) - 1,
> +          MAX_UINT32 // approx. 71 minutes

Do you think if the PcdCpuBootLogicalProcessorNumber is not zero, driver should always wait for all the processors specified by PcdCpuBootLogicalProcessorNumber ready? I just have little concern about the MAX_UINT32 usage. I not sure whether there has a case that some processors can't wake up?

Thanks,
Eric

> +          );
> +      } else {
> +        //
> +        // Here support two methods to collect AP count through adjust
> +        // PcdCpuApInitTimeOutInMicroSeconds values.
> +        //
> +        // one way is set a value to just let the first AP to start the
> +        // initialization, then through the later while loop to wait all Aps
> +        // finsh the initialization.
> +        // The other way is set a value to let all APs finished the
> +        // initialzation. In this case, the later while loop is useless.
> +        //
> +        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 (#48563): https://edk2.groups.io/g/devel/message/48563
> Mute This Topic: https://groups.io/mt/34441228/1768733
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [eric.dong@intel.com]
> -=-=-=-=-=-=