From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web11.1083.1585033321133148200 for ; Tue, 24 Mar 2020 00:02:06 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: star.zeng@intel.com) IronPort-SDR: vVnc4yqKhhYUfm6um7ahfsKgtt6K/im7zgYStZmuG2jv5NDhI7h8cQhzAbbx8IAcb+F3/Q1cUN rRgprP19TrEQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Mar 2020 00:02:00 -0700 IronPort-SDR: o2PTMTyvwpoDDKPNM8xNSKsoLSZQYXsUWiMm0S9Jubr8Qd8+Pf5zYqJ8rxmyg3/QrkLGRp8uAT 1xphkCVBcToQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,299,1580803200"; d="scan'208";a="265047956" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga002.jf.intel.com with ESMTP; 24 Mar 2020 00:01:58 -0700 Received: from fmsmsx125.amr.corp.intel.com (10.18.125.40) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 24 Mar 2020 00:01:58 -0700 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by FMSMSX125.amr.corp.intel.com (10.18.125.40) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 24 Mar 2020 00:01:58 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.50]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.232]) with mapi id 14.03.0439.000; Tue, 24 Mar 2020 15:01:56 +0800 From: "Zeng, Star" 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 Thread-Topic: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval Thread-Index: AQHWAaYyyYUiGMwACUeoBWcSVc8fG6hXUGrg Date: Tue, 24 Mar 2020 07:01:55 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB048310405D4087@shsmsx102.ccr.corp.intel.com> References: <20200324063328.8812-1-hao.a.wu@intel.com> In-Reply-To: <20200324063328.8812-1-hao.a.wu@intel.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: star.zeng@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable The code logic is good to me. Only minor concern, do we really need the PCD to be UINT64 type? :) 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 ; Dong, Eric ; Ni= , > Ray ; Laszlo Ersek ; Kinney, Michae= l > D ; Zeng, Star ; Brian = J . > Johnson > Subject: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to > control AP status check interval >=20 > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2627 >=20 > 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(). >=20 > 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. >=20 > 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 w= hen > the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait for A= P(s) > to complete the task, especially when the task can be finished considera= bly > fast on AP(s). >=20 > 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. >=20 > 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. >=20 > For least impact, the default value of the new PCD will be the same with= the > current interval value, which is 100 milliseconds. >=20 > Unitest done: > OS boot successfully. >=20 > 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 > --- > 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(-) >=20 > 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 >=20 > gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000| > UINT64|0x32132113 >=20 > + ## 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 @@ >=20 > #include >=20 > -#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (100)) > #define AP_SAFE_STACK_SIZE 128 >=20 > CPU_MP_DATA *mCpuMpData =3D NULL; > @@ -451,7 +450,7 @@ InitMpGlobalData ( > Status =3D gBS->SetTimer ( > mCheckAllApsEvent, > TimerPeriodic, > - AP_CHECK_INTERVAL > + PcdGet64 (PcdCpuApStatusCheckInterval) > ); > ASSERT_EFI_ERROR (Status); >=20 > 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. > +// Copyright (c) 2007 - 2020, Intel Corporation. All rights > +reserved.
> // > // 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" >=20 > #string > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HE > LP #language en-US "This value used to specify the count of pre alloc= ated > SMM MP tokens per chunk.\n" > + > +#string > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_PROMPT > #language en-US "Periodic interval value in milliseconds for AP status c= heck 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 >=20 >=20 >=20