> -----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. 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/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.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.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 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56137): https://edk2.groups.io/g/devel/message/56137 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] -=-=-=-=-=-=-=-=-=-=-=-
