On 10/28/15 07:42, Jeff Fan wrote: > There is one dead lock issue between BSP and AP. AP has one logic to enter > into > Sleep state (hlt) if it found its state is Idle in one loop. > In StartupThisAp() function, BSP will get current AP state (Sleep or Idle) to > know how to wakeup AP. If AP is in Idle state, BSP will write one semaphore to > wakeup AP. If AP is in Sleep state, BSP will send INIT-SIPI-SIPI to wakeup AP. > The dead lock may happen if BSP found AP is in Idle state firstly but AP > changed > its state to Sleep state later. BSP cannot wakeup AP by writing semaphore. > This fix is to release lock till BSP finished setting the semaphore. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <[email protected]> > Cc: Michael Kinney <[email protected]> > Cc: Jordan Justen <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Cc: Chen Fan <[email protected]> > --- > UefiCpuPkg/CpuDxe/CpuMp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c > index 04c2f1f..3de2aa5 100644 > --- a/UefiCpuPkg/CpuDxe/CpuMp.c > +++ b/UefiCpuPkg/CpuDxe/CpuMp.c > @@ -940,15 +940,18 @@ StartupThisAP ( > return EFI_INVALID_PARAMETER; > } > > - CpuState = GetApState (CpuData); > + GetMpSpinLock (CpuData); > + CpuState = CpuData->State; > if (CpuState != CpuStateIdle && > CpuState != CpuStateSleeping) { > + ReleaseMpSpinLock (CpuData); > return EFI_NOT_READY; > } > + CpuData->State = CpuStateReady; > + CpuData->Parameter = ProcedureArgument; > + CpuData->Procedure = Procedure; > + ReleaseMpSpinLock (CpuData); > > - SetApState (CpuData, CpuStateReady); > - > - SetApProcedure (CpuData, Procedure, ProcedureArgument); > // > // If this AP previous state is Sleeping, we should > // wake up this AP by sent a SIPI. and avoid >
I think this change is good, but I think the subject and the last sentence of the commit message should be improved. (1) The subject should state that we are eliminating a TOCTTOU race condition. (The deadlock is only a consequence of the TOCTTOU race.) Something like: UefiCpuPkg/CpuDxe: Fix TOCTTOU race on CpuState in StartupThisAP The issue is that we release the spinlock between retrieving the AP state (Idle) and acting upon it (= changing it to Ready). In that window the AP can look at its own state (Idle) and perform actions (= change it to Sleeping, and go to sleep) that invalidate the planned actions (= set AP state to Ready) of the BSP. So the solution is to *not* release the spinlock between retrieving the AP's state and flipping it to CpuStateReady -- this will prevent the AP from changing the state from Idle to Sleep. Such state transitions should be atomic in general. (2) The last sentence of the commit message should be: The fix is to *hold* the lock till BSP finished setting the semaphore. (I assume that by "semaphore" you mean the CpuState field. I find that somewhat confusing; semaphores are usually up/down, or counting. But for this specific driver "semaphore" could be the traditional terminology.) With the subject and the last sentence changed: Reviewed-by: Laszlo Ersek <[email protected]> Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
