From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web10.27837.1585183592565475646 for ; Wed, 25 Mar 2020 17:46:32 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: hao.a.wu@intel.com) IronPort-SDR: yv3byTbvq5G5EcrxLkX+lnRJFItSUlLYTN2wMZZnkj1meKivC4HKSnM4gIA4dd4lyG8MVB1Pgi W5ci6NrNZIYQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Mar 2020 17:46:31 -0700 IronPort-SDR: wCnmOWXwHoQFk+9ydzQX3kgF1WEXg/Vt9NlECId+H66lP4fKjCC/63Ee0JlQC6uilDPxcjQekV o3uoyneIZu6w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,306,1580803200"; d="scan'208";a="250584327" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga006.jf.intel.com with ESMTP; 25 Mar 2020 17:46:31 -0700 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 25 Mar 2020 17:46:31 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 25 Mar 2020 17:46:31 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.206]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.43]) with mapi id 14.03.0439.000; Thu, 26 Mar 2020 08:46:28 +0800 From: "Wu, Hao A" To: "devel@edk2.groups.io" , "lersek@redhat.com" 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 Thread-Topic: [edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval Thread-Index: AQHWAmNJl7XeoyqEpUefCtkQPmAhQahZKyAAgADc00A= Date: Thu, 26 Mar 2020 00:46:27 +0000 Message-ID: References: <20200325050725.9700-1-hao.a.wu@intel.com> <5d7fb00a-29f8-4904-44df-308e05b8c774@redhat.com> In-Reply-To: <5d7fb00a-29f8-4904-44df-308e05b8c774@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----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 . Johnso= n > Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD t= o > control AP status check interval >=20 > On 03/25/20 06:07, Wu, Hao A wrote: > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2627 > > > > The commit will introduce a static PCD to specify the periodic interva= l > > for checking the AP status when MP services StartupAllAPs() and > > StartupThisAP() are being executed in a non-blocking manner. Or in oth= er > > words, specifies the interval for callback function CheckApsStatus(). > > > > The purpose is to provide the platform owners with the ability to choo= se > > 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 wi= ll > > be trigerred more frequently, it can benefit the performance for the c= ase > > when the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wai= t > > for AP(s) to complete the task, especially when the task can be finish= ed > > 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 impa= ct > > on BSP by the time being consumed in CheckApsStatus(), especially when= the > > number of processors is huge so that the time consumed in CheckApsStat= us() > > is not negligible. > > > > The type of the PCD is UINT32, which means the maximum possible interv= al > > value can be set to: > > 4,294,967,295 microseconds =3D 4,295 seconds =3D 71.58 minutes =3D 1.1= 9 hours > > which should be sufficient for usage. > > > > For least impact, the default value of the new PCD will be the same wi= th > > the current interval value. It will be set to 100,000 microseconds, wh= ich > > 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 > > Cc: Ray Ni > > Cc: Laszlo Ersek > > Cc: Michael D Kinney > > Cc: Star Zeng > > Cc: Brian J. Johnson > > Signed-off-by: Hao A Wu > > --- > > > > 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 clo= ck in Hz > as is CPUID Leaf 0x15:ECX > > > gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|U > INT64|0x32132113 > > > > + ## Specifies the periodic interval value in microseconds for the st= atus check > > + # of APs for StartupAllAPs() and StartupThisAP() executed in non-b= locking > > + # mode in DXE phase. > > + # @Prompt Periodic interval value in microseconds for AP status che= ck 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 > > > > -#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (100)) > > #define AP_SAFE_STACK_SIZE 128 > > > > CPU_MP_DATA *mCpuMpData =3D NULL; > > @@ -451,7 +450,9 @@ InitMpGlobalData ( > > Status =3D gBS->SetTimer ( > > mCheckAllApsEvent, > > TimerPeriodic, > > - AP_CHECK_INTERVAL > > + EFI_TIMER_PERIOD_MICROSECONDS ( > > + PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSecon= ds) > > + ) > > ); > > 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.=
> > +// Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.=
> > // > > // 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 p= er > chunk.\n" > > > > #string > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HEL > P #language en-US "This value used to specify the count of pre alloca= ted 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" > > >=20 > For this patch: >=20 > Reviewed-by: Laszlo Ersek >=20 > I'm happy if this patch is pushed, as-is. Thanks for the review effort. >=20 > Independently, I have found that MpInitLib, despite using several PCDs, > does not list the PcdLib class in the [LibraryClasses] section, and > never #include's either. >=20 > (The [LibraryClasses] observation applies to both INF files, that is, > PEI and DXE alike.) >=20 > 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. >=20 > 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=3D2632 Let us address this in a separate series. Best Regards, Hao Wu >=20 > Thanks! > Laszlo >=20 >=20 >=20