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@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@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;
+
+ if (Handler != NULL) {
+ Handler (CpuIndex);
+ }
+ }
}
/**
--
2.9.3
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71918): https://edk2.groups.io/g/devel/message/71918
Mute This Topic: https://groups.io/mt/80819862/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 22/02/21 15:53, Laszlo Ersek wrote:
>> +
>> + 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.
In Linux terms, there is a control dependency here. However, it should
at least be a separate statement to load mCpuHotEjectData (which from my
EDK2 reminiscences should be a global) into a local variable. So
EjectData = mCPUHotEjectData;
// Optional AcquireMemoryFence here
if (EjectData != NULL) {
CPU_HOT_EJECT_HANDLER Handler;
Handler = EjectData->Handler;
if (Handler != NULL) {
Handler (CpuIndex);
}
}
Thanks,
Paolo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72038): https://edk2.groups.io/g/devel/message/72038
Mute This Topic: https://groups.io/mt/80819862/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 02/23/21 08:45, Paolo Bonzini wrote:
> On 22/02/21 15:53, Laszlo Ersek wrote:
>>> +
>>> + 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.
>
> In Linux terms, there is a control dependency here. However, it should
> at least be a separate statement to load mCpuHotEjectData (which from my
> EDK2 reminiscences should be a global) into a local variable. So
>
> EjectData = mCPUHotEjectData;
> // Optional AcquireMemoryFence here
> if (EjectData != NULL) {
> CPU_HOT_EJECT_HANDLER Handler;
>
> Handler = EjectData->Handler;
> if (Handler != NULL) {
> Handler (CpuIndex);
> }
> }
Yes, "mCPUHotEjectData" is a global.
"mCpuHotEjectData" itself is set up on the BSP (from the entry point
function of the PiSmmCpuSmmDxe driver), before any other APs have a
chance to execute any SMM-related code at all. Furthermore, once set up,
mCpuHotEjectData never changes -- it remains set to a particular
non-NULL value forever, or it remains NULL forever. (The latter case
applies when the possible CPU count is 1; IOW, then there is no AP at all.)
Because of that, I thought that the first comparison (mCpuHotEjectData
!= NULL) would not need any fence -- I thought it was similar to a
userspace program that (a) set a global variable in the "main" thread,
before calling pthread_create(), (b) treated the global variable as a
constant, ever after (meaning all threads).
However, mCpuHotEjectData->Handler is changed regularly (modified by the
BSP, and read "later" by all processors). That's why I thought the
acquire fence was needed in the following location:
if (mCpuHotEjectData != NULL) {
CPU_HOT_EJECT_HANDLER Handler;
//
// HERE -- AcquireMemoryFence()
//
Handler = mCpuHotEjectData->Handler;
if (Handler != NULL) {
Handler (CpuIndex);
}
}
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72093): https://edk2.groups.io/g/devel/message/72093
Mute This Topic: https://groups.io/mt/80819862/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 23/02/21 18:06, Laszlo Ersek wrote:
> On 02/23/21 08:45, Paolo Bonzini wrote:
>> On 22/02/21 15:53, Laszlo Ersek wrote:
>>>> +
>>>> + 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.
>>
>> In Linux terms, there is a control dependency here. However, it should
>> at least be a separate statement to load mCpuHotEjectData (which from my
>> EDK2 reminiscences should be a global) into a local variable. So
>>
>> EjectData = mCPUHotEjectData;
>> // Optional AcquireMemoryFence here
>> if (EjectData != NULL) {
>> CPU_HOT_EJECT_HANDLER Handler;
>>
>> Handler = EjectData->Handler;
>> if (Handler != NULL) {
>> Handler (CpuIndex);
>> }
>> }
>
> Yes, "mCPUHotEjectData" is a global.
>
> "mCpuHotEjectData" itself is set up on the BSP (from the entry point
> function of the PiSmmCpuSmmDxe driver), before any other APs have a
> chance to execute any SMM-related code at all. Furthermore, once set up,
> mCpuHotEjectData never changes -- it remains set to a particular
> non-NULL value forever, or it remains NULL forever. (The latter case
> applies when the possible CPU count is 1; IOW, then there is no AP at all.)
Ok, that's what I was missing. However, your code below has *two* loads
of mCpuHotEjectData and the fence would have to go after the second
(between the load of mCpuHotEjectData and the load of the Handler
field). Therefore I would still use a local variable even if you decide
to put the fence inside the "if", which I agree is the clearest.
Paolo
> Because of that, I thought that the first comparison (mCpuHotEjectData
> != NULL) would not need any fence -- I thought it was similar to a
> userspace program that (a) set a global variable in the "main" thread,
> before calling pthread_create(), (b) treated the global variable as a
> constant, ever after (meaning all threads).
>
> However, mCpuHotEjectData->Handler is changed regularly (modified by the
> BSP, and read "later" by all processors). That's why I thought the
> acquire fence was needed in the following location:
>
> if (mCpuHotEjectData != NULL) {
> CPU_HOT_EJECT_HANDLER Handler;
>
> //
> // HERE -- AcquireMemoryFence()
> //
> Handler = mCpuHotEjectData->Handler;
> if (Handler != NULL) {
> Handler (CpuIndex);
> }
> }
>
> Thanks!
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72094): https://edk2.groups.io/g/devel/message/72094
Mute This Topic: https://groups.io/mt/80819862/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 2021-02-23 9:18 a.m., Paolo Bonzini wrote:
> On 23/02/21 18:06, Laszlo Ersek wrote:
>> On 02/23/21 08:45, Paolo Bonzini wrote:
>>> On 22/02/21 15:53, Laszlo Ersek wrote:
>>>>> +
>>>>> + 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.
>>>
>>> In Linux terms, there is a control dependency here. However, it should
>>> at least be a separate statement to load mCpuHotEjectData (which from my
>>> EDK2 reminiscences should be a global) into a local variable. So
>>>
>>> EjectData = mCPUHotEjectData;
>>> // Optional AcquireMemoryFence here
>>> if (EjectData != NULL) {
>>> CPU_HOT_EJECT_HANDLER Handler;
>>>
>>> Handler = EjectData->Handler;
>>> if (Handler != NULL) {
>>> Handler (CpuIndex);
>>> }
>>> }
>>
>> Yes, "mCPUHotEjectData" is a global.
>>
>> "mCpuHotEjectData" itself is set up on the BSP (from the entry point
>> function of the PiSmmCpuSmmDxe driver), before any other APs have a
>> chance to execute any SMM-related code at all. Furthermore, once set up,
>> mCpuHotEjectData never changes -- it remains set to a particular
>> non-NULL value forever, or it remains NULL forever. (The latter case
>> applies when the possible CPU count is 1; IOW, then there is no AP at all.)
>
> Ok, that's what I was missing. However, your code below has *two* loads of mCpuHotEjectData and the fence would have to go after the second (between the load of mCpuHotEjectData and the load of the Handler field). Therefore I would still use a local variable even if you decide to put the fence inside the "if", which I agree is the clearest.
Sorry, I'm missing something here. As Laszlo said given that mCpuHotEjectData
does not change after being set, so why would it be a problem in referencing it
twice?
The generated code looks like this (load for mCpuHotEjectData at 0xf54b and
then the dependent mCpuHotEjectData->Handler load on 0xf645):
# 17d60 <mCpuHotEjectData>
f54b: 48 8b 05 0e 88 00 00 mov 0x880e(%rip),%rax
f54e: R_X86_64_PC32 .data+0x1d5c
f552: 48 85 c0 test %rax,%rax
f555: 0f 85 ea 00 00 00 jne f645 <SmiRendezvous+0x17e>
# Handler = mCpuHotEjectData->Handler
f645: 48 8b 40 08 mov 0x8(%rax),%rax
f649: 48 85 c0 test %rax,%rax
f64c: 74 05 je f653 <SmiRendezvous+0x18c>
f64e: 4c 89 e1 mov %r12,%rcx
f651: ff d0 callq *%rax
In the worst case, however, maybe it looks like this (two loads for
mCpuHotEjectData and then the dependent load):
# 17d60 <mCpuHotEjectData>
f54b: 48 8b 05 0e 88 00 00 mov 0x880e(%rip),%rax
f54e: R_X86_64_PC32 .data+0x1d5c
f552: 48 85 c0 test %rax,%rax
f555: 0f 85 ea 00 00 00 jne f645 <SmiRendezvous+0x17e>
# 17d60 <mCpuHotEjectData>
f645: 48 8b 05 0e 88 00 00 mov 0x880e(%rip),%rax
+3: R_X86_64_PC32 .data+0x1d5c
# Handler = mCpuHotEjectData->Handler
+7: 48 8b 40 08 mov 0x8(%rax),%rax
+11: 48 85 c0 test %rax,%rax
+14: 74 05 je f653 <SmiRendezvous+0x18c>
+16: 4c 89 e1 mov %r12,%rcx
+19: ff d0 callq *%rax
As you and Laszlo say -- we do need an acquire fence before this line
(which corresponds to the release fence in UnplugCpus(), patch 8
and the release fence in EjectCpu() in patch 9).
# Handler = mCpuHotEjectData->Handler
48 8b 40 08 mov 0x8(%rax),%rax
A local variable for mCpuHotEjectData, would be nice to have but I'm
not sure it is needed for correctness.
Ankur
> Paolo
>
>> Because of that, I thought that the first comparison (mCpuHotEjectData
>> != NULL) would not need any fence -- I thought it was similar to a
>> userspace program that (a) set a global variable in the "main" thread,
>> before calling pthread_create(), (b) treated the global variable as a
>> constant, ever after (meaning all threads).
>>
>> However, mCpuHotEjectData->Handler is changed regularly (modified by the
>> BSP, and read "later" by all processors). That's why I thought the
>> acquire fence was needed in the following location:
>>
>> if (mCpuHotEjectData != NULL) {
>> CPU_HOT_EJECT_HANDLER Handler;
>>
>> //
>> // HERE -- AcquireMemoryFence()
>> //
>> Handler = mCpuHotEjectData->Handler;
>> if (Handler != NULL) {
>> Handler (CpuIndex);
>> }
>> }
>>
>> Thanks!
>> Laszlo
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72114): https://edk2.groups.io/g/devel/message/72114
Mute This Topic: https://groups.io/mt/80819862/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Aaron Young <aaron.young@oracle.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 02/23/21 08:37, Ankur Arora wrote:
> 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@redhat.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Cc: Aaron Young <aaron.young@oracle.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>>> Signed-off-by: Ankur Arora <ankur.a.arora@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.
If you could do that, that would be awesome. It would make further work
(introducing the more specific fences) much easier.
I'll try to review the remaining patches in v8 still today.
Thanks!
Laszlo
>
>> ... 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 (#72091): https://edk2.groups.io/g/devel/message/72091
Mute This Topic: https://groups.io/mt/80819862/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.