Re: [edk2-devel] [PATCH v1 2/7] UefiCpuPkg/PiSmmCpuDxeSmm: Reduce times of BSP and AP Sync for SMM Exit

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

On 11/3/23 16:30, Wu, Jiaxin wrote:
> After review, there are unnecessary steps for BSP and AP sync for SMM
> exit. This patch is to reduce one round BSP and AP sync so as to improve
> SMI performance:
> BSP: WaitForAllAPs <-- AP: ReleaseBsp
> BSP: ReleaseAllAPs --> AP: WaitForBsp
> 
> Change-Id: Ic33f42f3daa7ff1847e524d0c3d9cd4fcdefa61b

(1) please drop this


> 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 | 44 +++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index e96c7f51d6..5a42a5dd12 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -665,11 +665,13 @@ BSPHandler (
>      //
>      *mSmmMpSyncData->AllCpusInSync = TRUE;
>      ApCount                        = LockdownSemaphore (mSmmMpSyncData->Counter) - 1;
>  
>      //
> -    // Wait for all APs to get ready for programming MTRRs
> +    // Wait for all APs:
> +    // 1. Make sure all Aps have set the Present.

(2.1) This comment ("set the Present") is confusing.

Either say "set the Present *flag*", or just "set Present".

(2.2) This comment update is unrelated to the performance optimization;
it just documents existent *and preserved* behavior.

It's very confusing to see this in the same patch. The comment documents
behavior that *precedes* the "Wait for something to happen" loop and the
"Invoke the scheduled procedure" action in APHandler(), but the
performance optimization is *after* that loop. (As the subject says, the
optimization is for the exit path.)

So please split the comment update to a separate patch.


> +    // 2. Get ready for programming MTRRs.
>      //
>      WaitForAllAPs (ApCount);
>  
>      if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
>        //
> @@ -768,16 +770,16 @@ BSPHandler (
>    // Notify all APs to exit
>    //
>    *mSmmMpSyncData->InsideSmm = FALSE;
>    ReleaseAllAPs ();
>  
> -  //
> -  // Wait for all APs to complete their pending tasks
> -  //
> -  WaitForAllAPs (ApCount);
> -
>    if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
> +    //
> +    // Wait for all APs to complete their pending tasks
> +    //
> +    WaitForAllAPs (ApCount);
> +
>      //
>      // Signal APs to restore MTRRs
>      //
>      ReleaseAllAPs ();
>  
> @@ -789,23 +791,23 @@ BSPHandler (
>  
>      //
>      // Wait for all APs to complete MTRR programming
>      //
>      WaitForAllAPs (ApCount);
> +
> +    //
> +    // Signal APs to Reset states/semaphore for this processor
> +    //
> +    ReleaseAllAPs ();
>    }
>  
>    //
>    // Stop source level debug in BSP handler, the code below will not be
>    // debugged.
>    //
>    InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL);
>  
> -  //
> -  // Signal APs to Reset states/semaphore for this processor
> -  //
> -  ReleaseAllAPs ();
> -
>    //
>    // Perform pending operations for hot-plug
>    //
>    SmmCpuUpdate ();
>  
> @@ -941,10 +943,12 @@ APHandler (
>    *(mSmmMpSyncData->CpuData[CpuIndex].Present) = TRUE;
>  
>    if ((SyncMode == SmmCpuSyncModeTradition) || SmmCpuFeaturesNeedConfigureMtrrs ()) {
>      //
>      // Notify BSP of arrival at this point
> +    // 1. Set the Present.

(3) Same as (2) (both sub-points).


> +    // 2. Get ready for programming MTRRs.
>      //
>      ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
>    }
>  
>    if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
> @@ -1033,21 +1037,21 @@ APHandler (
>      //
>      // Restore OS MTRRs
>      //
>      SmmCpuFeaturesReenableSmrr ();
>      MtrrSetAllMtrrs (&Mtrrs);
> -  }
>  
> -  //
> -  // Notify BSP the readiness of this AP to Reset states/semaphore for this processor
> -  //
> -  ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
> +    //
> +    // Notify BSP the readiness of this AP to Reset states/semaphore for this processor
> +    //
> +    ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
>  
> -  //
> -  // Wait for the signal from BSP to Reset states/semaphore for this processor
> -  //
> -  WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
> +    //
> +    // Wait for the signal from BSP to Reset states/semaphore for this processor
> +    //
> +    WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
> +  }
>  
>    //
>    // Reset states/semaphore for this processor
>    //
>    *(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE;

(4.1) If SmmCpuFeaturesNeedConfigureMtrrs() returns TRUE, then:

- the AP logic in unchanged

- the BSP logic changes, because InitializeDebugAgent() is invoked with
the APs released

(4.2) If SmmCpuFeaturesNeedConfigureMtrrs() returns FALSE, then:

- the AP logic omits an empty rendezvous with the BSP

- the BSP logic also omits the empty rendezvous with the APs, but it's
cheating: the rendezvous on the BSP side is originally *not* empty even
when there is no need to configure MTRRs -- the rendezvous protects the
InitializeDebugAgent() call.

(4.3) So, we need yet another precursor patch for this performance
optimization. Namely, you need to show that calling
InitializeDebugAgent() with DEBUG_AGENT_INIT_EXIT_SMI, with all the APs
released, is safe. Put differently, a precursor patch is needed that
does nothing other than *reordering* the ReleaseAllAPs() and
InitializeDebugAgent() calls in BSPHandler(). You need to justify and
document that change in a separate patch, and once that is in place, you
can employ this performance optimization.


(5) After the performance optimization, the comments on the final
semaphore operations inside the MTRR branch are misleading. Those
comments say

  Signal APs to Reset states/semaphore for this processor

on the BSP side, and they say

  Notify BSP the readiness of this AP to Reset states/semaphore for this
  processor

and

  Wait for the signal from BSP to Reset states/semaphore for this
  processor

on the AP side.

The problem is, this structuring and this wording imply that "state
resetting" is *conditional* on MTRR programming, which is of course not
true. State resetting must happen in all cases, it's only the
*additional rendezvous* that is necessary for, and conditional on, MTRR
configuration.

Put differently:

- Before the performance optimization, we have a rendezvous that
*unconditionally* happens (it is independent of MTRR configuration). The
good side of that is that we can directly tie this unconditional
rendezvous to the final "state resetting", and therefore the comments on
the rendezvous logic are correct, before the patch. However, the bad
side (before the patch) is that this approach wastes performance, when
MTRR config is unneeded.

- After the performance optimization, the rendezvous occurs only when it
is really required, for MTRR config. This is good. The bad side is that
the original comments no longer fit -- the rendezvous is now tied to
MTRR config, *not* the final state resetting!

All in all, the comments on the BSP side should be:

  if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
    //
    // Sync with the APs just before MTRR restoration
    //
    WaitForAllAPs (ApCount);
    ReleaseAllAPs ();

    //
    // Restore OS MTRRs
    //
    SmmCpuFeaturesReenableSmrr ();
    MtrrSetAllMtrrs (&Mtrrs);

    //
    // Sync with the APs just after MTRR restoration
    //
    WaitForAllAPs (ApCount);
    ReleaseAllAPs ();
  }

And on the AP side:

  if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
    //
    // Sync with the BSP just before MTRR restoration
    //
    ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
    WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);

    //
    // Restore OS MTRRs
    //
    SmmCpuFeaturesReenableSmrr ();
    MtrrSetAllMtrrs (&Mtrrs);

    //
    // Sync with the BSP just after MTRR restoration
    //
    ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
    WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
  }


* In summary:

- First patch: update the comment in the top part of the BSP and AP
handlers.

- Second patch: invert the order of ReleaseAllAPs() and
InitializeDebugAgent(DEBUG_AGENT_INIT_EXIT_SMI) in BSPHandler().
Furthermore, explain in the commit message that this reordering is valid.

- Third patch: the perf optimization. Restrict the rendezvous to when
MTRR configuration is needed. While at it, (a) explain the nature of the
opimization in the commit message (i.e., that without MTRR config, we
have a superfluous rendezvous), because the current commit message is
useless, (b) clean up the comments on the before and after
synchronizations (see proposal above).

Laszlo



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