Re: [edk2-devel] [PATCH v1] UefiCpuPkg/SmmCpu: Refine semaphore sync between BSP and AP

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

+Mike for this review. 



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin
> Sent: Wednesday, August 9, 2023 5:04 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>
> Subject: [edk2-devel] [PATCH v1] UefiCpuPkg/SmmCpu: Refine semaphore
> sync between BSP and AP
> 
> For SMM CPU semaphore sync, there is no need atomic semaphore
> operation, just use the flag to indicate it has complete the
> wait/release. Based on this, this patch is to refine 2 functions
> (WaitForAllAPs & ReleaseAllAPs) and define 2 new functions
> (WaitForBsp & ReleaseBsp) used for the semaphore sync between
> BSP & AP.
> 
> Sync flow like below:
> 1. BSP to Release All APs ---> 1. AP to Wait BSP
>    ReleaseAllAPs ()               WaitForBsp
> 2. BSP to Wait All APs    <--- 2. AP to Release BSP
>    WaitForAllAPs ()               ReleaseBsp
> 
> With this change, SMM CPU semaphore sync for SMI exit performance
> will be significant improved.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 68
> ++++++++++++++++++++++++++---------
>  1 file changed, 51 insertions(+), 17 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 25d058c5b9..0bf460e81c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -120,11 +120,11 @@ LockdownSemaphore (
> 
>    return Value;
>  }
> 
>  /**
> -  Wait all APs to performs an atomic compare exchange operation to release
> semaphore.
> +  Used for BSP to wait all APs.
> 
>    @param   NumberOfAPs      AP number
> 
>  **/
>  VOID
> @@ -133,18 +133,19 @@ WaitForAllAPs (
>    )
>  {
>    UINTN  BspIndex;
> 
>    BspIndex = mSmmMpSyncData->BspIndex;
> -  while (NumberOfAPs-- > 0) {
> -    WaitForSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
> +  while (NumberOfAPs != *mSmmMpSyncData->CpuData[BspIndex].Run) {
> +    CpuPause ();
>    }
> +
> +  *mSmmMpSyncData->CpuData[BspIndex].Run = 0;
>  }
> 
>  /**
> -  Performs an atomic compare exchange operation to release semaphore
> -  for each AP.
> +  Used for BSP to release all APs.
> 
>  **/
>  VOID
>  ReleaseAllAPs (
>    VOID
> @@ -152,15 +153,48 @@ ReleaseAllAPs (
>  {
>    UINTN  Index;
> 
>    for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
>      if (IsPresentAp (Index)) {
> -      ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run);
> +      ASSERT (*mSmmMpSyncData->CpuData[Index].Run == 0);
> +      *mSmmMpSyncData->CpuData[Index].Run  = 1;
>      }
>    }
>  }
> 
> +/**
> +  Used for Ap to wait BSP.
> +
> +  @param      ApSem      IN:  32-bit unsigned integer
> +                         OUT: original integer 0
> +**/
> +VOID
> +WaitForBsp  (
> +  IN OUT  volatile UINT32 *ApSem
> +  )
> +{
> +  while (*ApSem == 0) {
> +    CpuPause ();
> +  }
> +
> +  *ApSem = 0;
> +}
> +
> +/**
> +  Used for Ap to release BSP.
> +
> +  @param      BspSem     IN:  32-bit unsigned integer
> +                         OUT: original integer + 1
> +**/
> +VOID
> +ReleaseBsp   (
> +  IN OUT  volatile UINT32  *BspSem
> +  )
> +{
> +  InterlockedIncrement (BspSem);
> +}
> +
>  /**
>    Check whether the index of CPU perform the package level register
>    programming during System Management Mode initialization.
> 
>    The index of Processor specified by
> mPackageFirstThreadIndex[PackageIndex]
> @@ -898,50 +932,50 @@ APHandler (
> 
>    if ((SyncMode == SmmCpuSyncModeTradition) ||
> SmmCpuFeaturesNeedConfigureMtrrs ()) {
>      //
>      // Notify BSP of arrival at this point
>      //
> -    ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
> +    ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
>    }
> 
>    if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
>      //
>      // Wait for the signal from BSP to backup MTRRs
>      //
> -    WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
> +    WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
> 
>      //
>      // Backup OS MTRRs
>      //
>      MtrrGetAllMtrrs (&Mtrrs);
> 
>      //
>      // Signal BSP the completion of this AP
>      //
> -    ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
> +    ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
> 
>      //
>      // Wait for BSP's signal to program MTRRs
>      //
> -    WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
> +    WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
> 
>      //
>      // Replace OS MTRRs with SMI MTRRs
>      //
>      ReplaceOSMtrrs (CpuIndex);
> 
>      //
>      // Signal BSP the completion of this AP
>      //
> -    ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
> +    ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
>    }
> 
>    while (TRUE) {
>      //
>      // Wait for something to happen
>      //
> -    WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
> +    WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
> 
>      //
>      // Check if BSP wants to exit SMM
>      //
>      if (!(*mSmmMpSyncData->InsideSmm)) {
> @@ -977,16 +1011,16 @@ APHandler (
> 
>    if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
>      //
>      // Notify BSP the readiness of this AP to program MTRRs
>      //
> -    ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
> +    ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
> 
>      //
>      // Wait for the signal from BSP to program MTRRs
>      //
> -    WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
> +    WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
> 
>      //
>      // Restore OS MTRRs
>      //
>      SmmCpuFeaturesReenableSmrr ();
> @@ -994,26 +1028,26 @@ APHandler (
>    }
> 
>    //
>    // Notify BSP the readiness of this AP to Reset states/semaphore for this
> processor
>    //
> -  ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
> +  ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
> 
>    //
>    // Wait for the signal from BSP to Reset states/semaphore for this processor
>    //
> -  WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
> +  WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
> 
>    //
>    // Reset states/semaphore for this processor
>    //
>    *(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE;
> 
>    //
>    // Notify BSP the readiness of this AP to exit SMM
>    //
> -  ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
> +  ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
>  }
> 
>  /**
>    Checks whether the input token is the current used token.
> 
> --
> 2.16.2.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107773): https://edk2.groups.io/g/devel/message/107773
Mute This Topic: https://groups.io/mt/100639360/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-