Re: [edk2-devel] [Patch V2] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present.

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

On 11/13/23 06:47, Yuanhao Xie wrote:
> SMM read save state requires AP must be present.
> This patch aim to avoid the AP not ready for save state check.
> 
> Signed-off-by: Zhihao Li <zhihao.li@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c     | 25 +++++++++++++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 +++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> index 391b64e9f2..cdc7021ee9 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> @@ -406,8 +406,15 @@ SmmCpuRendezvous (
>    IN BOOLEAN                            BlockingMode
>    )
>  {
> +  UINTN       Index;
> +  UINTN       PresentCount;
> +  UINT32      BlockedCount;
> +  UINT32      DisabledCount;
>    EFI_STATUS  Status;
>  
> +  BlockedCount  = 0;
> +  DisabledCount = 0;
> +
>    //
>    // Return success immediately if all CPUs are already synchronized.
>    //
> @@ -426,6 +433,24 @@ SmmCpuRendezvous (
>      // There are some APs outside SMM, Wait for all avaiable APs to arrive.
>      //
>      SmmWaitForApArrival ();
> +
> +    //
> +    // Make sure all APs have their Present flag set
> +    //
> +    while (TRUE) {
> +      PresentCount = 0;
> +      for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> +        if (*(mSmmMpSyncData->CpuData[Index].Present)) {
> +          PresentCount++;
> +        }
> +      }
> +
> +      GetSmmDelayedBlockedDisabledCount (NULL, &BlockedCount, &DisabledCount);
> +      if (PresentCount == mMaxNumberOfCpus - BlockedCount - DisabledCount ) {
> +        break;
> +      }
> +    }
> +
>      Status = mSmmMpSyncData->AllApArrivedWithException ? EFI_SUCCESS : EFI_TIMEOUT;
>    } else {
>      //

(1) Here's why I don't like this:

we already have a function that is supposed to do this, and it is
SmmWaitForApArrival().

SmmWaitForApArrival() is called in two contexts. One, in BSPHandler().
Two, here.

Consider the following condition:

  (SyncMode == SmmCpuSyncModeTradition) ||
  SmmCpuFeaturesNeedConfigureMtrrs ()

If this condition evaluates to true, then BSPHandler() calls
SmmWaitForApArrival(), and SmmCpuRendezvous() doesn't.

(This is what the "else" branch in SmmCpuRendezvous() states, in a
comment, too.)

And if the condition evaluates to false, then SmmCpuRendezvous() calls
SmmWaitForApArrival(), and BSPHandler() doesn't.

This patch adds extra waiting logic to *one* of both call sites. And I
don't understand why the *other* call site (in BSPHandler()) does not
need the exact same logic.

In my opinion, this is a sign that SmmWaitForApArrival() isn't "strong
enough". It is not doing all of the work.

In my opinion, *both* call sites should receive this logic (i.e., ensure
that all AP's are "present"), but then in turn, the additional waiting /
checking should be pushed down into SmmWaitForApArrival().

What's your opinion on that?

(2) I notice that a similar "busy loop", waiting for Present flags, is
in BSPHandler().

Do you think we could call CpuPause() in all such "busy loops" (just
before the end of the "while" body)? I think that's supposed to improve
the system's throughput, considered as a whole. The function's comment says,

  Requests CPU to pause for a short period of time. Typically used in MP
  systems to prevent memory starvation while waiting for a spin lock.


Thanks
Laszlo





> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 20ada465c2..b5aa5f99d7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1552,6 +1552,19 @@ SmmWaitForApArrival (
>    VOID
>    );
>  
> +/**
> +  Returns the Number of SMM Delayed & Blocked & Disabled Thread Count.
> +  @param[in,out] DelayedCount  The Number of SMM Delayed Thread Count.
> +  @param[in,out] BlockedCount  The Number of SMM Blocked Thread Count.
> +  @param[in,out] DisabledCount The Number of SMM Disabled Thread Count.
> +**/
> +VOID
> +GetSmmDelayedBlockedDisabledCount (
> +  IN OUT UINT32  *DelayedCount,
> +  IN OUT UINT32  *BlockedCount,
> +  IN OUT UINT32  *DisabledCount
> +  );
> +
>  /**
>    Write unprotect read-only pages if Cr0.Bits.WP is 1.
>  



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