Re: [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval


Zeng, Star
 

Reviewed-by: Star Zeng <star.zeng@...>

-----Original Message-----
From: Wu, Hao A
Sent: Wednesday, March 25, 2020 1:07 PM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@...>; Dong, Eric <eric.dong@...>; Ni,
Ray <ray.ni@...>; Laszlo Ersek <lersek@...>; Kinney, Michael
D <michael.d.kinney@...>; Zeng, Star <star.zeng@...>; Brian J .
Johnson <brian.johnson@...>
Subject: [PATCH v4] 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.

The type of the PCD is UINT32, which means the maximum possible interval
value can be set to:
4,294,967,295 microseconds = 4,295 seconds = 71.58 minutes = 1.19 hours
which should be sufficient for usage.

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.

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 <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Star Zeng <star.zeng@...>
Cc: Brian J. Johnson <brian.johnson@...>
Signed-off-by: Hao A Wu <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 clock in Hz
as is CPUID Leaf 0x15:ECX

gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|
UINT64|0x32132113

+ ## Specifies the periodic interval value in microseconds for the
+ status check # of APs for StartupAllAPs() and StartupThisAP()
+ executed in non-blocking # 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/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 <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,9 @@ InitMpGlobalData (
Status = gBS->SetTimer (
mCheckAllApsEvent,
TimerPeriodic,
- AP_CHECK_INTERVAL
+ EFI_TIMER_PERIOD_MICROSECONDS (
+ PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSeconds)
+ )
);
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.<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_PcdCpuApStatusCheckIntervalInMicroSec
onds_PROMPT #language en-US "Periodic interval value in microseconds for
AP status check in DXE.\n"
+#string
STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSec
onds_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

Join devel@edk2.groups.io to automatically receive all group messages.