> On Oct 28, 2015, at 4:47 AM, Laszlo Ersek <[email protected]> wrote: > > 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.) >
Laszlo, I would call the CPU_STATE field the critical section of the CpuDataLock (type SPIN_LOCK). Reviewed-by: Andrew Fish <[email protected]> Thanks, Andrew Fish > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
