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...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Cc: Gerd Hoffmann <kra...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
