Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval

["Zeng, Star" <star.zeng@intel.com>]

The code logic is good to me.
Only minor concern, do we really need the PCD to be UINT64 type? :)

Thanks,
Star

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Wu, Hao A
> Sent: Tuesday, March 24, 2020 2:33 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: [edk2-devel] [PATCH v2] 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.
> 
> For least impact, the default value of the new PCD will be the same with the
> current interval value, which is 100 milliseconds.
> 
> Unitest done:
> OS boot successfully.
> 
> 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>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec                     | 6 ++++++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 2 +-
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 3 +--
>  UefiCpuPkg/UefiCpuPkg.uni                     | 5 ++++-
>  4 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index
> e91dc68cbe..06a3d52060 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 milliseconds for the
> + status check  #  of APs for StartupAllAPs() and StartupThisAP()
> + executed in non-blocking  #  mode in DXE phase.
> +  # @Prompt Periodic interval value in milliseconds for AP status check in
> DXE.
> +
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x
> 000
> + 0001E
> +
>  [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..05e004dc3c 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -69,5 +69,5 @@ [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval            ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> CONSUMES
> -
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index a987c32109..683547dc99 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,7 @@ InitMpGlobalData (
>    Status = gBS->SetTimer (
>                    mCheckAllApsEvent,
>                    TimerPeriodic,
> -                  AP_CHECK_INTERVAL
> +                  PcdGet64 (PcdCpuApStatusCheckInterval)
>                    );
>    ASSERT_EFI_ERROR (Status);
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni index
> c0d6ed5136..08d9b45f76 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_PR
> OMPT  #language en-US "Specify the count of pre allocated SMM MP tokens
> per chunk.\n"
> 
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HE
> LP    #language en-US "This value used to specify the count of pre allocated
> SMM MP tokens per chunk.\n"
> +
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_PROMPT
> #language en-US "Periodic interval value in milliseconds for AP status check in
> DXE.\n"
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_HELP
> #language en-US "Periodic interval value in milliseconds for the status check
> of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
> mode in DXE phase.\n"
> --
> 2.12.0.windows.1
> 
> 
>