Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between APHandler's release of Busy spinlock and user-triggered SmmStartupThisAP's

["Ni, Ray" <ray.ni@intel.com>]

1. can we directly call AcquireSpinLock()? *OrFail() can be removed IMO.
2. It's a patch to change the behavior of SmmStartupThisAP(). So that to reduce the potential bugs in caller's code. Patch title is a bit mis-leading.

> -----Original Message-----
> From: Nikodem, Damian
> Sent: Tuesday, September 3, 2019 7:58 AM
> To: devel@edk2.groups.io
> Cc: Nikodem, Damian <damian.nikodem@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; You,
> Benjamin <benjamin.you@intel.com>; Laszlo Ersek <lersek@redhat.com>; Rusocki, Krzysztof <krzysztof.rusocki@intel.com>
> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between APHandler's release of Busy spinlock and user-
> triggered SmmStartupThisAP's
> 
> Race condition between APHandler's release of Busy spinlock and
> user-triggered SmmStartupThisAP's acquisition attempt of the Busy spinlock (non-blocking mode).
> 
> UserProc is the user's procedure to execute on an AP.
> UserProcCompletion is the user procedure's completion spinlock.
> All other variables are from EDK2.
> 
> BSP                                          AP
> =====================================================================================
> 											 APHandler ()
> 											   WaitForSemaphore (Run)
> 
> 								   << initial state >>
> 
> AcquireSpinLock (UserProcCompletion)
> SmmStartupThisAp (Procedure)
>   AcquireSpinLockOrFail (Busy)
>   ReleaseSemaphore (Run)
> 											   UserProc ()
> DoStuff()                                        DoSomeOtherStuff ()
> 
> AcquireSpinLockOrFail (UserProcCompletion) AcquireSpinLockOrFail (UserProcCompletion)
> 
> ^^ waiting in a loop for user procedure's
>    completion == these fail
> ReleaseSpinLock (UserProcCompletion)       AcquireSpinLockOrFail (UserProcCompletion)
> 
> ^^ this succeeds
> 
> ReleaseSpinLock (UserProcCompletion)
> 
> << return control to the caller and
>    reenter the flow >>>
> 
> AcquireSpinLock (UserProcCompletion)
> SmmStartupThisAp (Procedure)
>   AcquireSpinLockOrFail (Busy)
>   ^^ this wins the race with AP's
> 	 ReleaseSpinLock and fails;
> 											   ReleaseSpinLock (Busy)
> 	return EFI_INVALID_PARAMETER;
> 
> To remedy, if AcquireSpinLockOrFail (of the Busy spinlock) fails, perform regular AcquireSpinLock -- this eliminates the race
> condition.
> 
> Signed-off-by: Damian Nikodem <damian.nikodem@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Krzysztof Rusocki <krzysztof.rusocki@intel.com>
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index d8d2b6f444..206e196a76 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1239,8 +1239,16 @@ InternalSmmStartupThisAp (
>      AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
>    } else {
>      if (!AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy)) {
> -      DEBUG((DEBUG_ERROR, "Can't acquire mSmmMpSyncData->CpuData[%d].Busy\n", CpuIndex));
> -      return EFI_NOT_READY;
> +      DEBUG ((DEBUG_INFO, "BSP[%d] finds AP[%d] busy at proc 0x%llX (param 0x%llX), ",
> +        mSmmMpSyncData->BspIndex,
> +        CpuIndex,
> +        *mSmmMpSyncData->CpuData[CpuIndex].Procedure,
> +        (VOID*)mSmmMpSyncData->CpuData[CpuIndex].Parameter));
> +      DEBUG ((DEBUG_INFO, "new proc 0x%llX (param 0x%llX). Waiting for the previous AP procedure to complete...\n",
> +        Procedure,
> +        ProcArguments));
> +
> +      AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
>      }
> 
>      *Token = (MM_COMPLETION) CreateToken ();