Hi Laszlo,
> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, July 25, 2018 6:55 PM > To: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@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 <ler...@redhat.com> > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong <eric.d...@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: > > 20180628112920.5296-1-eric.dong@intel.com">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; > > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
