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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, March 26, 2020 3:24 AM
> To: devel@edk2.groups.io; Wu, Hao A
> Cc: Dong, Eric; Ni, Ray; Kinney, Michael D; Zeng, Star; Brian J . Johnson
> Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to
> control AP status check interval
> 
> On 03/25/20 06:07, Wu, Hao A wrote:
> > 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|U
> INT64|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|1
> 00000|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_PRO
> MPT  #language en-US "Specify the count of pre allocated SMM MP tokens per
> chunk.\n"
> >
> >  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HEL
> P    #language en-US "This value used to specify the count of pre allocated SMM
> MP tokens per chunk.\n"
> > +
> > +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSecon
> ds_PROMPT  #language en-US "Periodic interval value in microseconds for AP
> status check in DXE.\n"
> > +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSecon
> ds_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"
> >
> 
> For this patch:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> I'm happy if this patch is pushed, as-is.


Thanks for the review effort.


> 
> Independently, I have found that MpInitLib, despite using several PCDs,
> does not list the PcdLib class in the [LibraryClasses] section, and
> never #include's <Library/PcdLib.h>  either.
> 
> (The [LibraryClasses] observation applies to both INF files, that is,
> PEI and DXE alike.)
> 
> Things happen to work in practice because both the #include and the lib
> class dependency must be inherited from other headers / lib instances.
> But it would be prudent to spell out the PcdLib dependency, especially
> because some of the subject PCDs are used with the dynamic access
> method, in practice (for example by OvmfPkg) -- meaning that the actual
> PCD library APIs are exercised.
> 
> Hao, can you post a followup patch for that, or do you prefer that we
> only enter a BZ ticket at the moment?


I have filed a BZ tracker for the issue you mentioned:
https://bugzilla.tianocore.org/show_bug.cgi?id=2632

Let us address this in a separate series.

Best Regards,
Hao Wu


> 
> Thanks!
> Laszlo
> 
> 
>