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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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