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

["Ankur Arora" <ankur.a.arora@oracle.com>]

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);
>> +    }
>> +  }
>>   }
>>   
>>   /**
>>
>