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

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

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