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

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

[Yuanhao Xie]

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 {
     //
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.
 
-- 
2.39.1.windows.1



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

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Wu, Jiaxin]

> (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?


Existing SmmWaitForApArrival() only make sure all Aps enter SMI except SMI blocked & disabled Aps, not consider the "Present" state. I think this is the original implementation purpose. It won't require all Aps must set the Present flag.

As you also commented there is a later loop for the Present flag:
	    WaitForAllAPs (ApCount)

Here, i still prefer to keep existing way instead of making SmmWaitForApArrival return until all aps set the Present flag, because that will be duplicate work within SmmWaitForApArrival() & existing  WaitForAllAPs (). We can't delete the WaitForAllAPs for the later sync to make sure all APs to get ready for programming MTRRs. MTRRs programming need all CPUs in the same start line. 

  WaitForAllAPs() has two purpose:
   1. Make sure all Aps have set the Present.
   2. Get ready for programming MTRRs to make sure cpus in the same start line.

if so, that will be better as existing logic, it can also save some time for the Present flag check in SmmWaitForApArrival

> 
> (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.
> 

Do you mean the below WaitForAllAPs()? There is already has the CpuPause check within WaitForSemaphore().

    //
    // Wait for all APs to get ready for programming MTRRs
    //
    WaitForAllAPs (ApCount);


Thanks,
Jiaxin 



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

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

[Laszlo Ersek]

On 12/6/23 04:35, Wu, Jiaxin wrote:
>> (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?
> 
> 
> Existing SmmWaitForApArrival() only make sure all Aps enter SMI except SMI blocked & disabled Aps, not consider the "Present" state. I think this is the original implementation purpose. It won't require all Aps must set the Present flag.
> 
> As you also commented there is a later loop for the Present flag:
> 	    WaitForAllAPs (ApCount)
> 
> Here, i still prefer to keep existing way instead of making SmmWaitForApArrival return until all aps set the Present flag, because that will be duplicate work within SmmWaitForApArrival() & existing  WaitForAllAPs (). We can't delete the WaitForAllAPs for the later sync to make sure all APs to get ready for programming MTRRs. MTRRs programming need all CPUs in the same start line. 
> 
>   WaitForAllAPs() has two purpose:
>    1. Make sure all Aps have set the Present.
>    2. Get ready for programming MTRRs to make sure cpus in the same start line.
> 
> if so, that will be better as existing logic, it can also save some time for the Present flag check in SmmWaitForApArrival

OK, this argument makes sense to me.

I didn't realize that WaitForAllAPs() -- called by BSPHandler() after
calling SmmWaitForApArrival() -- already *effectively* ensured that the
APs had their Present flag set!

That happens because:

(a) WaitForAllAPs() pends the "Run" semaphore of each AP.

(b) The APHandler() function sets the Present flag *first*.

(c) If

  (SyncMode == SmmCpuSyncModeTradition) ||
  SmmCpuFeaturesNeedConfigureMtrrs ()

is true, then APHandler() posts the "Run" semaphore, *second*.

Therefore, once WaitForAllAPs() has acquired all AP "Run" semaphores,
all AP Present flags must be set.

This is not obvious at all, but it looks correct.

Therefore I agree that your patch does not introduce asymmetry between
SmmCpuRendezvous() and BSPHandler(). Instead, your patch eliminates
asymmetry, because now SmmCpuRendezvous() will wait for the Present
flags of the APs (if the above-quoted condition is false), similarly to
how BSPHandler already does (if the condition is true).


Now, I have not had the time yet to re-review your patch set

  [PATCH v3 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib

As far as I remember (from v1), that patch set reorganizes exactly these
"Run" semaphore release/acquire patterns.

(3) Can you confirm that your v3 patch set will not invalidate this
discussion? I.e., can you confirm that WaitForAllAPs() will *still*
ensure, via the Run semaphores, that the Present flags will have been set?

(4) Please add the following to the commit message:

-------
BSPHandler -> { SmmWaitForApArrival, WaitForAllAPs } already awaits that
the Present flag is set for all APs, namely via the AP Run semaphores.
Therefore this patch ensures symmetry between BSPHandler (when [1] is
true) and SmmCpuRendezvous() (when [1] is false).

[1] (SyncMode == SmmCpuSyncModeTradition) ||
    SmmCpuFeaturesNeedConfigureMtrrs ()
-------

More comments below:

> 
>>
>> (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.
>>
> 
> Do you mean the below WaitForAllAPs()? There is already has the CpuPause check within WaitForSemaphore().
> 
>     //
>     // Wait for all APs to get ready for programming MTRRs
>     //
>     WaitForAllAPs (ApCount);

Yes, that's the pattern we should follow here.

The call chain

  BSPHandler -> WaitForAllAPs -> WaitForSemaphore -> CpuPause

already calls CpuPause() when condition [1] is true. That's the pattern
we should follow.

When condition [1] is false, your patch implements the wait for the
Present flags in SmmCpuRendezvous(), but there we have no CpuPause().
So, we could / should add it, right at the end of the while (TRUE) loop.

Summary of my requests:

- the patch seems good, but please confirm that your v3 sync rework will
leave the predicate intact that WaitForAllAPs() ensures the Present
flags via the Run semaphores

- please extend the commit message with the paragraph about symmetry
between BSPHandler and SmmCpuRendezvous

- please consider adding CpuPause to the end of the "while (TRUE)" loop.

(BTW, the patch is not really (or usefully) testable with OVMF. OVMF
sets PcdCpuSmmSyncMode to 1 (= SmmCpuSyncModeRelaxedAp) by default;
however, OVMF's SmmControl2Dxe driver sets the PCD back to 0 (=
SmmCpuSyncModeTradition) if QEMU supports "broadcast SMI". Given that
broadcast SMI is the *only* reliable method with QEMU/KVM + OVMF, and
given that this feature / method has been available for ages now,
testing any SMM-related change in edk2 with that feature manually
disabled in QEMU is arguably useless.)

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112345): https://edk2.groups.io/g/devel/message/112345
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Wu, Jiaxin]

> >   WaitForAllAPs() has two purpose:
> >    1. Make sure all Aps have set the Present.
> >    2. Get ready for programming MTRRs to make sure cpus in the same start
> line.
> >
> > if so, that will be better as existing logic, it can also save some time for the
> Present flag check in SmmWaitForApArrival
> 
> OK, this argument makes sense to me.
> 
> I didn't realize that WaitForAllAPs() -- called by BSPHandler() after
> calling SmmWaitForApArrival() -- already *effectively* ensured that the
> APs had their Present flag set!
> 
> That happens because:
> 
> (a) WaitForAllAPs() pends the "Run" semaphore of each AP.
> 
> (b) The APHandler() function sets the Present flag *first*.
> 
> (c) If
> 
>   (SyncMode == SmmCpuSyncModeTradition) ||
>   SmmCpuFeaturesNeedConfigureMtrrs ()
> 
> is true, then APHandler() posts the "Run" semaphore, *second*.
> 
> Therefore, once WaitForAllAPs() has acquired all AP "Run" semaphores,
> all AP Present flags must be set.
> 

Yes, correct!


> This is not obvious at all, but it looks correct.
> 
> Therefore I agree that your patch does not introduce asymmetry between
> SmmCpuRendezvous() and BSPHandler(). Instead, your patch eliminates
> asymmetry, because now SmmCpuRendezvous() will wait for the Present
> flags of the APs (if the above-quoted condition is false), similarly to
> how BSPHandler already does (if the condition is true).
> 
> 
> Now, I have not had the time yet to re-review your patch set
> 
>   [PATCH v3 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib
> 
> As far as I remember (from v1), that patch set reorganizes exactly these
> "Run" semaphore release/acquire patterns.
> 
> (3) Can you confirm that your v3 patch set will not invalidate this
> discussion? I.e., can you confirm that WaitForAllAPs() will *still*
> ensure, via the Run semaphores, that the Present flags will have been set?
> 

Yes, the series patch sent by me won't impact the behavior above, smm cpu driver will still make sure the present flag set.   


> (4) Please add the following to the commit message:
> 
> -------
> BSPHandler -> { SmmWaitForApArrival, WaitForAllAPs } already awaits that
> the Present flag is set for all APs, namely via the AP Run semaphores.
> Therefore this patch ensures symmetry between BSPHandler (when [1] is
> true) and SmmCpuRendezvous() (when [1] is false).
> 
> [1] (SyncMode == SmmCpuSyncModeTradition) ||
>     SmmCpuFeaturesNeedConfigureMtrrs ()
> -------
> 
> More comments below:
> 
> >
> >>
> >> (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.
> >>
> >
> > Do you mean the below WaitForAllAPs()? There is already has the CpuPause
> check within WaitForSemaphore().
> >
> >     //
> >     // Wait for all APs to get ready for programming MTRRs
> >     //
> >     WaitForAllAPs (ApCount);
> 
> Yes, that's the pattern we should follow here.
> 
> The call chain
> 
>   BSPHandler -> WaitForAllAPs -> WaitForSemaphore -> CpuPause
> 
> already calls CpuPause() when condition [1] is true. That's the pattern
> we should follow.
> 
> When condition [1] is false, your patch implements the wait for the
> Present flags in SmmCpuRendezvous(), but there we have no CpuPause().
> So, we could / should add it, right at the end of the while (TRUE) loop.
> 
> Summary of my requests:
> 
> - the patch seems good, but please confirm that your v3 sync rework will
> leave the predicate intact that WaitForAllAPs() ensures the Present
> flags via the Run semaphores
> 

sure. 

Thanks,
Jiaxin 


> - please extend the commit message with the paragraph about symmetry
> between BSPHandler and SmmCpuRendezvous
> 
> - please consider adding CpuPause to the end of the "while (TRUE)" loop.
> 
> (BTW, the patch is not really (or usefully) testable with OVMF. OVMF
> sets PcdCpuSmmSyncMode to 1 (= SmmCpuSyncModeRelaxedAp) by default;
> however, OVMF's SmmControl2Dxe driver sets the PCD back to 0 (=
> SmmCpuSyncModeTradition) if QEMU supports "broadcast SMI". Given that
> broadcast SMI is the *only* reliable method with QEMU/KVM + OVMF, and
> given that this feature / method has been available for ages now,
> testing any SMM-related change in edk2 with that feature manually
> disabled in QEMU is arguably useless.)
> 
> Thanks!
> Laszlo



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