Adding Paolo, one comment below: On 02/22/21 08:19, Ankur Arora wrote: > Call the CPU hot-eject handler if one is installed. The condition for > installation is (PcdCpuMaxLogicalProcessorNumber > 1), and there's > a hot-unplug request. > > The handler executes in context of SmmCpuFeaturesRendezvousExit(), > which is called at the tail end of SmiRendezvous() after the BSP has > given the signal to exit via the "AllCpusInSync" loop. > > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Ard Biesheuvel <ard.biesheu...@arm.com> > Cc: Igor Mammedov <imamm...@redhat.com> > Cc: Boris Ostrovsky <boris.ostrov...@oracle.com> > Cc: Aaron Young <aaron.yo...@oracle.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132 > Signed-off-by: Ankur Arora <ankur.a.ar...@oracle.com> > --- > > Notes: > Address the following review comments from v6, patch-6: > (19a) Move the call to the ejection handler to a separate patch. > (19b) Describe the calling context of SmmCpuFeaturesRendezvousExit(). > (20) Add comment describing the state when the Handler is not armed. > > OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > index adbfc90ad46e..99988285b6a2 100644 > --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > @@ -467,6 +467,21 @@ SmmCpuFeaturesRendezvousExit ( > IN UINTN CpuIndex > ) > { > + // > + // We only call the Handler if CPU hot-eject is enabled > + // (PcdCpuMaxLogicalProcessorNumber > 1), and hot-eject is needed > + // in this SMI exit (otherwise mCpuHotEjectData->Handler is not armed.) > + // > + > + if (mCpuHotEjectData != NULL) { > + CPU_HOT_EJECT_HANDLER Handler; > + > + Handler = mCpuHotEjectData->Handler;
This patch looks otherwise OK to me, but: In patch v8 08/10, we have a ReleaseMemoryFence(). (For now, it is only expressed as a MemoryFence() call; we'll make that more precise later.) (1) I think that should be paired with an AcquireMemoryFence() call, just before loading "mCpuHotEjectData->Handler" above -- for now, also expressed as a MemoryFence() call only. BTW the first article in Paolo's series has been published: https://lwn.net/Articles/844224/ so in terms of that, we have something similar to this diagram: thread 1 thread 2 -------------------------------- ------------------------ a.x = 1; smp_wmb(); WRITE_ONCE(message, &a); datum = READ_ONCE(message); smp_rmb(); if (datum != NULL) printk("%x\n", datum->x); In patch 8, UnplugCpus() does the first two lines of the "thread 1" (BSP) actions, and the third line is covered by the final "AllCpusInSync = FALSE" assignment in BSPHandler() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c]. Regarding the thread#2 (AP) actions, line#1 is covered by the "AllCpusInSync loop" near the end of SmiRendezvous(). Lines 3+ are covered by our SmmCpuFeaturesRendezvousExit() implementation here. But line#2 (the AcquireMemoryFence()) is missing. ... I'll suspend the review at this point for today; let's see whether we agree on the comments I've made so far. I hope to continue the review tomorrow. Thanks! Laszlo > + > + if (Handler != NULL) { > + Handler (CpuIndex); > + } > + } > } > > /** > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#71979): https://edk2.groups.io/g/devel/message/71979 Mute This Topic: https://groups.io/mt/80819862/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
