* [Patch V3 0/3] StartAllAPs should not use disabled APs
@ 2018-07-25 7:50 Eric Dong
2018-07-25 7:50 ` [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State Eric Dong
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Eric Dong @ 2018-07-25 7:50 UTC (permalink / raw)
To: edk2-devel
This patch series include changes:
1. StartAllAPs should not use disabled APs, this is required by UEFI spec.
2. Refine the code to remove the redundant definitions.
V2 changes:
Use CpuStateReady to distinguish the AP state from CpuStateIdle.
V3 Changes:
Only change 3/3 patch. Only not use disabled APs when WakeUpAP called
by StartAllAps function. In other cases, also include disabled APs.
Eric Dong (3):
UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.
UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
UefiCpuPkg/Library/MpInitLib/MpLib.c | 33 +++++++++++++++------------------
UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +---
2 files changed, 16 insertions(+), 21 deletions(-)
--
2.15.0.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
2018-07-25 7:50 [Patch V3 0/3] StartAllAPs should not use disabled APs Eric Dong
@ 2018-07-25 7:50 ` Eric Dong
2018-07-25 10:54 ` Laszlo Ersek
2018-07-25 7:50 ` [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition Eric Dong
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Eric Dong @ 2018-07-25 7:50 UTC (permalink / raw)
To: edk2-devel; +Cc: Laszlo Ersek, Ruiyu Ni
Current CPU state definition include CpuStateIdle and CpuStateFinished.
After investigation, current code can use CpuStateIdle to replace the
CpuStateFinished. It will reduce the state number and easy for maintenance.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 18 ++++++++----------
UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 -
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index c82b985943..ff09a0e9e7 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -696,7 +696,7 @@ ApWakeupFunction (
}
}
}
- SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateFinished);
+ SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateIdle);
}
}
@@ -1352,18 +1352,17 @@ CheckThisAP (
CpuData = &CpuMpData->CpuData[ProcessorNumber];
//
- // Check the CPU state of AP. If it is CpuStateFinished, then the AP has finished its task.
+ // Check the CPU state of AP. If it is CpuStateIdle, then the AP has finished its task.
// Only BSP and corresponding AP access this unit of CPU Data. This means the AP will not modify the
- // value of state after setting the it to CpuStateFinished, so BSP can safely make use of its value.
+ // value of state after setting the it to CpuStateIdle, so BSP can safely make use of its value.
//
//
// If the AP finishes for StartupThisAP(), return EFI_SUCCESS.
//
- if (GetApState(CpuData) == CpuStateFinished) {
+ if (GetApState(CpuData) == CpuStateIdle) {
if (CpuData->Finished != NULL) {
*(CpuData->Finished) = TRUE;
}
- SetApState (CpuData, CpuStateIdle);
return EFI_SUCCESS;
} else {
//
@@ -1420,14 +1419,13 @@ CheckAllAPs (
CpuData = &CpuMpData->CpuData[ProcessorNumber];
//
- // Check the CPU state of AP. If it is CpuStateFinished, then the AP has finished its task.
+ // Check the CPU state of AP. If it is CpuStateIdle, then the AP has finished its task.
// Only BSP and corresponding AP access this unit of CPU Data. This means the AP will not modify the
- // value of state after setting the it to CpuStateFinished, so BSP can safely make use of its value.
+ // value of state after setting the it to CpuStateIdle, so BSP can safely make use of its value.
//
- if (GetApState(CpuData) == CpuStateFinished) {
+ if (GetApState(CpuData) == CpuStateIdle) {
CpuMpData->RunningCount ++;
CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
- SetApState(CpuData, CpuStateIdle);
//
// If in Single Thread mode, then search for the next waiting AP for execution.
@@ -1923,7 +1921,7 @@ SwitchBSPWorker (
//
// Wait for old BSP finished AP task
//
- while (GetApState (&CpuMpData->CpuData[CallerNumber]) != CpuStateFinished) {
+ while (GetApState (&CpuMpData->CpuData[CallerNumber]) != CpuStateIdle) {
CpuPause ();
}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 9d0b866d09..962bce685d 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -85,7 +85,6 @@ typedef enum {
CpuStateIdle,
CpuStateReady,
CpuStateBusy,
- CpuStateFinished,
CpuStateDisabled
} CPU_STATE;
--
2.15.0.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.
2018-07-25 7:50 [Patch V3 0/3] StartAllAPs should not use disabled APs Eric Dong
2018-07-25 7:50 ` [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State Eric Dong
@ 2018-07-25 7:50 ` Eric Dong
2018-07-25 11:47 ` Laszlo Ersek
2018-07-26 5:22 ` Ni, Ruiyu
2018-07-25 7:50 ` [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs Eric Dong
2018-07-25 12:12 ` [Patch V3 0/3] StartAllAPs should not use disabled APs Laszlo Ersek
3 siblings, 2 replies; 17+ messages in thread
From: Eric Dong @ 2018-07-25 7:50 UTC (permalink / raw)
To: edk2-devel; +Cc: Laszlo Ersek, Ruiyu Ni
The StartCount is duplicated with RunningCount, replace it with
RunningCount. Also the volatile for RunningCount is not needed.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +++++------
UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +--
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index ff09a0e9e7..0e57cc86bf 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1424,7 +1424,7 @@ CheckAllAPs (
// value of state after setting the it to CpuStateIdle, so BSP can safely make use of its value.
//
if (GetApState(CpuData) == CpuStateIdle) {
- CpuMpData->RunningCount ++;
+ CpuMpData->RunningCount --;
CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
//
@@ -1449,7 +1449,7 @@ CheckAllAPs (
//
// If all APs finish, return EFI_SUCCESS.
//
- if (CpuMpData->RunningCount == CpuMpData->StartCount) {
+ if (CpuMpData->RunningCount == 0) {
return EFI_SUCCESS;
}
@@ -1466,7 +1466,7 @@ CheckAllAPs (
//
if (CpuMpData->FailedCpuList != NULL) {
*CpuMpData->FailedCpuList =
- AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount + 1) * sizeof (UINTN));
+ AllocatePool ((CpuMpData->RunningCount + 1) * sizeof (UINTN));
ASSERT (*CpuMpData->FailedCpuList != NULL);
}
ListIndex = 0;
@@ -2212,7 +2212,7 @@ StartupAllAPsWorker (
return EFI_NOT_STARTED;
}
- CpuMpData->StartCount = 0;
+ CpuMpData->RunningCount = 0;
for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; ProcessorNumber++) {
CpuData = &CpuMpData->CpuData[ProcessorNumber];
CpuData->Waiting = FALSE;
@@ -2222,7 +2222,7 @@ StartupAllAPsWorker (
// Mark this processor as responsible for current calling.
//
CpuData->Waiting = TRUE;
- CpuMpData->StartCount++;
+ CpuMpData->RunningCount++;
}
}
}
@@ -2231,7 +2231,6 @@ StartupAllAPsWorker (
CpuMpData->ProcArguments = ProcedureArgument;
CpuMpData->SingleThread = SingleThread;
CpuMpData->FinishedCount = 0;
- CpuMpData->RunningCount = 0;
CpuMpData->FailedCpuList = FailedCpuList;
CpuMpData->ExpectedTime = CalculateTimeout (
TimeoutInMicroseconds,
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 962bce685d..5002b7e9c0 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -211,9 +211,8 @@ struct _CPU_MP_DATA {
UINTN BackupBuffer;
UINTN BackupBufferSize;
- volatile UINT32 StartCount;
volatile UINT32 FinishedCount;
- volatile UINT32 RunningCount;
+ UINT32 RunningCount;
BOOLEAN SingleThread;
EFI_AP_PROCEDURE Procedure;
VOID *ProcArguments;
--
2.15.0.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
2018-07-25 7:50 [Patch V3 0/3] StartAllAPs should not use disabled APs Eric Dong
2018-07-25 7:50 ` [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State Eric Dong
2018-07-25 7:50 ` [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition Eric Dong
@ 2018-07-25 7:50 ` Eric Dong
2018-07-25 12:11 ` Laszlo Ersek
2018-07-26 8:36 ` Ni, Ruiyu
2018-07-25 12:12 ` [Patch V3 0/3] StartAllAPs should not use disabled APs Laszlo Ersek
3 siblings, 2 replies; 17+ messages in thread
From: Eric Dong @ 2018-07-25 7:50 UTC (permalink / raw)
To: edk2-devel; +Cc: Laszlo Ersek, Ruiyu Ni
Base on UEFI spec requirement, StartAllAPs function should not
use the APs which has been disabled before. This patch just
change current code to follow this rule.
V3 changes:
Only called by StartUpAllAps, WakeUpAp will not wake up
the disabled APs, in other cases also need to include the
disabled APs, such as CpuDxe driver start up and
ChangeApLoopCallback function.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +-
UefiCpuPkg/Library/MpInitLib/MpLib.c | 27 +++++++++++++++++----------
UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +++-
3 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index d82e9aea45..c17daa0fc0 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -306,7 +306,7 @@ MpInitChangeApLoopCallback (
CpuMpData->PmCodeSegment = GetProtectedModeCS ();
CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
mNumberToFinish = CpuMpData->CpuCount - 1;
- WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
+ WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE);
while (mNumberToFinish > 0) {
CpuPause ();
}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 0e57cc86bf..1a81062a3f 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -470,7 +470,7 @@ CollectProcessorCount (
//
CpuMpData->InitFlag = ApInitConfig;
CpuMpData->X2ApicEnable = FALSE;
- WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL);
+ WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
CpuMpData->InitFlag = ApInitDone;
ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
//
@@ -491,7 +491,7 @@ CollectProcessorCount (
//
// Wakeup all APs to enable x2APIC mode
//
- WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL);
+ WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL, TRUE);
//
// Wait for all known APs finished
//
@@ -969,6 +969,7 @@ FreeResetVector (
@param[in] ProcessorNumber The handle number of specified processor
@param[in] Procedure The function to be invoked by AP
@param[in] ProcedureArgument The argument to be passed into AP function
+ @param[in] WakeUpDisabledAps Whether need to wake up disabled APs in broadcast mode.
**/
VOID
WakeUpAP (
@@ -976,7 +977,8 @@ WakeUpAP (
IN BOOLEAN Broadcast,
IN UINTN ProcessorNumber,
IN EFI_AP_PROCEDURE Procedure, OPTIONAL
- IN VOID *ProcedureArgument OPTIONAL
+ IN VOID *ProcedureArgument, OPTIONAL
+ IN BOOLEAN WakeUpDisabledAps
)
{
volatile MP_CPU_EXCHANGE_INFO *ExchangeInfo;
@@ -1010,6 +1012,10 @@ WakeUpAP (
for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
if (Index != CpuMpData->BspNumber) {
CpuData = &CpuMpData->CpuData[Index];
+ if (GetApState (CpuData) == CpuStateDisabled && !WakeUpDisabledAps) {
+ continue;
+ }
+
CpuData->ApFunction = (UINTN) Procedure;
CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
SetApState (CpuData, CpuStateReady);
@@ -1289,7 +1295,7 @@ ResetProcessorToIdleState (
CpuMpData = GetCpuMpData ();
CpuMpData->InitFlag = ApInitReconfig;
- WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL);
+ WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL, TRUE);
while (CpuMpData->FinishedCount < 1) {
CpuPause ();
}
@@ -1439,7 +1445,8 @@ CheckAllAPs (
FALSE,
(UINT32) NextProcessorNumber,
CpuMpData->Procedure,
- CpuMpData->ProcArguments
+ CpuMpData->ProcArguments,
+ TRUE
);
}
}
@@ -1711,7 +1718,7 @@ MpInitLibInitialize (
//
// Wakeup APs to do some AP initialize sync
//
- WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData);
+ WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
//
// Wait for all APs finished initialization
//
@@ -1906,7 +1913,7 @@ SwitchBSPWorker (
//
// Need to wakeUp AP (future BSP).
//
- WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, CpuMpData);
+ WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, CpuMpData, TRUE);
AsmExchangeRole (&CpuMpData->BSPInfo, &CpuMpData->APInfo);
@@ -2240,14 +2247,14 @@ StartupAllAPsWorker (
CpuMpData->WaitEvent = WaitEvent;
if (!SingleThread) {
- WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument);
+ WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument, FALSE);
} else {
for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; ProcessorNumber++) {
if (ProcessorNumber == CallerNumber) {
continue;
}
if (CpuMpData->CpuData[ProcessorNumber].Waiting) {
- WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument);
+ WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument, TRUE);
break;
}
}
@@ -2359,7 +2366,7 @@ StartupThisAPWorker (
CpuData->ExpectedTime = CalculateTimeout (TimeoutInMicroseconds, &CpuData->CurrentTime);
CpuData->TotalTime = 0;
- WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument);
+ WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument, TRUE);
//
// If WaitEvent is NULL, execute in blocking mode.
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 5002b7e9c0..fe7a917e49 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -373,6 +373,7 @@ GetModeTransitionBuffer (
@param[in] ProcessorNumber The handle number of specified processor
@param[in] Procedure The function to be invoked by AP
@param[in] ProcedureArgument The argument to be passed into AP function
+ @param[in] WakeUpDisabledAps Whether need to wake up disabled APs in broadcast mode.
**/
VOID
WakeUpAP (
@@ -380,7 +381,8 @@ WakeUpAP (
IN BOOLEAN Broadcast,
IN UINTN ProcessorNumber,
IN EFI_AP_PROCEDURE Procedure, OPTIONAL
- IN VOID *ProcedureArgument OPTIONAL
+ IN VOID *ProcedureArgument, OPTIONAL
+ IN BOOLEAN WakeUpDisabledAps OPTIONAL
);
/**
--
2.15.0.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
2018-07-25 7:50 ` [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State Eric Dong
@ 2018-07-25 10:54 ` Laszlo Ersek
2018-07-25 11:15 ` Dong, Eric
0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2018-07-25 10:54 UTC (permalink / raw)
To: Eric Dong, edk2-devel; +Cc: Ruiyu Ni
Hi Eric,
On 07/25/18 09:50, Eric Dong wrote:
> Current CPU state definition include CpuStateIdle and CpuStateFinished.
> After investigation, current code can use CpuStateIdle to replace the
> CpuStateFinished. It will reduce the state number and easy for maintenance.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 18 ++++++++----------
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 -
> 2 files changed, 8 insertions(+), 11 deletions(-)
After looking over this patch, it seems that you are preserving the
CpuStateReady enum constant, relative to:
http://mid.mail-archive.com/20180628112920.5296-1-eric.dong@intel.com
However, based on your analysis in
http://mid.mail-archive.com/ED077930C258884BBCB450DB737E66224AC5A453@shsmsx102.ccr.corp.intel.com
isn't it still possible to run into the exact same issue? (Namely, BSP
thinks the AP has gone through Idle -> Busy -> Idle, but the AP has
never actually left Idle?)
Hm, wait, is it the case that the BSP first sets Ready, and so if the
check for an AP returns Idle, it implies the AP must have gone through:
Idle ----> Ready ----> Busy ----> Idle
?
If this is correct, can you please include the following in the commit
message:
> Before this patch, the state transitions for an AP are:
>
> Idle ----> Ready ----> Busy ----> Finished ----> Idle
> [BSP] [AP] [AP] [BSP]
>
> After the patch, the state transitions for an AP are:
>
> Idle ----> Ready ----> Busy ----> Idle
> [BSP] [AP] [AP]
Do you agree?
I have another question:
On 07/25/18 09:50, Eric Dong wrote:
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index c82b985943..ff09a0e9e7 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -696,7 +696,7 @@ ApWakeupFunction (
> }
> }
> }
> - SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateFinished);
> + SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateIdle);
> }
> }
>
> @@ -1352,18 +1352,17 @@ CheckThisAP (
> CpuData = &CpuMpData->CpuData[ProcessorNumber];
>
> //
> - // Check the CPU state of AP. If it is CpuStateFinished, then the AP has finished its task.
> + // Check the CPU state of AP. If it is CpuStateIdle, then the AP has finished its task.
> // Only BSP and corresponding AP access this unit of CPU Data. This means the AP will not modify the
> - // value of state after setting the it to CpuStateFinished, so BSP can safely make use of its value.
> + // value of state after setting the it to CpuStateIdle, so BSP can safely make use of its value.
> //
> //
> // If the AP finishes for StartupThisAP(), return EFI_SUCCESS.
> //
> - if (GetApState(CpuData) == CpuStateFinished) {
> + if (GetApState(CpuData) == CpuStateIdle) {
> if (CpuData->Finished != NULL) {
> *(CpuData->Finished) = TRUE;
> }
> - SetApState (CpuData, CpuStateIdle);
> return EFI_SUCCESS;
> } else {
> //
> @@ -1420,14 +1419,13 @@ CheckAllAPs (
>
> CpuData = &CpuMpData->CpuData[ProcessorNumber];
> //
> - // Check the CPU state of AP. If it is CpuStateFinished, then the AP has finished its task.
> + // Check the CPU state of AP. If it is CpuStateIdle, then the AP has finished its task.
> // Only BSP and corresponding AP access this unit of CPU Data. This means the AP will not modify the
> - // value of state after setting the it to CpuStateFinished, so BSP can safely make use of its value.
> + // value of state after setting the it to CpuStateIdle, so BSP can safely make use of its value.
> //
> - if (GetApState(CpuData) == CpuStateFinished) {
> + if (GetApState(CpuData) == CpuStateIdle) {
> CpuMpData->RunningCount ++;
> CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> - SetApState(CpuData, CpuStateIdle);
>
> //
> // If in Single Thread mode, then search for the next waiting AP for execution.
This part of the code, after the patch, does not seem idempotent; in
other words, if the BSP calls CheckAllAPs() multiple times, then
RunningCount will be increased every time. Before the patch, this wasn't
the case, because after the Finished -> Idle transition, the increment
wouldn't be reached again.
Hmmm, wait, I'm wrong: we set the Waiting field to FALSE as well, so at
the next call to CheckAllAPs(), we'll take the early "continue" branch.
Looks OK after all.
I'll follow up with test results.
Thanks,
Laszlo
> @@ -1923,7 +1921,7 @@ SwitchBSPWorker (
> //
> // Wait for old BSP finished AP task
> //
> - while (GetApState (&CpuMpData->CpuData[CallerNumber]) != CpuStateFinished) {
> + while (GetApState (&CpuMpData->CpuData[CallerNumber]) != CpuStateIdle) {
> CpuPause ();
> }
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 9d0b866d09..962bce685d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -85,7 +85,6 @@ typedef enum {
> CpuStateIdle,
> CpuStateReady,
> CpuStateBusy,
> - CpuStateFinished,
> CpuStateDisabled
> } CPU_STATE;
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
2018-07-25 10:54 ` Laszlo Ersek
@ 2018-07-25 11:15 ` Dong, Eric
2018-07-26 5:18 ` Ni, Ruiyu
0 siblings, 1 reply; 17+ messages in thread
From: Dong, Eric @ 2018-07-25 11:15 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, July 25, 2018 6:55 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant
> CpuStateFinished State.
>
> Hi Eric,
>
> On 07/25/18 09:50, Eric Dong wrote:
> > Current CPU state definition include CpuStateIdle and CpuStateFinished.
> > After investigation, current code can use CpuStateIdle to replace the
> > CpuStateFinished. It will reduce the state number and easy for maintenance.
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> > UefiCpuPkg/Library/MpInitLib/MpLib.c | 18 ++++++++----------
> > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 -
> > 2 files changed, 8 insertions(+), 11 deletions(-)
>
> After looking over this patch, it seems that you are preserving the
> CpuStateReady enum constant, relative to:
>
> http://mid.mail-archive.com/20180628112920.5296-1-eric.dong@intel.com
>
> However, based on your analysis in
>
> http://mid.mail-
> archive.com/ED077930C258884BBCB450DB737E66224AC5A453@shsmsx102.
> ccr.corp.intel.com
>
> isn't it still possible to run into the exact same issue? (Namely, BSP thinks the
> AP has gone through Idle -> Busy -> Idle, but the AP has never actually left
> Idle?)
>
> Hm, wait, is it the case that the BSP first sets Ready, and so if the check for an
> AP returns Idle, it implies the AP must have gone through:
>
> Idle ----> Ready ----> Busy ----> Idle
>
> ?
Correct! The Ready state is begin state and the Idle is the finish state.
>
> If this is correct, can you please include the following in the commit
> message:
>
> > Before this patch, the state transitions for an AP are:
> >
> > Idle ----> Ready ----> Busy ----> Finished ----> Idle
> > [BSP] [AP] [AP] [BSP]
> >
> > After the patch, the state transitions for an AP are:
> >
> > Idle ----> Ready ----> Busy ----> Idle
> > [BSP] [AP] [AP]
>
> Do you agree?
Good suggestion, I will include this info in the commit message.
>
> I have another question:
>
> On 07/25/18 09:50, Eric Dong wrote:
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index c82b985943..ff09a0e9e7 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -696,7 +696,7 @@ ApWakeupFunction (
> > }
> > }
> > }
> > - SetApState (&CpuMpData->CpuData[ProcessorNumber],
> CpuStateFinished);
> > + SetApState (&CpuMpData->CpuData[ProcessorNumber],
> > + CpuStateIdle);
> > }
> > }
> >
> > @@ -1352,18 +1352,17 @@ CheckThisAP (
> > CpuData = &CpuMpData->CpuData[ProcessorNumber];
> >
> > //
> > - // Check the CPU state of AP. If it is CpuStateFinished, then the AP has
> finished its task.
> > + // Check the CPU state of AP. If it is CpuStateIdle, then the AP has
> finished its task.
> > // Only BSP and corresponding AP access this unit of CPU Data.
> > This means the AP will not modify the
> > - // value of state after setting the it to CpuStateFinished, so BSP can safely
> make use of its value.
> > + // value of state after setting the it to CpuStateIdle, so BSP can safely
> make use of its value.
> > //
> > //
> > // If the AP finishes for StartupThisAP(), return EFI_SUCCESS.
> > //
> > - if (GetApState(CpuData) == CpuStateFinished) {
> > + if (GetApState(CpuData) == CpuStateIdle) {
> > if (CpuData->Finished != NULL) {
> > *(CpuData->Finished) = TRUE;
> > }
> > - SetApState (CpuData, CpuStateIdle);
> > return EFI_SUCCESS;
> > } else {
> > //
> > @@ -1420,14 +1419,13 @@ CheckAllAPs (
> >
> > CpuData = &CpuMpData->CpuData[ProcessorNumber];
> > //
> > - // Check the CPU state of AP. If it is CpuStateFinished, then the AP has
> finished its task.
> > + // Check the CPU state of AP. If it is CpuStateIdle, then the AP has
> finished its task.
> > // Only BSP and corresponding AP access this unit of CPU Data. This
> means the AP will not modify the
> > - // value of state after setting the it to CpuStateFinished, so BSP can
> safely make use of its value.
> > + // value of state after setting the it to CpuStateIdle, so BSP can safely
> make use of its value.
> > //
> > - if (GetApState(CpuData) == CpuStateFinished) {
> > + if (GetApState(CpuData) == CpuStateIdle) {
> > CpuMpData->RunningCount ++;
> > CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> > - SetApState(CpuData, CpuStateIdle);
> >
> > //
> > // If in Single Thread mode, then search for the next waiting AP for
> execution.
>
> This part of the code, after the patch, does not seem idempotent; in other
> words, if the BSP calls CheckAllAPs() multiple times, then RunningCount will
> be increased every time. Before the patch, this wasn't the case, because after
> the Finished -> Idle transition, the increment wouldn't be reached again.
>
> Hmmm, wait, I'm wrong: we set the Waiting field to FALSE as well, so at the
> next call to CheckAllAPs(), we'll take the early "continue" branch.
> Looks OK after all.
>
Yes, we have two flags here. Waiting flags means the AP will do the task. State flag means whether the task has finished.
Both flags will be checked and updated.
> I'll follow up with test results.
>
> Thanks,
> Laszlo
>
> > @@ -1923,7 +1921,7 @@ SwitchBSPWorker (
> > //
> > // Wait for old BSP finished AP task
> > //
> > - while (GetApState (&CpuMpData->CpuData[CallerNumber]) !=
> > CpuStateFinished) {
> > + while (GetApState (&CpuMpData->CpuData[CallerNumber]) !=
> > + CpuStateIdle) {
> > CpuPause ();
> > }
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 9d0b866d09..962bce685d 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -85,7 +85,6 @@ typedef enum {
> > CpuStateIdle,
> > CpuStateReady,
> > CpuStateBusy,
> > - CpuStateFinished,
> > CpuStateDisabled
> > } CPU_STATE;
> >
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.
2018-07-25 7:50 ` [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition Eric Dong
@ 2018-07-25 11:47 ` Laszlo Ersek
2018-07-25 12:09 ` Dong, Eric
2018-07-26 5:22 ` Ni, Ruiyu
1 sibling, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2018-07-25 11:47 UTC (permalink / raw)
To: Eric Dong, edk2-devel; +Cc: Ruiyu Ni
Hi Eric,
On 07/25/18 09:50, Eric Dong wrote:
> The StartCount is duplicated with RunningCount, replace it with
> RunningCount. Also the volatile for RunningCount is not needed.
after staring at this patch for a long time, I think it is correct.
However, I suggest that we improve the commit message, because this
patch actually does three things:
(1) It removes "volatile" from RunningCount.
[This is OK, because only the BSP modifies it.]
(2) [This is the tricky part.]
When we detect a timeout in CheckAllAPs(), and collect the list of
failed CPUs, the size of the list is derived from the following
difference, before the patch:
StartCount - FinishedCount
where "StartCount" is set by the BSP at startup, and FinishedCount is
incremented by the APs themselves.
The patch replaces this difference with
StartCount - RunningCount
that is, the difference is no more calculated from the BSP's startup
counter and the AP's shared finish counter, but from the RunningCount
measurement that the BSP does itself, in CheckAllAPs().
[This change is OK to me as well, but we should be clear about it.]
(3) Finally, the patch changes the meaning of RunningCount. Before the
patch, we have:
- StartCount: the number of APs the BSP stars up,
- RunningCount: the number of finished APs that the BSP collected
After the patch, StartCount is removed, and RunningCount is *redefined*
as the following difference:
OLD_StartCount - OLD_RunningCount
Giving the number of APs that the BSP started up but hasn't collected yet.
[This change looks good to me as well.]
----*----
Importantly, what we see in the AllocatePool() argument, is the
*composition* of steps (2) and (3).
If you agree, can you please update the commit message to include my
points (1) through (3)? It's OK if you leave out my remarks in brackets [].
No need to repost just because of this, of course.
Thanks!
Laszlo
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +++++------
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +--
> 2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index ff09a0e9e7..0e57cc86bf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1424,7 +1424,7 @@ CheckAllAPs (
> // value of state after setting the it to CpuStateIdle, so BSP can safely make use of its value.
> //
> if (GetApState(CpuData) == CpuStateIdle) {
> - CpuMpData->RunningCount ++;
> + CpuMpData->RunningCount --;
> CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
>
> //
> @@ -1449,7 +1449,7 @@ CheckAllAPs (
> //
> // If all APs finish, return EFI_SUCCESS.
> //
> - if (CpuMpData->RunningCount == CpuMpData->StartCount) {
> + if (CpuMpData->RunningCount == 0) {
> return EFI_SUCCESS;
> }
>
> @@ -1466,7 +1466,7 @@ CheckAllAPs (
> //
> if (CpuMpData->FailedCpuList != NULL) {
> *CpuMpData->FailedCpuList =
> - AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount + 1) * sizeof (UINTN));
> + AllocatePool ((CpuMpData->RunningCount + 1) * sizeof (UINTN));
> ASSERT (*CpuMpData->FailedCpuList != NULL);
> }
> ListIndex = 0;
> @@ -2212,7 +2212,7 @@ StartupAllAPsWorker (
> return EFI_NOT_STARTED;
> }
>
> - CpuMpData->StartCount = 0;
> + CpuMpData->RunningCount = 0;
> for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; ProcessorNumber++) {
> CpuData = &CpuMpData->CpuData[ProcessorNumber];
> CpuData->Waiting = FALSE;
> @@ -2222,7 +2222,7 @@ StartupAllAPsWorker (
> // Mark this processor as responsible for current calling.
> //
> CpuData->Waiting = TRUE;
> - CpuMpData->StartCount++;
> + CpuMpData->RunningCount++;
> }
> }
> }
> @@ -2231,7 +2231,6 @@ StartupAllAPsWorker (
> CpuMpData->ProcArguments = ProcedureArgument;
> CpuMpData->SingleThread = SingleThread;
> CpuMpData->FinishedCount = 0;
> - CpuMpData->RunningCount = 0;
> CpuMpData->FailedCpuList = FailedCpuList;
> CpuMpData->ExpectedTime = CalculateTimeout (
> TimeoutInMicroseconds,
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 962bce685d..5002b7e9c0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -211,9 +211,8 @@ struct _CPU_MP_DATA {
> UINTN BackupBuffer;
> UINTN BackupBufferSize;
>
> - volatile UINT32 StartCount;
> volatile UINT32 FinishedCount;
> - volatile UINT32 RunningCount;
> + UINT32 RunningCount;
> BOOLEAN SingleThread;
> EFI_AP_PROCEDURE Procedure;
> VOID *ProcArguments;
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.
2018-07-25 11:47 ` Laszlo Ersek
@ 2018-07-25 12:09 ` Dong, Eric
2018-07-25 15:18 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: Dong, Eric @ 2018-07-25 12:09 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, July 25, 2018 7:47 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount
> and volatile definition.
>
> Hi Eric,
>
> On 07/25/18 09:50, Eric Dong wrote:
> > The StartCount is duplicated with RunningCount, replace it with
> > RunningCount. Also the volatile for RunningCount is not needed.
>
> after staring at this patch for a long time, I think it is correct.
> However, I suggest that we improve the commit message, because this patch
> actually does three things:
>
> (1) It removes "volatile" from RunningCount.
>
> [This is OK, because only the BSP modifies it.]
>
> (2) [This is the tricky part.]
>
> When we detect a timeout in CheckAllAPs(), and collect the list of failed CPUs,
> the size of the list is derived from the following difference, before the patch:
>
> StartCount - FinishedCount
>
> where "StartCount" is set by the BSP at startup, and FinishedCount is
> incremented by the APs themselves.
>
I think FinishedCount used here is a typo. What the logic from the code context here is want to get the failed Aps and return them. So it should use RunningCount here, right? Also the FinishedCount may be bigger than StartCount if many Aps has been disabled. Right?
> The patch replaces this difference with
>
> StartCount - RunningCount
>
> that is, the difference is no more calculated from the BSP's startup counter
> and the AP's shared finish counter, but from the RunningCount measurement
> that the BSP does itself, in CheckAllAPs().
>
> [This change is OK to me as well, but we should be clear about it.]
>
> (3) Finally, the patch changes the meaning of RunningCount. Before the patch,
> we have:
>
> - StartCount: the number of APs the BSP stars up,
> - RunningCount: the number of finished APs that the BSP collected
>
> After the patch, StartCount is removed, and RunningCount is *redefined* as
> the following difference:
>
> OLD_StartCount - OLD_RunningCount
>
> Giving the number of APs that the BSP started up but hasn't collected yet.
>
> [This change looks good to me as well.]
>
> ----*----
>
> Importantly, what we see in the AllocatePool() argument, is the
> *composition* of steps (2) and (3).
>
> If you agree, can you please update the commit message to include my points
> (1) through (3)? It's OK if you leave out my remarks in brackets [].
Agreed. Will use your content when commit the changes.
Thanks,
Eric
>
> No need to repost just because of this, of course.
>
> Thanks!
> Laszlo
>
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> > UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +++++------
> > UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +--
> > 2 files changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index ff09a0e9e7..0e57cc86bf 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -1424,7 +1424,7 @@ CheckAllAPs (
> > // value of state after setting the it to CpuStateIdle, so BSP can safely
> make use of its value.
> > //
> > if (GetApState(CpuData) == CpuStateIdle) {
> > - CpuMpData->RunningCount ++;
> > + CpuMpData->RunningCount --;
> > CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> >
> > //
> > @@ -1449,7 +1449,7 @@ CheckAllAPs (
> > //
> > // If all APs finish, return EFI_SUCCESS.
> > //
> > - if (CpuMpData->RunningCount == CpuMpData->StartCount) {
> > + if (CpuMpData->RunningCount == 0) {
> > return EFI_SUCCESS;
> > }
> >
> > @@ -1466,7 +1466,7 @@ CheckAllAPs (
> > //
> > if (CpuMpData->FailedCpuList != NULL) {
> > *CpuMpData->FailedCpuList =
> > - AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount
> + 1) * sizeof (UINTN));
> > + AllocatePool ((CpuMpData->RunningCount + 1) * sizeof
> > + (UINTN));
> > ASSERT (*CpuMpData->FailedCpuList != NULL);
> > }
> > ListIndex = 0;
> > @@ -2212,7 +2212,7 @@ StartupAllAPsWorker (
> > return EFI_NOT_STARTED;
> > }
> >
> > - CpuMpData->StartCount = 0;
> > + CpuMpData->RunningCount = 0;
> > for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount;
> ProcessorNumber++) {
> > CpuData = &CpuMpData->CpuData[ProcessorNumber];
> > CpuData->Waiting = FALSE;
> > @@ -2222,7 +2222,7 @@ StartupAllAPsWorker (
> > // Mark this processor as responsible for current calling.
> > //
> > CpuData->Waiting = TRUE;
> > - CpuMpData->StartCount++;
> > + CpuMpData->RunningCount++;
> > }
> > }
> > }
> > @@ -2231,7 +2231,6 @@ StartupAllAPsWorker (
> > CpuMpData->ProcArguments = ProcedureArgument;
> > CpuMpData->SingleThread = SingleThread;
> > CpuMpData->FinishedCount = 0;
> > - CpuMpData->RunningCount = 0;
> > CpuMpData->FailedCpuList = FailedCpuList;
> > CpuMpData->ExpectedTime = CalculateTimeout (
> > TimeoutInMicroseconds, diff --git
> > a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 962bce685d..5002b7e9c0 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -211,9 +211,8 @@ struct _CPU_MP_DATA {
> > UINTN BackupBuffer;
> > UINTN BackupBufferSize;
> >
> > - volatile UINT32 StartCount;
> > volatile UINT32 FinishedCount;
> > - volatile UINT32 RunningCount;
> > + UINT32 RunningCount;
> > BOOLEAN SingleThread;
> > EFI_AP_PROCEDURE Procedure;
> > VOID *ProcArguments;
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
2018-07-25 7:50 ` [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs Eric Dong
@ 2018-07-25 12:11 ` Laszlo Ersek
2018-07-25 12:44 ` Dong, Eric
2018-07-26 8:36 ` Ni, Ruiyu
1 sibling, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2018-07-25 12:11 UTC (permalink / raw)
To: Eric Dong, edk2-devel; +Cc: Ruiyu Ni
Hi Eric,
On 07/25/18 09:50, Eric Dong wrote:
> Base on UEFI spec requirement, StartAllAPs function should not
> use the APs which has been disabled before. This patch just
> change current code to follow this rule.
>
> V3 changes:
> Only called by StartUpAllAps, WakeUpAp will not wake up
> the disabled APs, in other cases also need to include the
> disabled APs, such as CpuDxe driver start up and
> ChangeApLoopCallback function.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +-
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 27 +++++++++++++++++----------
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +++-
> 3 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index d82e9aea45..c17daa0fc0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -306,7 +306,7 @@ MpInitChangeApLoopCallback (
> CpuMpData->PmCodeSegment = GetProtectedModeCS ();
> CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
> mNumberToFinish = CpuMpData->CpuCount - 1;
> - WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
> + WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE);
> while (mNumberToFinish > 0) {
> CpuPause ();
> }
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 0e57cc86bf..1a81062a3f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -470,7 +470,7 @@ CollectProcessorCount (
> //
> CpuMpData->InitFlag = ApInitConfig;
> CpuMpData->X2ApicEnable = FALSE;
> - WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL);
> + WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
> CpuMpData->InitFlag = ApInitDone;
> ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> //
> @@ -491,7 +491,7 @@ CollectProcessorCount (
> //
> // Wakeup all APs to enable x2APIC mode
> //
> - WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL);
> + WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL, TRUE);
> //
> // Wait for all known APs finished
> //
> @@ -969,6 +969,7 @@ FreeResetVector (
> @param[in] ProcessorNumber The handle number of specified processor
> @param[in] Procedure The function to be invoked by AP
> @param[in] ProcedureArgument The argument to be passed into AP function
> + @param[in] WakeUpDisabledAps Whether need to wake up disabled APs in broadcast mode.
> **/
> VOID
> WakeUpAP (
> @@ -976,7 +977,8 @@ WakeUpAP (
> IN BOOLEAN Broadcast,
> IN UINTN ProcessorNumber,
> IN EFI_AP_PROCEDURE Procedure, OPTIONAL
> - IN VOID *ProcedureArgument OPTIONAL
> + IN VOID *ProcedureArgument, OPTIONAL
> + IN BOOLEAN WakeUpDisabledAps
> )
> {
> volatile MP_CPU_EXCHANGE_INFO *ExchangeInfo;
> @@ -1010,6 +1012,10 @@ WakeUpAP (
> for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> if (Index != CpuMpData->BspNumber) {
> CpuData = &CpuMpData->CpuData[Index];
> + if (GetApState (CpuData) == CpuStateDisabled && !WakeUpDisabledAps) {
> + continue;
> + }
> +
> CpuData->ApFunction = (UINTN) Procedure;
> CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
> SetApState (CpuData, CpuStateReady);
I think something *might* be missing from the patch here, but I'm not sure.
Namely, is it possible that we can reach WakeUpAP() with:
(Broadcast && WakeUpDisabledAps)
*after* some APs have been disabled?
Because, in that case, we set the AP state from Disabled to Ready (and
the AP will perform the Procedure as necessary) -- however, once the AP
is done, we do not seem to re-set its state to Disabled.
Is that correct?
I tried to collect all the WakeUpAp() calls that pass TRUE for both
booleans mentioned above. Their callers are as follows:
- MpInitLibInitialize()
- CollectProcessorCount()
- MpInitChangeApLoopCallback()
>From these three, MpInitLibInitialize() and CollectProcessorCount() run
before the protocol or PPI user has any chance to disable a CPU, so it
looks impossible to reach the problematic code path I'm describing from
those functions.
What about MpInitChangeApLoopCallback()?... Hm, that function is only
called as a notification function for:
- the exit-boot-services event, and
- the legacy boot event
After which the MP protocol is unusable anyway, so it doesn't matter if
we don't flip the AP status back to Disabled.
OK, so this patch looks correct to me; can you please update the commit
message:
"WakeUpAP() is called with (Broadcast && WakeUpDisabledAps) from
MpInitLibInitialize(), CollectProcessorCount() and
MpInitChangeApLoopCallback() only. The first two run before the PPI or
Protocol user has a chance to disable any APs. The last one runs in
response to the ExitBootServices and LegacyBoot events, after which the
MP protocol is unusable. For this reason, it doesn't matter that an
originally disabled AP's state is not restored to Disabled, when
WakeUpAP() is called with (Broadcast && WakeUpDisabledAps)."
Thanks!
Laszlo
> @@ -1289,7 +1295,7 @@ ResetProcessorToIdleState (
> CpuMpData = GetCpuMpData ();
>
> CpuMpData->InitFlag = ApInitReconfig;
> - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL);
> + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL, TRUE);
> while (CpuMpData->FinishedCount < 1) {
> CpuPause ();
> }
> @@ -1439,7 +1445,8 @@ CheckAllAPs (
> FALSE,
> (UINT32) NextProcessorNumber,
> CpuMpData->Procedure,
> - CpuMpData->ProcArguments
> + CpuMpData->ProcArguments,
> + TRUE
> );
> }
> }
> @@ -1711,7 +1718,7 @@ MpInitLibInitialize (
> //
> // Wakeup APs to do some AP initialize sync
> //
> - WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData);
> + WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
> //
> // Wait for all APs finished initialization
> //
> @@ -1906,7 +1913,7 @@ SwitchBSPWorker (
> //
> // Need to wakeUp AP (future BSP).
> //
> - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, CpuMpData);
> + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, CpuMpData, TRUE);
>
> AsmExchangeRole (&CpuMpData->BSPInfo, &CpuMpData->APInfo);
>
> @@ -2240,14 +2247,14 @@ StartupAllAPsWorker (
> CpuMpData->WaitEvent = WaitEvent;
>
> if (!SingleThread) {
> - WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument);
> + WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument, FALSE);
> } else {
> for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; ProcessorNumber++) {
> if (ProcessorNumber == CallerNumber) {
> continue;
> }
> if (CpuMpData->CpuData[ProcessorNumber].Waiting) {
> - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument);
> + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument, TRUE);
> break;
> }
> }
> @@ -2359,7 +2366,7 @@ StartupThisAPWorker (
> CpuData->ExpectedTime = CalculateTimeout (TimeoutInMicroseconds, &CpuData->CurrentTime);
> CpuData->TotalTime = 0;
>
> - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument);
> + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument, TRUE);
>
> //
> // If WaitEvent is NULL, execute in blocking mode.
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 5002b7e9c0..fe7a917e49 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -373,6 +373,7 @@ GetModeTransitionBuffer (
> @param[in] ProcessorNumber The handle number of specified processor
> @param[in] Procedure The function to be invoked by AP
> @param[in] ProcedureArgument The argument to be passed into AP function
> + @param[in] WakeUpDisabledAps Whether need to wake up disabled APs in broadcast mode.
> **/
> VOID
> WakeUpAP (
> @@ -380,7 +381,8 @@ WakeUpAP (
> IN BOOLEAN Broadcast,
> IN UINTN ProcessorNumber,
> IN EFI_AP_PROCEDURE Procedure, OPTIONAL
> - IN VOID *ProcedureArgument OPTIONAL
> + IN VOID *ProcedureArgument, OPTIONAL
> + IN BOOLEAN WakeUpDisabledAps OPTIONAL
> );
>
> /**
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch V3 0/3] StartAllAPs should not use disabled APs
2018-07-25 7:50 [Patch V3 0/3] StartAllAPs should not use disabled APs Eric Dong
` (2 preceding siblings ...)
2018-07-25 7:50 ` [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs Eric Dong
@ 2018-07-25 12:12 ` Laszlo Ersek
2018-07-25 15:18 ` Laszlo Ersek
3 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2018-07-25 12:12 UTC (permalink / raw)
To: Eric Dong, edk2-devel
On 07/25/18 09:50, Eric Dong wrote:
> This patch series include changes:
> 1. StartAllAPs should not use disabled APs, this is required by UEFI spec.
> 2. Refine the code to remove the redundant definitions.
>
> V2 changes:
> Use CpuStateReady to distinguish the AP state from CpuStateIdle.
>
> V3 Changes:
> Only change 3/3 patch. Only not use disabled APs when WakeUpAP called
> by StartAllAps function. In other cases, also include disabled APs.
>
> Eric Dong (3):
> UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
> UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.
> UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
>
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 33 +++++++++++++++------------------
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +---
> 2 files changed, 16 insertions(+), 21 deletions(-)
>
I requested commit message updates for all three patches. With those
implemented, please add:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(No need to repost.)
Please give me some more time to regression-test this series as well.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
2018-07-25 12:11 ` Laszlo Ersek
@ 2018-07-25 12:44 ` Dong, Eric
0 siblings, 0 replies; 17+ messages in thread
From: Dong, Eric @ 2018-07-25 12:44 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, July 25, 2018 8:12 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP
> when call StartAllAPs.
>
> Hi Eric,
>
> On 07/25/18 09:50, Eric Dong wrote:
> > Base on UEFI spec requirement, StartAllAPs function should not use the
> > APs which has been disabled before. This patch just change current
> > code to follow this rule.
> >
> > V3 changes:
> > Only called by StartUpAllAps, WakeUpAp will not wake up the disabled
> > APs, in other cases also need to include the disabled APs, such as
> > CpuDxe driver start up and ChangeApLoopCallback function.
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +-
> > UefiCpuPkg/Library/MpInitLib/MpLib.c | 27 +++++++++++++++++----------
> > UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +++-
> > 3 files changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > index d82e9aea45..c17daa0fc0 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > @@ -306,7 +306,7 @@ MpInitChangeApLoopCallback (
> > CpuMpData->PmCodeSegment = GetProtectedModeCS ();
> > CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
> > mNumberToFinish = CpuMpData->CpuCount - 1;
> > - WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
> > + WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE);
> > while (mNumberToFinish > 0) {
> > CpuPause ();
> > }
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 0e57cc86bf..1a81062a3f 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -470,7 +470,7 @@ CollectProcessorCount (
> > //
> > CpuMpData->InitFlag = ApInitConfig;
> > CpuMpData->X2ApicEnable = FALSE;
> > - WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL);
> > + WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
> > CpuMpData->InitFlag = ApInitDone;
> > ASSERT (CpuMpData->CpuCount <= PcdGet32
> (PcdCpuMaxLogicalProcessorNumber));
> > //
> > @@ -491,7 +491,7 @@ CollectProcessorCount (
> > //
> > // Wakeup all APs to enable x2APIC mode
> > //
> > - WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL);
> > + WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL, TRUE);
> > //
> > // Wait for all known APs finished
> > //
> > @@ -969,6 +969,7 @@ FreeResetVector (
> > @param[in] ProcessorNumber The handle number of specified
> processor
> > @param[in] Procedure The function to be invoked by AP
> > @param[in] ProcedureArgument The argument to be passed into AP
> > function
> > + @param[in] WakeUpDisabledAps Whether need to wake up disabled APs
> in broadcast mode.
> > **/
> > VOID
> > WakeUpAP (
> > @@ -976,7 +977,8 @@ WakeUpAP (
> > IN BOOLEAN Broadcast,
> > IN UINTN ProcessorNumber,
> > IN EFI_AP_PROCEDURE Procedure, OPTIONAL
> > - IN VOID *ProcedureArgument OPTIONAL
> > + IN VOID *ProcedureArgument, OPTIONAL
> > + IN BOOLEAN WakeUpDisabledAps
> > )
> > {
> > volatile MP_CPU_EXCHANGE_INFO *ExchangeInfo;
> > @@ -1010,6 +1012,10 @@ WakeUpAP (
> > for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> > if (Index != CpuMpData->BspNumber) {
> > CpuData = &CpuMpData->CpuData[Index];
> > + if (GetApState (CpuData) == CpuStateDisabled
> && !WakeUpDisabledAps) {
> > + continue;
> > + }
> > +
> > CpuData->ApFunction = (UINTN) Procedure;
> > CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
> > SetApState (CpuData, CpuStateReady);
>
> I think something *might* be missing from the patch here, but I'm not sure.
>
> Namely, is it possible that we can reach WakeUpAP() with:
>
> (Broadcast && WakeUpDisabledAps)
>
> *after* some APs have been disabled?
>
> Because, in that case, we set the AP state from Disabled to Ready (and the AP
> will perform the Procedure as necessary) -- however, once the AP is done, we
> do not seem to re-set its state to Disabled.
>
> Is that correct?
Yes, for current use cases, we don't have a valid case need to re-set the AP to Disabled state.
>
> I tried to collect all the WakeUpAp() calls that pass TRUE for both booleans
> mentioned above. Their callers are as follows:
>
> - MpInitLibInitialize()
> - CollectProcessorCount()
> - MpInitChangeApLoopCallback()
>
> From these three, MpInitLibInitialize() and CollectProcessorCount() run
> before the protocol or PPI user has any chance to disable a CPU, so it looks
> impossible to reach the problematic code path I'm describing from those
> functions.
>
> What about MpInitChangeApLoopCallback()?... Hm, that function is only
> called as a notification function for:
>
> - the exit-boot-services event, and
> - the legacy boot event
>
> After which the MP protocol is unusable anyway, so it doesn't matter if we
> don't flip the AP status back to Disabled.
>
> OK, so this patch looks correct to me; can you please update the commit
> message:
>
> "WakeUpAP() is called with (Broadcast && WakeUpDisabledAps) from
> MpInitLibInitialize(), CollectProcessorCount() and
> MpInitChangeApLoopCallback() only. The first two run before the PPI or
> Protocol user has a chance to disable any APs. The last one runs in response to
> the ExitBootServices and LegacyBoot events, after which the MP protocol is
> unusable. For this reason, it doesn't matter that an originally disabled AP's
> state is not restored to Disabled, when
> WakeUpAP() is called with (Broadcast && WakeUpDisabledAps)."
>
Good suggestion, will include it in the commit messages.
> Thanks!
> Laszlo
>
> > @@ -1289,7 +1295,7 @@ ResetProcessorToIdleState (
> > CpuMpData = GetCpuMpData ();
> >
> > CpuMpData->InitFlag = ApInitReconfig;
> > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL);
> > + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL, TRUE);
> > while (CpuMpData->FinishedCount < 1) {
> > CpuPause ();
> > }
> > @@ -1439,7 +1445,8 @@ CheckAllAPs (
> > FALSE,
> > (UINT32) NextProcessorNumber,
> > CpuMpData->Procedure,
> > - CpuMpData->ProcArguments
> > + CpuMpData->ProcArguments,
> > + TRUE
> > );
> > }
> > }
> > @@ -1711,7 +1718,7 @@ MpInitLibInitialize (
> > //
> > // Wakeup APs to do some AP initialize sync
> > //
> > - WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData);
> > + WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData,
> > + TRUE);
> > //
> > // Wait for all APs finished initialization
> > //
> > @@ -1906,7 +1913,7 @@ SwitchBSPWorker (
> > //
> > // Need to wakeUp AP (future BSP).
> > //
> > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc,
> > CpuMpData);
> > + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc,
> > + CpuMpData, TRUE);
> >
> > AsmExchangeRole (&CpuMpData->BSPInfo, &CpuMpData->APInfo);
> >
> > @@ -2240,14 +2247,14 @@ StartupAllAPsWorker (
> > CpuMpData->WaitEvent = WaitEvent;
> >
> > if (!SingleThread) {
> > - WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument);
> > + WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument,
> > + FALSE);
> > } else {
> > for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount;
> ProcessorNumber++) {
> > if (ProcessorNumber == CallerNumber) {
> > continue;
> > }
> > if (CpuMpData->CpuData[ProcessorNumber].Waiting) {
> > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure,
> ProcedureArgument);
> > + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure,
> > + ProcedureArgument, TRUE);
> > break;
> > }
> > }
> > @@ -2359,7 +2366,7 @@ StartupThisAPWorker (
> > CpuData->ExpectedTime = CalculateTimeout (TimeoutInMicroseconds,
> &CpuData->CurrentTime);
> > CpuData->TotalTime = 0;
> >
> > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure,
> > ProcedureArgument);
> > + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure,
> > + ProcedureArgument, TRUE);
> >
> > //
> > // If WaitEvent is NULL, execute in blocking mode.
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 5002b7e9c0..fe7a917e49 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -373,6 +373,7 @@ GetModeTransitionBuffer (
> > @param[in] ProcessorNumber The handle number of specified
> processor
> > @param[in] Procedure The function to be invoked by AP
> > @param[in] ProcedureArgument The argument to be passed into AP
> > function
> > + @param[in] WakeUpDisabledAps Whether need to wake up disabled APs
> in broadcast mode.
> > **/
> > VOID
> > WakeUpAP (
> > @@ -380,7 +381,8 @@ WakeUpAP (
> > IN BOOLEAN Broadcast,
> > IN UINTN ProcessorNumber,
> > IN EFI_AP_PROCEDURE Procedure, OPTIONAL
> > - IN VOID *ProcedureArgument OPTIONAL
> > + IN VOID *ProcedureArgument, OPTIONAL
> > + IN BOOLEAN WakeUpDisabledAps OPTIONAL
> > );
> >
> > /**
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.
2018-07-25 12:09 ` Dong, Eric
@ 2018-07-25 15:18 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2018-07-25 15:18 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu
On 07/25/18 14:09, Dong, Eric wrote:
> Hi Laszlo,
>
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, July 25, 2018 7:47 PM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount
>> and volatile definition.
>>
>> Hi Eric,
>>
>> On 07/25/18 09:50, Eric Dong wrote:
>>> The StartCount is duplicated with RunningCount, replace it with
>>> RunningCount. Also the volatile for RunningCount is not needed.
>>
>> after staring at this patch for a long time, I think it is correct.
>> However, I suggest that we improve the commit message, because this patch
>> actually does three things:
>>
>> (1) It removes "volatile" from RunningCount.
>>
>> [This is OK, because only the BSP modifies it.]
>>
>> (2) [This is the tricky part.]
>>
>> When we detect a timeout in CheckAllAPs(), and collect the list of failed CPUs,
>> the size of the list is derived from the following difference, before the patch:
>>
>> StartCount - FinishedCount
>>
>> where "StartCount" is set by the BSP at startup, and FinishedCount is
>> incremented by the APs themselves.
>>
>
> I think FinishedCount used here is a typo. What the logic from the code context here is want to get the failed Aps and return them. So it should use RunningCount here, right? Also the FinishedCount may be bigger than StartCount if many Aps has been disabled. Right?
I agree FinishedCount looks questionable / out of place in the original
code.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch V3 0/3] StartAllAPs should not use disabled APs
2018-07-25 12:12 ` [Patch V3 0/3] StartAllAPs should not use disabled APs Laszlo Ersek
@ 2018-07-25 15:18 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2018-07-25 15:18 UTC (permalink / raw)
To: Eric Dong, edk2-devel
On 07/25/18 14:12, Laszlo Ersek wrote:
> On 07/25/18 09:50, Eric Dong wrote:
>> This patch series include changes:
>> 1. StartAllAPs should not use disabled APs, this is required by UEFI spec.
>> 2. Refine the code to remove the redundant definitions.
>>
>> V2 changes:
>> Use CpuStateReady to distinguish the AP state from CpuStateIdle.
>>
>> V3 Changes:
>> Only change 3/3 patch. Only not use disabled APs when WakeUpAP called
>> by StartAllAps function. In other cases, also include disabled APs.
>>
>> Eric Dong (3):
>> UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
>> UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.
>> UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
>>
>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 33 +++++++++++++++------------------
>> UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +---
>> 2 files changed, 16 insertions(+), 21 deletions(-)
>>
>
> I requested commit message updates for all three patches. With those
> implemented, please add:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> (No need to repost.)
>
> Please give me some more time to regression-test this series as well.
This series works nicely on my end. I performed my usual Linux guest
tests, in all my usual guest environments, plus 10 cold boots in each
environment. Everything worked fine.
For patches #1 and #2:
Tested-by: Laszlo Ersek <lersek@redhat.com>
For patch #3:
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
(I'm giving R-t-b and not T-b for patch #3 because OVMF doesn't exercise
the MP PPI/Protocol member functions that disable APs.)
>From my side, please update the commit messages as agreed, and go ahead
with the pushing.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
2018-07-25 11:15 ` Dong, Eric
@ 2018-07-26 5:18 ` Ni, Ruiyu
0 siblings, 0 replies; 17+ messages in thread
From: Ni, Ruiyu @ 2018-07-26 5:18 UTC (permalink / raw)
To: Dong, Eric, Laszlo Ersek, edk2-devel@lists.01.org
Eric,
Please also include the state machine in comments for ENUM CPU_STATE definition.
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Thanks/Ray
> -----Original Message-----
> From: Dong, Eric
> Sent: Wednesday, July 25, 2018 7:15 PM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: RE: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant
> CpuStateFinished State.
>
> Hi Laszlo,
>
>
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Wednesday, July 25, 2018 6:55 PM
> > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: Re: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove
> > redundant CpuStateFinished State.
> >
> > Hi Eric,
> >
> > On 07/25/18 09:50, Eric Dong wrote:
> > > Current CPU state definition include CpuStateIdle and CpuStateFinished.
> > > After investigation, current code can use CpuStateIdle to replace
> > > the CpuStateFinished. It will reduce the state number and easy for
> maintenance.
> > >
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > > ---
> > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 18 ++++++++----------
> > > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 -
> > > 2 files changed, 8 insertions(+), 11 deletions(-)
> >
> > After looking over this patch, it seems that you are preserving the
> > CpuStateReady enum constant, relative to:
> >
> >
> > http://mid.mail-archive.com/20180628112920.5296-1-eric.dong@intel.com
> >
> > However, based on your analysis in
> >
> > http://mid.mail-
> > archive.com/ED077930C258884BBCB450DB737E66224AC5A453@shsmsx102.
> > ccr.corp.intel.com
> >
> > isn't it still possible to run into the exact same issue? (Namely, BSP
> > thinks the AP has gone through Idle -> Busy -> Idle, but the AP has
> > never actually left
> > Idle?)
> >
> > Hm, wait, is it the case that the BSP first sets Ready, and so if the
> > check for an AP returns Idle, it implies the AP must have gone through:
> >
> > Idle ----> Ready ----> Busy ----> Idle
> >
> > ?
>
> Correct! The Ready state is begin state and the Idle is the finish state.
>
> >
> > If this is correct, can you please include the following in the commit
> > message:
> >
> > > Before this patch, the state transitions for an AP are:
> > >
> > > Idle ----> Ready ----> Busy ----> Finished ----> Idle
> > > [BSP] [AP] [AP] [BSP]
> > >
> > > After the patch, the state transitions for an AP are:
> > >
> > > Idle ----> Ready ----> Busy ----> Idle
> > > [BSP] [AP] [AP]
> >
> > Do you agree?
>
> Good suggestion, I will include this info in the commit message.
>
> >
> > I have another question:
> >
> > On 07/25/18 09:50, Eric Dong wrote:
> > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > index c82b985943..ff09a0e9e7 100644
> > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > @@ -696,7 +696,7 @@ ApWakeupFunction (
> > > }
> > > }
> > > }
> > > - SetApState (&CpuMpData->CpuData[ProcessorNumber],
> > CpuStateFinished);
> > > + SetApState (&CpuMpData->CpuData[ProcessorNumber],
> > > + CpuStateIdle);
> > > }
> > > }
> > >
> > > @@ -1352,18 +1352,17 @@ CheckThisAP (
> > > CpuData = &CpuMpData->CpuData[ProcessorNumber];
> > >
> > > //
> > > - // Check the CPU state of AP. If it is CpuStateFinished, then
> > > the AP has
> > finished its task.
> > > + // Check the CPU state of AP. If it is CpuStateIdle, then the AP
> > > + has
> > finished its task.
> > > // Only BSP and corresponding AP access this unit of CPU Data.
> > > This means the AP will not modify the
> > > - // value of state after setting the it to CpuStateFinished, so
> > > BSP can safely
> > make use of its value.
> > > + // value of state after setting the it to CpuStateIdle, so BSP
> > > + can safely
> > make use of its value.
> > > //
> > > //
> > > // If the AP finishes for StartupThisAP(), return EFI_SUCCESS.
> > > //
> > > - if (GetApState(CpuData) == CpuStateFinished) {
> > > + if (GetApState(CpuData) == CpuStateIdle) {
> > > if (CpuData->Finished != NULL) {
> > > *(CpuData->Finished) = TRUE;
> > > }
> > > - SetApState (CpuData, CpuStateIdle);
> > > return EFI_SUCCESS;
> > > } else {
> > > //
> > > @@ -1420,14 +1419,13 @@ CheckAllAPs (
> > >
> > > CpuData = &CpuMpData->CpuData[ProcessorNumber];
> > > //
> > > - // Check the CPU state of AP. If it is CpuStateFinished, then the AP has
> > finished its task.
> > > + // Check the CPU state of AP. If it is CpuStateIdle, then the
> > > + AP has
> > finished its task.
> > > // Only BSP and corresponding AP access this unit of CPU Data.
> > > This
> > means the AP will not modify the
> > > - // value of state after setting the it to CpuStateFinished, so BSP can
> > safely make use of its value.
> > > + // value of state after setting the it to CpuStateIdle, so BSP
> > > + can safely
> > make use of its value.
> > > //
> > > - if (GetApState(CpuData) == CpuStateFinished) {
> > > + if (GetApState(CpuData) == CpuStateIdle) {
> > > CpuMpData->RunningCount ++;
> > > CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> > > - SetApState(CpuData, CpuStateIdle);
> > >
> > > //
> > > // If in Single Thread mode, then search for the next waiting
> > > AP for
> > execution.
> >
> > This part of the code, after the patch, does not seem idempotent; in
> > other words, if the BSP calls CheckAllAPs() multiple times, then
> > RunningCount will be increased every time. Before the patch, this
> > wasn't the case, because after the Finished -> Idle transition, the increment
> wouldn't be reached again.
> >
> > Hmmm, wait, I'm wrong: we set the Waiting field to FALSE as well, so
> > at the next call to CheckAllAPs(), we'll take the early "continue" branch.
> > Looks OK after all.
> >
>
> Yes, we have two flags here. Waiting flags means the AP will do the task. State
> flag means whether the task has finished.
> Both flags will be checked and updated.
>
> > I'll follow up with test results.
> >
> > Thanks,
> > Laszlo
> >
> > > @@ -1923,7 +1921,7 @@ SwitchBSPWorker (
> > > //
> > > // Wait for old BSP finished AP task
> > > //
> > > - while (GetApState (&CpuMpData->CpuData[CallerNumber]) !=
> > > CpuStateFinished) {
> > > + while (GetApState (&CpuMpData->CpuData[CallerNumber]) !=
> > > + CpuStateIdle) {
> > > CpuPause ();
> > > }
> > >
> > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > > index 9d0b866d09..962bce685d 100644
> > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > > @@ -85,7 +85,6 @@ typedef enum {
> > > CpuStateIdle,
> > > CpuStateReady,
> > > CpuStateBusy,
> > > - CpuStateFinished,
> > > CpuStateDisabled
> > > } CPU_STATE;
> > >
> > >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.
2018-07-25 7:50 ` [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition Eric Dong
2018-07-25 11:47 ` Laszlo Ersek
@ 2018-07-26 5:22 ` Ni, Ruiyu
1 sibling, 0 replies; 17+ messages in thread
From: Ni, Ruiyu @ 2018-07-26 5:22 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Thanks/Ray
> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Eric Dong
> Sent: Wednesday, July 25, 2018 3:50 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and
> volatile definition.
>
> The StartCount is duplicated with RunningCount, replace it with RunningCount.
> Also the volatile for RunningCount is not needed.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +++++------
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +--
> 2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index ff09a0e9e7..0e57cc86bf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1424,7 +1424,7 @@ CheckAllAPs (
> // value of state after setting the it to CpuStateIdle, so BSP can safely make
> use of its value.
> //
> if (GetApState(CpuData) == CpuStateIdle) {
> - CpuMpData->RunningCount ++;
> + CpuMpData->RunningCount --;
> CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
>
> //
> @@ -1449,7 +1449,7 @@ CheckAllAPs (
> //
> // If all APs finish, return EFI_SUCCESS.
> //
> - if (CpuMpData->RunningCount == CpuMpData->StartCount) {
> + if (CpuMpData->RunningCount == 0) {
> return EFI_SUCCESS;
> }
>
> @@ -1466,7 +1466,7 @@ CheckAllAPs (
> //
> if (CpuMpData->FailedCpuList != NULL) {
> *CpuMpData->FailedCpuList =
> - AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount + 1)
> * sizeof (UINTN));
> + AllocatePool ((CpuMpData->RunningCount + 1) * sizeof (UINTN));
> ASSERT (*CpuMpData->FailedCpuList != NULL);
> }
> ListIndex = 0;
> @@ -2212,7 +2212,7 @@ StartupAllAPsWorker (
> return EFI_NOT_STARTED;
> }
>
> - CpuMpData->StartCount = 0;
> + CpuMpData->RunningCount = 0;
> for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount;
> ProcessorNumber++) {
> CpuData = &CpuMpData->CpuData[ProcessorNumber];
> CpuData->Waiting = FALSE;
> @@ -2222,7 +2222,7 @@ StartupAllAPsWorker (
> // Mark this processor as responsible for current calling.
> //
> CpuData->Waiting = TRUE;
> - CpuMpData->StartCount++;
> + CpuMpData->RunningCount++;
> }
> }
> }
> @@ -2231,7 +2231,6 @@ StartupAllAPsWorker (
> CpuMpData->ProcArguments = ProcedureArgument;
> CpuMpData->SingleThread = SingleThread;
> CpuMpData->FinishedCount = 0;
> - CpuMpData->RunningCount = 0;
> CpuMpData->FailedCpuList = FailedCpuList;
> CpuMpData->ExpectedTime = CalculateTimeout (
> TimeoutInMicroseconds, diff --git
> a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 962bce685d..5002b7e9c0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -211,9 +211,8 @@ struct _CPU_MP_DATA {
> UINTN BackupBuffer;
> UINTN BackupBufferSize;
>
> - volatile UINT32 StartCount;
> volatile UINT32 FinishedCount;
> - volatile UINT32 RunningCount;
> + UINT32 RunningCount;
> BOOLEAN SingleThread;
> EFI_AP_PROCEDURE Procedure;
> VOID *ProcArguments;
> --
> 2.15.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
2018-07-25 7:50 ` [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs Eric Dong
2018-07-25 12:11 ` Laszlo Ersek
@ 2018-07-26 8:36 ` Ni, Ruiyu
2018-07-26 8:36 ` Ni, Ruiyu
1 sibling, 1 reply; 17+ messages in thread
From: Ni, Ruiyu @ 2018-07-26 8:36 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek
> + if (GetApState (CpuData) == CpuStateDisabled && !WakeUpDisabledAps) {
> + continue;
> + }
> +
> CpuData->ApFunction = (UINTN) Procedure;
> CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
> SetApState (CpuData, CpuStateReady);
Eric, can you add more comments here to say ALL AP will be woke up by INIT-SIPI-SIPI,
but the AP procedure will be skipped when AP state is not Ready.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
2018-07-26 8:36 ` Ni, Ruiyu
@ 2018-07-26 8:36 ` Ni, Ruiyu
0 siblings, 0 replies; 17+ messages in thread
From: Ni, Ruiyu @ 2018-07-26 8:36 UTC (permalink / raw)
To: Dong, Eric, 'edk2-devel@lists.01.org'; +Cc: 'Laszlo Ersek'
With that, Reviewed-by: Ruiyu Ni <Ruiyu.ni@intel.com>
Thanks/Ray
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Thursday, July 26, 2018 4:36 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call
> StartAllAPs.
>
> > + if (GetApState (CpuData) == CpuStateDisabled && !WakeUpDisabledAps)
> {
> > + continue;
> > + }
> > +
> > CpuData->ApFunction = (UINTN) Procedure;
> > CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
> > SetApState (CpuData, CpuStateReady);
>
> Eric, can you add more comments here to say ALL AP will be woke up by INIT-
> SIPI-SIPI, but the AP procedure will be skipped when AP state is not Ready.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-07-26 8:37 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-25 7:50 [Patch V3 0/3] StartAllAPs should not use disabled APs Eric Dong
2018-07-25 7:50 ` [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State Eric Dong
2018-07-25 10:54 ` Laszlo Ersek
2018-07-25 11:15 ` Dong, Eric
2018-07-26 5:18 ` Ni, Ruiyu
2018-07-25 7:50 ` [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition Eric Dong
2018-07-25 11:47 ` Laszlo Ersek
2018-07-25 12:09 ` Dong, Eric
2018-07-25 15:18 ` Laszlo Ersek
2018-07-26 5:22 ` Ni, Ruiyu
2018-07-25 7:50 ` [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs Eric Dong
2018-07-25 12:11 ` Laszlo Ersek
2018-07-25 12:44 ` Dong, Eric
2018-07-26 8:36 ` Ni, Ruiyu
2018-07-26 8:36 ` Ni, Ruiyu
2018-07-25 12:12 ` [Patch V3 0/3] StartAllAPs should not use disabled APs Laszlo Ersek
2018-07-25 15:18 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox