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

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

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