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


Zeng, Star
 

-----Original Message-----
From: Wu, Hao A
Sent: Tuesday, March 24, 2020 3:12 PM
To: Zeng, Star <star.zeng@...>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Laszlo
Ersek <lersek@...>; Kinney, Michael D
<michael.d.kinney@...>; Brian J . Johnson <brian.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 PCD
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.
Got your good point.
For me, UINT32 (FFFFFFFFh = 4294967295d = 4294967.295 second = 71582.78825 minutes ~= 1193.0464708333333333333333333333 hours) should be totally enough.
I do not insist on that if others think UINT64 is ok.

Thanks,
Star


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 <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: [edk2-devel] [PATCH v2] 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.

For least impact, the default value of the new PCD will be the same
with the current interval value, which is 100 milliseconds.

Unitest done:
OS boot successfully.

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@...>
---
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 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 @@

#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,7 @@ InitMpGlobalData (
Status = 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 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_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() 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.