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

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

> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, March 25, 2020 8:46 AM
> To: Kinney, Michael D; Zeng, Star; Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek; Brian J . Johnson
> Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
> control AP status check interval
> 
> Mike, Hao,
> I searched all edk2 code using regex pattern "Pcd.*Timeout.*", "Pcd.*Timer.*",
> "Pcd.*Delay.*"
> and found below PCDs:
> 
> gEfiMdePkgTokenSpaceGuid.PcdSpinLockTimeout: in tick unit depending on the
> TimerLib used
> gEfiMdePkgTokenSpaceGuid.PcdUsbTransferTimeoutValue: in 1ms unit
> gPcAtChipsetPkgTokenSpaceGuid.PcdHpetDefaultTimerPeriod: in 100ns unit
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdSataSpinUpDelayInSecForRecoveryPat
> h: in 1s unit
> gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds: in 1us unit
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout: in 1us unit
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbXhciDebugDetectTimeout: in
> 1us unit
> 
> 1 uses timer ticks as unit.
> 1 uses as unit.
> 1 uses ms as unit.
> 3 use us as unit.
> 1 uses 100ns as unit.
> 
> UefiCpuPkg uses us consistently as the unit.
> So how about using us for this PCD as well?
> And don't forget to name the PCD as
> "PcdCpuApStatusCheckIntervalInMicroSeconds" so developers who set the PCD
> can be aware by reading the name.


Hello Ray,

Agree with your suggestion.
I will follow up with this approach and provide a new version of the patch.

Best Regards,
Hao Wu


> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Tuesday, March 24, 2020 11:59 PM
> > To: Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>;
> > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Brian J . Johnson <brian.johnson@hpe.com>
> > Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
> control AP status check interval
> >
> > How was the milliseconds units selected?
> >
> > We have other PCDs that provide timer intervals in
> > 100ns unit and my microseconds.  I would prefer we
> > be consistent so platform developers do not have
> > to keep track of so many different units for time
> > intervals.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Tuesday, March 24, 2020 12:53 AM
> > > To: Zeng, Star <star.zeng@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; devel@edk2.groups.io
> > > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Brian J . Johnson
> > > <brian.johnson@hpe.com>
> > > Subject: RE: [edk2-devel] [PATCH v2]
> > > UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> > > check interval
> > >
> > > From the perspective of PCD producer, I agree with Star
> > > to use UINT32.
> > > Internal implementation can be changed to use other
> > > polling methods which don't require UINT64 data type.
> > >
> > > > -----Original Message-----
> > > > From: Zeng, Star <star.zeng@intel.com>
> > > > Sent: Tuesday, March 24, 2020 3:29 PM
> > > > To: Wu, Hao A <hao.a.wu@intel.com>;
> > > devel@edk2.groups.io
> > > > Cc: 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>; Brian J . Johnson
> > > <brian.johnson@hpe.com>; Zeng, Star
> > > <star.zeng@intel.com>
> > > > Subject: RE: [edk2-devel] [PATCH v2]
> > > UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> > > check interval
> > > >
> > > > > -----Original Message-----
> > > > > From: Wu, Hao A
> > > > > Sent: Tuesday, March 24, 2020 3:12 PM
> > > > > To: Zeng, Star <star.zeng@intel.com>;
> > > devel@edk2.groups.io
> > > > > Cc: 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>; Brian J . Johnson
> > > <brian.johnson@hpe.com>
> > > > > Subject: RE: [edk2-devel] [PATCH v2]
> > > UefiCpuPkg/MpInitLib DXE: Add PCD to
> > > > > control AP status check interval
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zeng, Star
> > > > > > Sent: Tuesday, March 24, 2020 3:02 PM
> > > > > > To: devel@edk2.groups.io; Wu, Hao A
> > > > > > Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney,
> > > Michael D; Brian J .
> > > > > > Johnson; Zeng, Star
> > > > > > Subject: RE: [edk2-devel] [PATCH v2]
> > > UefiCpuPkg/MpInitLib DXE: Add PCD
> > > > > > to control AP status check interval
> > > > > >
> > > > > > The code logic is good to me.
> > > > > > Only minor concern, do we really need the PCD to
> > > be UINT64 type? :)
> > > > >
> > > > >
> > > > > Hi Star,
> > > > >
> > > > > I am thinking the UINT64 type fits with the
> > > parameter for the SetTimer()
> > > > > service in EFI_BOOT_SERVICES when preparing the
> > > patch.
> > > > >
> > > > > If there is concern, I can switch it to other type.
> > > >
> > > > Got your good point.
> > > > For me, UINT32 (FFFFFFFFh = 4294967295d = 4294967.295
> > > second = 71582.78825 minutes ~=
> > > > 1193.0464708333333333333333333333 hours) should be
> > > totally enough.
> > > > I do not insist on that if others think UINT64 is ok.
> > > >
> > > > Thanks,
> > > > Star
> > > >
> > > > >
> > > > > Best Regards,
> > > > > Hao Wu
> > > > >
> > > > >
> > > > > >
> > > > > > 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/DxeRegisterCp
> > > uFeaturesLib.
> > > > > > > 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.PcdCpuCoreCrystalClockFrequen
> > > cy|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|1
> > > 00|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.PcdCpuMicrocodePatchRegionSiz
> > > e         ##
> > > > > > > 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_PcdCpuSmmMpTokenCountPerC
> > > hunk_PR
> > > > > > > OMPT  #language en-US "Specify the count of pre
> > > allocated SMM MP
> > > > > > > tokens per chunk.\n"
> > > > > > >
> > > > > > >  #string
> > > > > > >
> > > > >
> > > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerC
> > > hunk_HE
> > > > > > > LP    #language en-US "This value used to
> > > specify the count of pre
> > > > > allocated
> > > > > > > SMM MP tokens per chunk.\n"
> > > > > > > +
> > > > > > > +#string
> > > > > > >
> > > > >
> > > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterv
> > > al_PROMPT
> > > > > > > #language en-US "Periodic interval value in
> > > milliseconds for AP
> > > > > > > status check in DXE.\n"
> > > > > > > +#string
> > > > > > >
> > > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterv
> > > al_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
> > > > > > >
> > > > > > >
> > > > > > >