On 09/03/19 16:57, Damian Nikodem wrote:
> 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;
Sorry, I can't make any sense of this sequence diagram. It seems to have
fallen apart due to formatting or other email issues. For example, if we
have "AP" and "BSP" columns, I would expect every function name to show
up in either. But APHandler() is to the right of *both* columns.
Please clean up the commit message:
- subject line should be no longer than 74 chars
- continuous text paragraphs should be properly filled, and wrapped at
74 chars
- the diagram can extend more widely, but it should be a diagram please.
(I'm not as familiar with this code as other UefiCpuPkg reviewers, so I
absolutely depend on the commit message to guide me.)
Thanks
Laszlo
>
> To remedy, if AcquireSpinLockOrFail (of the Busy spinlock) fails, perform
> regular AcquireSpinLock -- this eliminates the race condition.
>
> Signed-off-by: Damian Nikodem <damian.niko...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Benjamin You <benjamin....@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Krzysztof Rusocki <krzysztof.ruso...@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 ();
> --------------------------------------------------------------------
>
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII
> Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP
> 957-07-52-316 | Kapital zakladowy 200.000 PLN.
>
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i
> moze zawierac informacje poufne. W razie przypadkowego otrzymania tej
> wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie;
> jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the
> sole use of the intended recipient(s). If you are not the intended recipient,
> please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46721): https://edk2.groups.io/g/devel/message/46721
Mute This Topic: https://groups.io/mt/33128825/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
