[PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs.

[PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs.

[Dong, Eric]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2268

In current implementation, when check whether APs called by StartUpAllAPs
or StartUpThisAp, it checks the Tokens value used by other APs. Also the AP
will update the Token value for itself if its task finished. In this
case, the potential race condition  issues happens for the tokens.
Because of this, system may trig ASSERT during cycling test.

This change enhance the code logic, add new attributes for the token to
remove the reference for the tokens belongs to other APs.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 107 +++++++--------------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   4 +-
 2 files changed, 36 insertions(+), 75 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 757f1056f7..bd5fdfd728 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -402,38 +402,6 @@ IsPresentAp (
     *(mSmmMpSyncData->CpuData[CpuIndex].Present));
 }
 
-/**
-  Check whether execute in single AP or all APs.
-
-  Compare two Tokens used by different APs to know whether in StartAllAps call.
-
-  Whether is an valid AP base on AP's Present flag.
-
-  @retval  TRUE      IN StartAllAps call.
-  @retval  FALSE     Not in StartAllAps call.
-
-**/
-BOOLEAN
-InStartAllApsCall (
-  VOID
-  )
-{
-  UINTN      ApIndex;
-  UINTN      ApIndex2;
-
-  for (ApIndex = mMaxNumberOfCpus; ApIndex-- > 0;) {
-    if (IsPresentAp (ApIndex) && (mSmmMpSyncData->CpuData[ApIndex].Token != NULL)) {
-      for (ApIndex2 = ApIndex; ApIndex2-- > 0;) {
-        if (IsPresentAp (ApIndex2) && (mSmmMpSyncData->CpuData[ApIndex2].Token != NULL)) {
-          return mSmmMpSyncData->CpuData[ApIndex2].Token == mSmmMpSyncData->CpuData[ApIndex].Token;
-        }
-      }
-    }
-  }
-
-  return FALSE;
-}
-
 /**
   Clean up the status flags used during executing the procedure.
 
@@ -445,40 +413,20 @@ ReleaseToken (
   IN UINTN                  CpuIndex
   )
 {
-  UINTN                             Index;
-  BOOLEAN                           Released;
+  PROCEDURE_TOKEN                         *Token;
 
-  if (InStartAllApsCall ()) {
-    //
-    // In Start All APs mode, make sure all APs have finished task.
-    //
-    if (WaitForAllAPsNotBusy (FALSE)) {
-      //
-      // Clean the flags update in the function call.
-      //
-      Released = FALSE;
-      for (Index = mMaxNumberOfCpus; Index-- > 0;) {
-        //
-        // Only In SMM APs need to be clean up.
-        //
-        if (mSmmMpSyncData->CpuData[Index].Present && mSmmMpSyncData->CpuData[Index].Token != NULL) {
-          if (!Released) {
-            ReleaseSpinLock (mSmmMpSyncData->CpuData[Index].Token);
-            Released = TRUE;
-          }
-          mSmmMpSyncData->CpuData[Index].Token = NULL;
-        }
-      }
-    }
-  } else {
-    //
-    // In single AP mode.
-    //
-    if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) {
-      ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Token);
-      mSmmMpSyncData->CpuData[CpuIndex].Token = NULL;
+  Token = mSmmMpSyncData->CpuData[CpuIndex].Token;
+  mSmmMpSyncData->CpuData[CpuIndex].Token = NULL;
+
+  if (!Token->SingleAp) {
+    ReleaseSemaphore (&Token->FinishedApCount);
+
+    if (Token->FinishedApCount < (UINT32)mMaxNumberOfCpus) {
+      return;
     }
   }
+
+  ReleaseSpinLock (Token->ProcedureToken);
 }
 
 /**
@@ -912,12 +860,14 @@ APHandler (
       *mSmmMpSyncData->CpuData[CpuIndex].Status = ProcedureStatus;
     }
 
+    if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) {
+      ReleaseToken (CpuIndex);
+    }
+
     //
     // Release BUSY
     //
     ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
-
-    ReleaseToken (CpuIndex);
   }
 
   if (SmmCpuFeaturesNeedConfigureMtrrs()) {
@@ -1127,7 +1077,7 @@ IsTokenInUse (
   @retval    return the spin lock used as token.
 
 **/
-SPIN_LOCK *
+PROCEDURE_TOKEN *
 CreateToken (
   VOID
   )
@@ -1170,10 +1120,12 @@ CreateToken (
   ASSERT (ProcToken != NULL);
   ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE;
   ProcToken->ProcedureToken = CpuToken;
+  ProcToken->FinishedApCount = 0;
+  ProcToken->SingleAp = TRUE;
 
   InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link);
 
-  return CpuToken;
+  return ProcToken;
 }
 
 /**
@@ -1278,14 +1230,11 @@ InternalSmmStartupThisAp (
 
   AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
 
-  if (Token != NULL) {
-    *Token = (MM_COMPLETION) CreateToken ();
-  }
-
   mSmmMpSyncData->CpuData[CpuIndex].Procedure = Procedure;
   mSmmMpSyncData->CpuData[CpuIndex].Parameter = ProcArguments;
   if (Token != NULL) {
-    mSmmMpSyncData->CpuData[CpuIndex].Token   = (SPIN_LOCK *)(*Token);
+    mSmmMpSyncData->CpuData[CpuIndex].Token = CreateToken ();
+    *Token = (MM_COMPLETION) mSmmMpSyncData->CpuData[CpuIndex].Token->ProcedureToken;
   }
   mSmmMpSyncData->CpuData[CpuIndex].Status    = CpuStatus;
   if (mSmmMpSyncData->CpuData[CpuIndex].Status != NULL) {
@@ -1343,6 +1292,7 @@ InternalSmmStartupAllAPs (
 {
   UINTN               Index;
   UINTN               CpuCount;
+  PROCEDURE_TOKEN     *ProcToken;
 
   if ((TimeoutInMicroseconds != 0) && ((mSmmMp.Attributes & EFI_MM_MP_TIMEOUT_SUPPORTED) == 0)) {
     return EFI_INVALID_PARAMETER;
@@ -1371,7 +1321,11 @@ InternalSmmStartupAllAPs (
   }
 
   if (Token != NULL) {
-    *Token = (MM_COMPLETION) CreateToken ();
+    ProcToken = CreateToken ();
+    *Token = (MM_COMPLETION)ProcToken->ProcedureToken;
+    ProcToken->SingleAp = FALSE;
+  } else {
+    ProcToken = NULL;
   }
 
   //
@@ -1392,7 +1346,7 @@ InternalSmmStartupAllAPs (
       mSmmMpSyncData->CpuData[Index].Procedure = (EFI_AP_PROCEDURE2) Procedure;
       mSmmMpSyncData->CpuData[Index].Parameter = ProcedureArguments;
       if (Token != NULL) {
-        mSmmMpSyncData->CpuData[Index].Token   = (SPIN_LOCK *)(*Token);
+        mSmmMpSyncData->CpuData[Index].Token   = ProcToken;
       }
       if (CPUStatus != NULL) {
         mSmmMpSyncData->CpuData[Index].Status    = &CPUStatus[Index];
@@ -1408,6 +1362,11 @@ InternalSmmStartupAllAPs (
       if (CPUStatus != NULL) {
         CPUStatus[Index] = EFI_NOT_STARTED;
       }
+
+      //
+      // Increate the count to mark this AP as finished.
+      //
+      ReleaseSemaphore (&ProcToken->FinishedApCount);
     }
   }
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 5c1a01e42b..0551539aee 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -213,6 +213,8 @@ typedef struct {
   LIST_ENTRY              Link;
 
   SPIN_LOCK               *ProcedureToken;
+  BOOLEAN                 SingleAp;
+  volatile UINT32         FinishedApCount;
 } PROCEDURE_TOKEN;
 
 #define PROCEDURE_TOKEN_FROM_LINK(a)  CR (a, PROCEDURE_TOKEN, Link, PROCEDURE_TOKEN_SIGNATURE)
@@ -407,7 +409,7 @@ typedef struct {
   volatile VOID                     *Parameter;
   volatile UINT32                   *Run;
   volatile BOOLEAN                  *Present;
-  SPIN_LOCK                         *Token;
+  PROCEDURE_TOKEN                   *Token;
   EFI_STATUS                        *Status;
 } SMM_CPU_DATA_BLOCK;
 
-- 
2.23.0.windows.1

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs.

[Ni, Ray]

> 
> +  if (!Token->SingleAp) {
> 
> +    ReleaseSemaphore (&Token->FinishedApCount);

1. If the FinishedApCount is renamed to RunningApCount and
InterlockedDecrement() is called for it.

SingleAp flag is unneeded.

For StartupAllAps(), RunningApCount = mMaxNumberOfCpus - 1;
For StartupThisAps(), RunningApCount = 1;

When RunningApCount == 0, the spinlock is released.

> +    if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) {
> 
> +      ReleaseToken (CpuIndex);

2. Can you directly pass in mSmmMpSyncData->CpuData[CpuIndex].Token?
It simplifies the ReleaseToken() and also make people understand that
ReleaseToken() will only modifies the Token but other states in CpuData[Index]
is NOT changed.

> 
> @@ -1170,10 +1120,12 @@ CreateToken (

3. With the comment #1, CreateToken() can carry additional parameter which specifies
the RunningApCount.

>    ASSERT (ProcToken != NULL);
> 
>    ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE;
> 
>    ProcToken->ProcedureToken = CpuToken;

4. ProcToken->ProcedureToken looks a bit strange.
Can we use "ProcToken->Spinlock"?

> 
> +    *Token = (MM_COMPLETION) mSmmMpSyncData-
> >CpuData[CpuIndex].Token->ProcedureToken;

5. It will become
*Token = (MM_COMPLETION) mSmmMpSyncData->CpuData[CpuIndex].Token->Spinlock;

> 
> +      ReleaseSemaphore (&ProcToken->FinishedApCount);

6. I can now understand why "FinishedApCount is directly compared against mMaxNumberOfCpus because
the FinishedApCount is already increased for BSP. It's not a comment for code change.


Thanks,
Ray

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs.

[Dong, Eric]

[-- Attachment #1: Type: text/plain, Size: 2321 bytes --]

Hi Ray,

> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, December 20, 2019 2:15 PM
> To: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Subject: RE: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence
> between APs.
>
> >
> > +  if (!Token->SingleAp) {
> >
> > +    ReleaseSemaphore (&Token->FinishedApCount);
>
> 1. If the FinishedApCount is renamed to RunningApCount and
> InterlockedDecrement() is called for it.
>
> SingleAp flag is unneeded.
>
> For StartupAllAps(), RunningApCount = mMaxNumberOfCpus - 1; For
> StartupThisAps(), RunningApCount = 1;
>
> When RunningApCount == 0, the spinlock is released.
>
[[Eric]] good idea, will update the logic.


> > +    if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) {
> >
> > +      ReleaseToken (CpuIndex);
>
> 2. Can you directly pass in mSmmMpSyncData->CpuData[CpuIndex].Token?
> It simplifies the ReleaseToken() and also make people understand that
> ReleaseToken() will only modifies the Token but other states in
> CpuData[Index] is NOT changed.
>
[[Eric]] ReleaseToken also set mSmmMpSyncData->CpuData[CpuIndex].Token to NULL
at the end. So can't directly input Token.

> >
> > @@ -1170,10 +1120,12 @@ CreateToken (
>
> 3. With the comment #1, CreateToken() can carry additional parameter which
> specifies the RunningApCount.
>
[[Eric]] yes, will update the logic.

> >    ASSERT (ProcToken != NULL);
> >
> >    ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE;
> >
> >    ProcToken->ProcedureToken = CpuToken;
>
> 4. ProcToken->ProcedureToken looks a bit strange.
> Can we use "ProcToken->Spinlock"?
[[Eric]] yes, will update the name.

Thanks,
Eric
>
> >
> > +    *Token = (MM_COMPLETION) mSmmMpSyncData-
> > >CpuData[CpuIndex].Token->ProcedureToken;
>
> 5. It will become
> *Token = (MM_COMPLETION) mSmmMpSyncData-
> >CpuData[CpuIndex].Token->Spinlock;
>
> >
> > +      ReleaseSemaphore (&ProcToken->FinishedApCount);
>
> 6. I can now understand why "FinishedApCount is directly compared against
> mMaxNumberOfCpus because the FinishedApCount is already increased for
> BSP. It's not a comment for code change.
>
>
> Thanks,
> Ray

[-- Attachment #2: Type: text/html, Size: 8734 bytes --]