From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mx.groups.io with SMTP id smtpd.web10.1527.1585036364289685687 for ; Tue, 24 Mar 2020 00:52:44 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: ray.ni@intel.com) IronPort-SDR: fDZp0Ha7Du3GRM/NYfQ+UR6SzcFPGDPnrD6+Af2eNAkZxQVfWoLNRyEYmC9klKTJWEn0F6Zd+p ALE/hI1YSw3Q== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Mar 2020 00:52:43 -0700 IronPort-SDR: aK8HTnFXvS3CPpy1i8Pez+WSLTaNUub/nKF76DjoYiz+xRKfwlcxsmpKi8nmjZt36ZrAO934Dz 0tcdicTvCREA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,299,1580803200"; d="scan'208,223";a="240191085" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga008.jf.intel.com with ESMTP; 24 Mar 2020 00:52:43 -0700 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) 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:52:43 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 24 Mar 2020 00:52:42 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.206]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.201]) with mapi id 14.03.0439.000; Tue, 24 Mar 2020 15:52:40 +0800 From: "Ni, Ray" To: "Zeng, Star" , "Wu, Hao A" , "devel@edk2.groups.io" CC: "Dong, Eric" , Laszlo Ersek , "Kinney, Michael D" , "Brian J . Johnson" 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: AQHWAaYn0USXVXmyxk+9WUS12oJiP6hWysuAgAACsYCAAAT0AIAAjEfw Date: Tue, 24 Mar 2020 07:52:39 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C4AA5E3@SHSMSX104.ccr.corp.intel.com> References: <20200324063328.8812-1-hao.a.wu@intel.com> <0C09AFA07DD0434D9E2A0C6AEB048310405D4087@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB048310405D411B@shsmsx102.ccr.corp.intel.com> In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB048310405D411B@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable >>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 > Sent: Tuesday, March 24, 2020 3:29 PM > To: Wu, Hao A ; devel@edk2.groups.io > 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 t= o control AP status check interval >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Tuesday, March 24, 2020 3:12 PM > > To: Zeng, Star ; devel@edk2.groups.io > > Cc: Dong, Eric ; Ni, Ray ; Lasz= lo > > Ersek ; Kinney, Michael D > > ; Brian J . Johnson > > 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 P= CD > > > 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. >=20 > Got your good point. > For me, UINT32 (FFFFFFFFh =3D 4294967295d =3D 4294967.295 second =3D 715= 82.78825 minutes ~=3D > 1193.0464708333333333333333333333 hours) should be totally enough. > I do not insist on that if others think UINT64 is ok. >=20 > Thanks, > Star >=20 > > > > 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 ; Dong, Eric > > > > ; Ni, Ray ; Laszlo Ersek > > > > ; Kinney, Michael D ; > > Zeng, Star ; Brian J . > > > > Johnson > > > > 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=3D2627 > > > > > > > > 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 CheckAps= Status(). > > > > > > > > The purpose is to provide the platform owners with the ability to > > > > choose the proper interval value to trigger CheckApsStatus() accor= ding 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 callbac= k > > > > 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/DxeRegisterCpuFeaturesLi= b. > > > > 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 sam= e > > > > with the current interval value, which is 100 milliseconds. > > > > > > > > Unitest done: > > > > OS boot successfully. > > > > > > > > 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(-) > > > > > > > > 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 th= e > > > > + 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 > > > > > > > > -#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS > > (100)) > > > > #define AP_SAFE_STACK_SIZE 128 > > > > > > > > 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); > > > > > > > > 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 librarie= s. > > > > // > > > > -// 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" > > > > > > > > #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() execut= ed > > > > in non-blocking mode in DXE phase.\n" > > > > -- > > > > 2.12.0.windows.1 > > > > > > > > > > > >=20