Refactor the logic for placing APs in
Mwait/Runloop into a separate function.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
1 file changed, 50 insertions(+), 33 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 4e517a9b19..8aecd29a94 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -659,6 +659,54 @@ PlaceAPInHltLoop (
}
}
+/**
+ This function place APs in Mwait or Run loop.
+
+ @param[in] ApLoopMode Ap Loop Mode
+ @param[in] ApStartupSignalBuffer Pointer to Ap Startup Signal Buffer
+ @param[in] ApTargetCState Ap Target CState
+**/
+VOID
+PlaceAPInMwaitLoopOrRunLoop (
+ IN UINT8 ApLoopMode,
+ IN volatile UINT32 *ApStartupSignalBuffer,
+ IN UINT8 ApTargetCState
+ )
+{
+ while (TRUE) {
+ DisableInterrupts ();
+ if (ApLoopMode == ApInMwaitLoop) {
+ //
+ // Place AP in MWAIT-loop
+ //
+ AsmMonitor ((UINTN)ApStartupSignalBuffer, 0, 0);
+ if ((*ApStartupSignalBuffer != WAKEUP_AP_SIGNAL) && (*ApStartupSignalBuffer != MP_HAND_OFF_SIGNAL)) {
+ //
+ // Check AP start-up signal again.
+ // If AP start-up signal is not set, place AP into
+ // the specified C-state
+ //
+ AsmMwait (ApTargetCState << 4, 0);
+ }
+ } else if (ApLoopMode == ApInRunLoop) {
+ //
+ // Place AP in Run-loop
+ //
+ CpuPause ();
+ } else {
+ ASSERT (FALSE);
+ }
+
+ //
+ // If AP start-up signal is written, AP is waken up
+ // otherwise place AP in loop again
+ //
+ if ((*ApStartupSignalBuffer == WAKEUP_AP_SIGNAL) || (*ApStartupSignalBuffer == MP_HAND_OFF_SIGNAL)) {
+ break;
+ }
+ }
+}
+
/**
This function will be called from AP reset code if BSP uses WakeUpAP.
@@ -839,39 +887,8 @@ ApWakeupFunction (
//
// Never run here
//
- }
-
- while (TRUE) {
- DisableInterrupts ();
- if (CpuMpData->ApLoopMode == ApInMwaitLoop) {
- //
- // Place AP in MWAIT-loop
- //
- AsmMonitor ((UINTN)ApStartupSignalBuffer, 0, 0);
- if (*ApStartupSignalBuffer != WAKEUP_AP_SIGNAL) {
- //
- // Check AP start-up signal again.
- // If AP start-up signal is not set, place AP into
- // the specified C-state
- //
- AsmMwait (CpuMpData->ApTargetCState << 4, 0);
- }
- } else if (CpuMpData->ApLoopMode == ApInRunLoop) {
- //
- // Place AP in Run-loop
- //
- CpuPause ();
- } else {
- ASSERT (FALSE);
- }
-
- //
- // If AP start-up signal is written, AP is waken up
- // otherwise place AP in loop again
- //
- if (*ApStartupSignalBuffer == WAKEUP_AP_SIGNAL) {
- break;
- }
+ } else {
+ PlaceAPInMwaitLoopOrRunLoop (CpuMpData->ApLoopMode, ApStartupSignalBuffer, CpuMpData->ApTargetCState);
}
}
}
--
2.36.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106017): https://edk2.groups.io/g/devel/message/106017
Mute This Topic: https://groups.io/mt/99483117/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-On Mon, Jun 12, 2023 at 09:37:18PM +0800, Yuanhao Xie wrote:
> Refactor the logic for placing APs in
> Mwait/Runloop into a separate function.
> + // If AP start-up signal is written, AP is waken up
> + // otherwise place AP in loop again
> + //
> + if ((*ApStartupSignalBuffer == WAKEUP_AP_SIGNAL) || (*ApStartupSignalBuffer == MP_HAND_OFF_SIGNAL)) {
> + break;
> + }
> - // If AP start-up signal is written, AP is waken up
> - // otherwise place AP in loop again
> - //
> - if (*ApStartupSignalBuffer == WAKEUP_AP_SIGNAL) {
> - break;
> - }
Nope. This isn't refactoring. Please split the patch into two, one
doing the refactoring without functional change, and one adding the code
for MP_HAND_OFF_SIGNAL.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106216): https://edk2.groups.io/g/devel/message/106216
Mute This Topic: https://groups.io/mt/99483117/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Gerd,
Thanks for the feedback.
I have updated in V2.
Please have a check.
Yuanhao
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd Hoffmann
Sent: Tuesday, June 20, 2023 10:11 PM
To: devel@edk2.groups.io; Xie, Yuanhao <yuanhao.xie@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH 3/5] UefiCpuPkg: Refactor the logic for placing APs in Mwait/Runloop.
On Mon, Jun 12, 2023 at 09:37:18PM +0800, Yuanhao Xie wrote:
> Refactor the logic for placing APs in
> Mwait/Runloop into a separate function.
> + // If AP start-up signal is written, AP is waken up
> + // otherwise place AP in loop again
> + //
> + if ((*ApStartupSignalBuffer == WAKEUP_AP_SIGNAL) || (*ApStartupSignalBuffer == MP_HAND_OFF_SIGNAL)) {
> + break;
> + }
> - // If AP start-up signal is written, AP is waken up
> - // otherwise place AP in loop again
> - //
> - if (*ApStartupSignalBuffer == WAKEUP_AP_SIGNAL) {
> - break;
> - }
Nope. This isn't refactoring. Please split the patch into two, one doing the refactoring without functional change, and one adding the code for MP_HAND_OFF_SIGNAL.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106330): https://edk2.groups.io/g/devel/message/106330
Mute This Topic: https://groups.io/mt/99483117/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.