[Patch V2 0/3] StartAllAPs should not use disabled APs

[Patch V2 0/3] StartAllAPs should not use disabled APs

[Eric Dong]

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.

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

[Patch 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.

[Eric Dong]

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

[Patch 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.

[Eric Dong]

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

[Patch 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.

[Eric Dong]

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.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 0e57cc86bf..69f69f2b79 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1008,8 +1008,8 @@ WakeUpAP (
 
   if (Broadcast) {
     for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
-      if (Index != CpuMpData->BspNumber) {
-        CpuData = &CpuMpData->CpuData[Index];
+      CpuData = &CpuMpData->CpuData[Index];
+      if (Index != CpuMpData->BspNumber && (GetApState (CpuData) != CpuStateDisabled)) {
         CpuData->ApFunction         = (UINTN) Procedure;
         CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
         SetApState (CpuData, CpuStateReady);
-- 
2.15.0.windows.1