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

[Patch V3 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.

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

[Patch v3 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

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

[Laszlo Ersek]

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;
>
>

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

[Dong, Eric]

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;
> >
> >

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

[Ni, Ruiyu]

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;
> > >
> > >

[Patch v3 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

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

[Laszlo Ersek]

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;
> 

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

[Dong, Eric]

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;
> >

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

[Laszlo Ersek]

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

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

[Ni, Ruiyu]

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

[Patch v3 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.

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

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

[Laszlo Ersek]

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
>    );
>  
>  /**
> 

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

[Dong, Eric]

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
> >    );
> >
> >  /**
> >

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

[Ni, Ruiyu]

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

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

[Ni, Ruiyu]

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.

Re: [Patch V3 0/3] StartAllAPs should not use disabled APs

[Laszlo Ersek]

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

Re: [Patch V3 0/3] StartAllAPs should not use disabled APs

[Laszlo Ersek]

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