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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

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