Re: [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval

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

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

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Wednesday, March 25, 2020 1:07 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; Brian J .
> Johnson <brian.johnson@hpe.com>
> Subject: [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2627
> 
> The commit will introduce a static PCD to specify the periodic interval
> for checking the AP status when MP services StartupAllAPs() and
> StartupThisAP() are being executed in a non-blocking manner. Or in other
> words, specifies the interval for callback function CheckApsStatus().
> 
> The purpose is to provide the platform owners with the ability to choose
> the proper interval value to trigger CheckApsStatus() according to:
> A) The number of processors in the system;
> B) How MP services (StartupAllAPs & StartupThisAP) being used.
> 
> Setting the PCD to a small value means the AP status check callback will
> be trigerred more frequently, it can benefit the performance for the case
> when the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait
> for AP(s) to complete the task, especially when the task can be finished
> considerably fast on AP(s).
> 
> An example is within function CpuFeaturesInitialize() under
> UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c,
> where BSP will perform the same task with APs and requires all the
> processors to finish the task before BSP proceeds to its next task.
> 
> Setting the PCD to a big value, on the other hand, can reduce the impact
> on BSP by the time being consumed in CheckApsStatus(), especially when the
> number of processors is huge so that the time consumed in CheckApsStatus()
> is not negligible.
> 
> The type of the PCD is UINT32, which means the maximum possible interval
> value can be set to:
> 4,294,967,295 microseconds = 4,295 seconds = 71.58 minutes = 1.19 hours
> which should be sufficient for usage.
> 
> For least impact, the default value of the new PCD will be the same with
> the current interval value. It will be set to 100,000 microseconds, which
> is 100 milliseconds.
> 
> Unitest done:
> A) OS boot successfully;
> B) Use debug message to confirm the 'TriggerTime' parameter for the
>    'SetTimer' service is the same before & after this patch.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Brian J. Johnson <brian.johnson@hpe.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
> 
> Notes:
>  V4
>  Avoiding introducing a local variable in InitMpGlobalData().
> 
>  V3
>  A) Use microseconds, instead of milliseconds as the interval unit;
>  B) Use UINT32, instead of UINT64, for the PCD type;
>  C) Address the bug that incorrect 'TriggerTime' parameter was passed into
>     the 'SetTimer' service call in V2 patch
> 
>  V2
>  Introduce a PCD to specify the AP status check interval.
> 
> 
>  UefiCpuPkg/UefiCpuPkg.dec                     |  6 ++++++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 20 ++++++++++----------
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  5 +++--
>  UefiCpuPkg/UefiCpuPkg.uni                     |  5 ++++-
>  4 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index e91dc68cbe..762badf5d2 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -230,6 +230,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # @Prompt This PCD is the nominal frequency of the core crystal clock in Hz as is CPUID Leaf 0x15:ECX
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|UINT64|0x32132113
> 
> +  ## Specifies the periodic interval value in microseconds for the status check
> +  #  of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
> +  #  mode in DXE phase.
> +  # @Prompt Periodic interval value in microseconds for AP status check in DXE.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds|100000|UINT32|0x0000001E
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    ## Specifies max supported number of Logical Processors.
>    # @Prompt Configure max supported number of Logical Processors
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 45aaa179ff..a51a9ec1d2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -61,13 +61,13 @@ [Guids]
>    gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ## HOB
> 
>  [Pcd]
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ## SOMETIMES_CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ## CONSUMES
> -
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber            ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds          ## SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                          ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress                ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize             ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                       ## SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds  ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index a987c32109..56b776d3d8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -15,7 +15,6 @@
> 
>  #include <Protocol/Timer.h>
> 
> -#define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
>  #define  AP_SAFE_STACK_SIZE    128
> 
>  CPU_MP_DATA      *mCpuMpData = NULL;
> @@ -451,7 +450,9 @@ InitMpGlobalData (
>    Status = gBS->SetTimer (
>                    mCheckAllApsEvent,
>                    TimerPeriodic,
> -                  AP_CHECK_INTERVAL
> +                  EFI_TIMER_PERIOD_MICROSECONDS (
> +                    PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSeconds)
> +                    )
>                    );
>    ASSERT_EFI_ERROR (Status);
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index c0d6ed5136..1780dfdc12 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -3,7 +3,7 @@
>  //
>  // This Package provides UEFI compatible CPU modules and libraries.
>  //
> -// Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
> +// Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
>  //
>  // SPDX-License-Identifier: BSD-2-Clause-Patent
>  //
> @@ -275,3 +275,6 @@
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_PROMPT  #language en-US "Specify the
> count of pre allocated SMM MP tokens per chunk.\n"
> 
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HELP    #language en-US "This value used
> to specify the count of pre allocated SMM MP tokens per chunk.\n"
> +
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSeconds_PROMPT  #language en-US
> "Periodic interval value in microseconds for AP status check in DXE.\n"
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSeconds_HELP    #language en-US
> "Periodic interval value in microseconds for the status check of APs for StartupAllAPs() and StartupThisAP() executed in
> non-blocking mode in DXE phase.\n"
> --
> 2.12.0.windows.1