From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mx.groups.io with SMTP id smtpd.web12.6198.1585115033786960980 for ; Tue, 24 Mar 2020 22:43:53 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.65, mailfrom: ray.ni@intel.com) IronPort-SDR: Y7VNq+vEBS6SUW2kT69uRNp/NIwKU6oltfPXU+98ixgeBpzanGp9OZd+ImPhKp1FO7kgb0L+T3 h+Z8k2QrEyrQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Mar 2020 22:43:53 -0700 IronPort-SDR: YZfyqIIN8hENZeUVF8VgctzRjrtwkb/qJgXdzY0luTpRhH/mcAZfXgCH90csiyxRZKM40ZXXOc p5QN9wL76sqA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,303,1580803200"; d="scan'208";a="393524587" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga004.jf.intel.com with ESMTP; 24 Mar 2020 22:43:52 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 24 Mar 2020 22:43:52 -0700 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 24 Mar 2020 22:43:52 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.206]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.145]) with mapi id 14.03.0439.000; Wed, 25 Mar 2020 13:43:49 +0800 From: "Ni, Ray" To: "Wu, Hao A" , "devel@edk2.groups.io" CC: "Dong, Eric" , Laszlo Ersek , "Kinney, Michael D" , "Zeng, Star" , "Brian J . Johnson" Subject: Re: [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval Thread-Topic: [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval Thread-Index: AQHWAmNJWkQNAZAink2q6AbPOyEeRKhYy6kg Date: Wed, 25 Mar 2020 05:43:48 +0000 Deferred-Delivery: Wed, 25 Mar 2020 05:43:00 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C4B27C7@SHSMSX104.ccr.corp.intel.com> References: <20200325050725.9700-1-hao.a.wu@intel.com> In-Reply-To: <20200325050725.9700-1-hao.a.wu@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 Reviewed-by: Ray Ni > -----Original Message----- > From: Wu, Hao A > Sent: Wednesday, March 25, 2020 1:07 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: [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP statu= s 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 > 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). >=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 th= e > number of processors is huge so that the time consumed in CheckApsStatus(= ) > is not negligible. >=20 > The type of the PCD is UINT32, which means the maximum possible interval > value can be set to: > 4,294,967,295 microseconds =3D 4,295 seconds =3D 71.58 minutes =3D 1.19 h= ours > which should be sufficient for usage. >=20 > For least impact, the default value of the new PCD will be the same with > the current interval value. It will be set to 100,000 microseconds, which > is 100 milliseconds. >=20 > 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. >=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 > --- >=20 > Notes: > V4 > Avoiding introducing a local variable in InitMpGlobalData(). >=20 > 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 int= o > the 'SetTimer' service call in V2 patch >=20 > V2 > Introduce a PCD to specify the AP status check interval. >=20 >=20 > 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(-) >=20 > 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 clock = in Hz as is CPUID Leaf 0x15:ECX > gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|UIN= T64|0x32132113 >=20 > + ## Specifies the periodic interval value in microseconds for the statu= s check > + # of APs for StartupAllAPs() and StartupThisAP() executed in non-bloc= king > + # mode in DXE phase. > + # @Prompt Periodic interval value in microseconds for AP status check = in DXE. > + gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds|10= 0000|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/L= ibrary/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 >=20 > [Pcd] > - gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## CO= NSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber ## CO= NSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## SO= METIMES_CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## CO= NSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## CO= NSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ## CO= NSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode ## CO= NSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate ## SO= METIMES_CONSUMES > - gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CO= NSUMES > - > + 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 @@ >=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,9 @@ InitMpGlobalData ( > Status =3D gBS->SetTimer ( > mCheckAllApsEvent, > TimerPeriodic, > - AP_CHECK_INTERVAL > + EFI_TIMER_PERIOD_MICROSECONDS ( > + PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSeconds) > + ) > ); > ASSERT_EFI_ERROR (Status); >=20 > 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_PROM= PT #language en-US "Specify the > count of pre allocated SMM MP tokens per chunk.\n" >=20 > #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HELP= #language en-US "This value used > to specify the count of pre allocated SMM MP tokens per chunk.\n" > + > +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicro= Seconds_PROMPT #language en-US > "Periodic interval value in microseconds for AP status check in DXE.\n" > +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicro= Seconds_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" > -- > 2.12.0.windows.1