Eric, Please also include the state machine in comments for ENUM CPU_STATE definition.
Reviewed-by: Ruiyu Ni <[email protected]> Thanks/Ray > -----Original Message----- > From: Dong, Eric > Sent: Wednesday, July 25, 2018 7:15 PM > To: Laszlo Ersek <[email protected]>; [email protected] > Cc: Ni, Ruiyu <[email protected]> > Subject: RE: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant > CpuStateFinished State. > > Hi Laszlo, > > > > -----Original Message----- > > From: Laszlo Ersek [mailto:[email protected]] > > Sent: Wednesday, July 25, 2018 6:55 PM > > To: Dong, Eric <[email protected]>; [email protected] > > Cc: Ni, Ruiyu <[email protected]> > > 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 <[email protected]> > > > Cc: Ruiyu Ni <[email protected]> > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Eric Dong <[email protected]> > > > --- > > > 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: > > > > > > [email protected]">http://mid.mail-archive.com/[email protected] > > > > 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; > > > > > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
