On 2021-02-22 6:53 a.m., Laszlo Ersek wrote:
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);
Thanks for the link (and Paolo for writing it.) This is great.
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.
Yeah you are right. Just think out aloud here... without this it is possible
that on the the AP, the CPU could reorder loads on line-1 and line-3.
This patch does need an AcquireMemoryFence() (or a MemoryFence() and a
comment stating that it needs acquire semantics.
This also makes me realize that although I have somewhat detailed comments
in patches 8 and 9, but I do need to specify which fence needs to have
acquire semantics and which release.
... 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.
Agreed so far! And, thanks.
Ankur
Thanks!
Laszlo
+
+ if (Handler != NULL) {
+ Handler (CpuIndex);
+ }
+ }
}
/**
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72035): https://edk2.groups.io/g/devel/message/72035
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]
-=-=-=-=-=-=-=-=-=-=-=-
