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 state in one loop.
> In ProcessorToIdleState() function, AP will get current AP state, if its state
> is Sleep state, then it will go into real sleep(hlt) state.
> The dead lock may happen if BSP found AP state is Sleep state and send
> INIT-SIPI
> -SIPI to AP. But AP just acquried the lock to get its state. If AP is reset,
> the
> lock will not released correctly.
> This fix is to let AP enter into real sleep(hlt) state immediately after it
> changes its state is set to Sleep and needn't get lock again.
>
> 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 | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> index 3de2aa5..357ee42 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -1289,14 +1289,18 @@ ProcessorToIdleState (
> GetMpSpinLock (CpuData);
> if (CpuData->State == CpuStateIdle) {
> CpuData->State = CpuStateSleeping;
> + ReleaseMpSpinLock (CpuData);
> + while (TRUE) {
> + //
> + // Place AP into cli-hlt loop to handle RSM from SMM case
> + //
> + DisableInterrupts ();
> + CpuSleep ();
> + }
> }
> ReleaseMpSpinLock (CpuData);
> }
>
> - if (GetApState (CpuData) == CpuStateSleeping) {
> - CpuSleep ();
> - }
> -
> CpuPause ();
> }
>
>
I think the idea in this patch is good, but I recommend a *much* more sweeping
change for this function. From my past multi-threaded programming experience,
it is always best to hold the lock *by default* in a critical section, and only
release it *temporarily* while yielding, or invoking a callback, or invoking a
blocking operation.
- It makes no sense to release and reacquire a mutex or splinlock if our thread
or CPU has actual, fast work to do, without wanting to yield intentionally.
- For loops especially, this means that the loop body is both *entered* and
*exited* with the lock held.
- This approach tends to improve throughput and happens to eliminate a *bunch*
of TOCTTOU races.
Therefore my proposal would be:
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> index 98976a1..31c6222 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -1288,40 +1288,42 @@ ProcessorToIdleState (
> CpuData->Procedure = NULL;
> }
> CpuData->State = CpuStateIdle;
> - ReleaseMpSpinLock (CpuData);
>
> while (TRUE) {
> - GetMpSpinLock (CpuData);
> ProcedureArgument = CpuData->Parameter;
> Procedure = CpuData->Procedure;
> - ReleaseMpSpinLock (CpuData);
>
> if (Procedure != NULL) {
> - SetApState (CpuData, CpuStateBusy);
> + CpuData->State = CpuStateBusy;
> + ReleaseMpSpinLock (CpuData);
>
> Procedure ((VOID*) ProcedureArgument);
>
> GetMpSpinLock (CpuData);
> CpuData->Procedure = NULL;
> CpuData->State = CpuStateFinished;
> - ReleaseMpSpinLock (CpuData);
> } else {
> //
> // if no procedure to execution, we simply put AP
> // into sleeping state, and waiting BSP sent SIPI.
> //
> - GetMpSpinLock (CpuData);
> if (CpuData->State == CpuStateIdle) {
> - CpuData->State = CpuStateSleeping;
> + CpuData->State = CpuStateSleeping;
> + ReleaseMpSpinLock (CpuData);
> +
> + //
> + // wait for INIT-SIPI-SIPI
> + //
> + while (TRUE) {
> + DisableInterrupts ();
> + CpuSleep ();
> + }
> }
> - ReleaseMpSpinLock (CpuData);
> - }
> -
> - if (GetApState (CpuData) == CpuStateSleeping) {
> - CpuSleep ();
> }
>
> + ReleaseMpSpinLock (CpuData);
> CpuPause ();
> + GetMpSpinLock (CpuData);
> }
>
> CpuSleep ();
Thank you,
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
