[PATCH v3] UefiCpuPkg: Fix potential spinLock issue in SmmStartupThisAp

[PATCH v3] UefiCpuPkg: Fix potential spinLock issue in SmmStartupThisAp

[Damian Nikodem]

Due to needs a tackling the deficiency of the AP API, is necessary to ensure that in non-blocking mode
previous AP executed command is finished before start new one. 

To remedy above: 
  1) execute AcquireSpinLock instead AcquireSpinLockOrFail - this will ensure time "window" to eliminate potential
     race condition beetwen BSP and AP spinLock release in non-blocking mode.
     This also will eliminate possibility to start executing new AP function before last is finished.
  2) remove returns EFI_STATUS - EFI_NOT_READY - in new scenario returned status is not necessary to caller.

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..0685637c2b 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1235,14 +1235,9 @@ InternalSmmStartupThisAp (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (Token == NULL) {
-    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;
-    }
+  AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
 
+  if (Token != NULL) {
     *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.

Re: [PATCH v3] UefiCpuPkg: Fix potential spinLock issue in SmmStartupThisAp

[Laszlo Ersek]

On 09/11/19 10:23, Damian Nikodem wrote:
> Due to needs a tackling the deficiency of the AP API, is necessary to ensure that in non-blocking mode
> previous AP executed command is finished before start new one. 
> 
> To remedy above: 
>   1) execute AcquireSpinLock instead AcquireSpinLockOrFail - this will ensure time "window" to eliminate potential
>      race condition beetwen BSP and AP spinLock release in non-blocking mode.
>      This also will eliminate possibility to start executing new AP function before last is finished.
>   2) remove returns EFI_STATUS - EFI_NOT_READY - in new scenario returned status is not necessary to caller.
> 
> 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>

Eric, Ray, please don't wait for my feedback on this one. It would be
prudent for me to test this, but I got no time.

Thanks
Laszlo

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index d8d2b6f444..0685637c2b 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1235,14 +1235,9 @@ InternalSmmStartupThisAp (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -  if (Token == NULL) {
> -    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;
> -    }
> +  AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
>  
> +  if (Token != NULL) {
>      *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.
> 

Re: [PATCH v3] UefiCpuPkg: Fix potential spinLock issue in SmmStartupThisAp

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Nikodem, Damian
> Sent: Wednesday, September 11, 2019 1:23 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 v3] UefiCpuPkg: Fix potential spinLock issue in SmmStartupThisAp
> 
> Due to needs a tackling the deficiency of the AP API, is necessary to ensure that in non-blocking mode
> previous AP executed command is finished before start new one.
> 
> To remedy above:
>   1) execute AcquireSpinLock instead AcquireSpinLockOrFail - this will ensure time "window" to eliminate potential
>      race condition beetwen BSP and AP spinLock release in non-blocking mode.
>      This also will eliminate possibility to start executing new AP function before last is finished.
>   2) remove returns EFI_STATUS - EFI_NOT_READY - in new scenario returned status is not necessary to caller.
> 
> 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..0685637c2b 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1235,14 +1235,9 @@ InternalSmmStartupThisAp (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> -  if (Token == NULL) {
> -    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;
> -    }
> +  AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
> 
> +  if (Token != NULL) {
>      *Token = (MM_COMPLETION) CreateToken ();
>    }