Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

2021-02-23 Thread Ankur Arora 

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 
f54b:   48 8b 05 0e 88 00 00mov0x880e(%rip),%rax
f54e: R_X86_64_PC32 .data+0x1d5c
f552:   48 85 c0test   %rax,%rax
f555:   0f 85 ea 00 00 00   jnef645 

  # Handler = mCpuHotEjectData->Handler
f645:   48 8b 40 08 mov0x8(%rax),%rax
f649:   48 85 c0test   %rax,%rax
f64c:   74 05   je f653 
f64e:   4c 89 e1mov%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 
f54b:   48 8b 05 0e 88 00 00mov0x880e(%rip),%rax
f54e: R_X86_64_PC32 .data+0x1d5c
f552:   48 85 c0test   %rax,%rax
f555:   0f 85 ea 00 00 00   jnef645 

  # 17d60 
f645:   48 8b 05 0e 88 00 00mov0x880e(%rip),%rax
  +3: R_X86_64_PC32 .data+0x1d5c

  # Handler = mCpuHotEjectData->Handler
  +7:   48 8b 40 08 mov0x8(%rax),%rax
 +11:   48 85 c0test   %rax,%rax
 +14:   74 05   je f653 
 +16:   4c 89 e1mov%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 mov0x8(%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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub

Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

2021-02-23 Thread Paolo Bonzini 

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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

2021-02-23 Thread Laszlo Ersek 
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

2021-02-23 Thread Laszlo Ersek 
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 
>>> Cc: Jordan Justen 
>>> Cc: Ard Biesheuvel 
>>> Cc: Igor Mammedov 
>>> Cc: Boris Ostrovsky 
>>> Cc: Aaron Young 
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>>> Signed-off-by: Ankur Arora 
>>> ---
>>>
>>> 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, );  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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

2021-02-22 Thread Paolo Bonzini 

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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

2021-02-22 Thread Ankur Arora 

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 
Cc: Jordan Justen 
Cc: Ard Biesheuvel 
Cc: Igor Mammedov 
Cc: Boris Ostrovsky 
Cc: Aaron Young 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora 
---

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, );  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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

2021-02-22 Thread Laszlo Ersek 
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 
> Cc: Jordan Justen 
> Cc: Ard Biesheuvel 
> Cc: Igor Mammedov 
> Cc: Boris Ostrovsky 
> Cc: Aaron Young 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora 
> ---
> 
> 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, );  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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

2021-02-21 Thread Ankur Arora 
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 
Cc: Jordan Justen 
Cc: Ard Biesheuvel 
Cc: Igor Mammedov 
Cc: Boris Ostrovsky 
Cc: Aaron Young 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora 
---

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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-