[edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler
Ankur Arora
ankur.a.arora at oracle.com
Tue Feb 23 07:37:35 UTC 2021
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 <lersek at redhat.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Cc: Igor Mammedov <imammedo at redhat.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky at oracle.com>
>> Cc: Aaron Young <aaron.young at oracle.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list