> > 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
