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

["Laszlo Ersek" <lersek@redhat.com>]

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