Re: [edk2-devel] [PATCH v1 1/7] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP

["Laszlo Ersek" <lersek@redhat.com>]

On 11/3/23 16:30, Wu, Jiaxin wrote:
> This patch is to define 3 new functions (WaitForBsp & ReleaseBsp &
> ReleaseOneAp) used for the semaphore sync between BSP & AP. With the
> change, BSP and AP Sync flow will be easy understand as below:
> BSP: ReleaseAllAPs or ReleaseOneAp --> AP: WaitForBsp
> BSP: WaitForAllAPs                 <-- AP: ReleaseBsp
> 
> Change-Id: I0fb25e26e1015e918800f4d8d62e5276dcd5b5b1

(1) please remove this; not useful upstream

> 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 | 72 ++++++++++++++++++++++++++++-------
>  1 file changed, 58 insertions(+), 14 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 25d058c5b9..e96c7f51d6 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -120,10 +120,11 @@ LockdownSemaphore (
>  
>    return Value;
>  }
>  
>  /**
> +  Used for BSP to wait all APs.
>    Wait all APs to performs an atomic compare exchange operation to release semaphore.
>  
>    @param   NumberOfAPs      AP number
>  
>  **/
> @@ -139,10 +140,11 @@ WaitForAllAPs (
>      WaitForSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
>    }
>  }
>  
>  /**
> +  Used for BSP to release all APs.
>    Performs an atomic compare exchange operation to release semaphore
>    for each AP.
>  
>  **/
>  VOID
> @@ -157,10 +159,52 @@ ReleaseAllAPs (
>        ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run);
>      }
>    }
>  }
>  
> +/**
> +  Used for BSP to release one AP.
> +
> +  @param      ApSem     IN:  32-bit unsigned integer
> +                        OUT: original integer + 1
> +**/
> +VOID
> +ReleaseOneAp   (
> +  IN OUT  volatile UINT32  *ApSem
> +  )
> +{
> +  ReleaseSemaphore (ApSem);
> +}
> +
> +/**
> +  Used for AP to wait BSP.
> +
> +  @param      ApSem      IN:  32-bit unsigned integer
> +                         OUT: original integer 0

(2) wrong comment; WaitForSemaphore() says "OUT: original integer - 1".

> +**/
> +VOID
> +WaitForBsp  (
> +  IN OUT  volatile UINT32  *ApSem
> +  )
> +{
> +  WaitForSemaphore (ApSem);
> +}
> +
> +/**
> +  Used for AP to release BSP.
> +
> +  @param      BspSem     IN:  32-bit unsigned integer
> +                         OUT: original integer + 1
> +**/
> +VOID
> +ReleaseBsp   (
> +  IN OUT  volatile UINT32  *BspSem
> +  )
> +{
> +  ReleaseSemaphore (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]
> @@ -632,11 +676,11 @@ BSPHandler (
>        // Signal all APs it's time for backup MTRRs
>        //
>        ReleaseAllAPs ();
>  
>        //
> -      // WaitForSemaphore() may wait for ever if an AP happens to enter SMM at
> +      // WaitForBsp() may wait for ever if an AP happens to enter SMM at
>        // exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has been set
>        // to a large enough value to avoid this situation.
>        // Note: For HT capable CPUs, threads within a core share the same set of MTRRs.
>        // We do the backup first and then set MTRR to avoid race condition for threads
>        // in the same core.
> @@ -652,11 +696,11 @@ BSPHandler (
>        // Let all processors program SMM MTRRs together
>        //
>        ReleaseAllAPs ();
>  
>        //
> -      // WaitForSemaphore() may wait for ever if an AP happens to enter SMM at
> +      // WaitForBsp() may wait for ever if an AP happens to enter SMM at
>        // exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has been set
>        // to a large enough value to avoid this situation.
>        //
>        ReplaceOSMtrrs (CpuIndex);
>  

(3) I believe (but am not fully sure) that the above comment updates are
wrong. Both contexts belong to BSPHandler(), where the BSP orchestrates
MTRR programming on all processors (which must occur on all processors
at the same time). The comments explain that the BSP releases the APs to
do their jobs, and then waits for them to finish. We have two
WaitForAllAPs() calls. IOW, the BSP does not wait for the BSP, but the
APs. Thus, the updated comments should say WaitForAllAPs(), not
WaitForBsp().

> @@ -898,50 +942,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 +1021,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 +1038,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.
>  
> @@ -1277,11 +1321,11 @@ InternalSmmStartupThisAp (
>    mSmmMpSyncData->CpuData[CpuIndex].Status = CpuStatus;
>    if (mSmmMpSyncData->CpuData[CpuIndex].Status != NULL) {
>      *mSmmMpSyncData->CpuData[CpuIndex].Status = EFI_NOT_READY;
>    }
>  
> -  ReleaseSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
> +  ReleaseOneAp (mSmmMpSyncData->CpuData[CpuIndex].Run);
>  
>    if (Token == NULL) {
>      AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
>      ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
>    }

Looks OK to me otherwise.

Laszlo



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