> -----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.kin...@intel.com> > > Sent: Tuesday, March 24, 2020 11:59 PM > > To: Ni, Ray <ray...@intel.com>; Zeng, Star <star.z...@intel.com>; Wu, Hao A > <hao.a...@intel.com>; > > devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com> > > Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > Brian J . Johnson <brian.john...@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...@intel.com> > > > Sent: Tuesday, March 24, 2020 12:53 AM > > > To: Zeng, Star <star.z...@intel.com>; Wu, Hao A > > > <hao.a...@intel.com>; devel@edk2.groups.io > > > Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek > > > <ler...@redhat.com>; Kinney, Michael D > > > <michael.d.kin...@intel.com>; Brian J . Johnson > > > <brian.john...@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.z...@intel.com> > > > > Sent: Tuesday, March 24, 2020 3:29 PM > > > > To: Wu, Hao A <hao.a...@intel.com>; > > > devel@edk2.groups.io > > > > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray > > > <ray...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > > > Kinney, Michael D > > > > <michael.d.kin...@intel.com>; Brian J . Johnson > > > <brian.john...@hpe.com>; Zeng, Star > > > <star.z...@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.z...@intel.com>; > > > devel@edk2.groups.io > > > > > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray > > > <ray...@intel.com>; Laszlo > > > > > Ersek <ler...@redhat.com>; Kinney, Michael D > > > > > <michael.d.kin...@intel.com>; Brian J . Johnson > > > <brian.john...@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...@intel.com>; Dong, Eric > > > > > > > <eric.d...@intel.com>; Ni, Ray > > > <ray...@intel.com>; Laszlo Ersek > > > > > > > <ler...@redhat.com>; Kinney, Michael D > > > <michael.d.kin...@intel.com>; > > > > > Zeng, Star <star.z...@intel.com>; Brian J . > > > > > > > Johnson <brian.john...@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.d...@intel.com> > > > > > > > Cc: Ray Ni <ray...@intel.com> > > > > > > > Cc: Laszlo Ersek <ler...@redhat.com> > > > > > > > Cc: Michael D Kinney > > > <michael.d.kin...@intel.com> > > > > > > > Cc: Star Zeng <star.z...@intel.com> > > > > > > > Cc: Brian J. Johnson <brian.john...@hpe.com> > > > > > > > Signed-off-by: Hao A Wu <hao.a...@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 > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56230): https://edk2.groups.io/g/devel/message/56230 Mute This Topic: https://groups.io/mt/72512073/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
