Re: [edk2-devel] [PATCH v8 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject

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

On 02/24/21 04:44, Ankur Arora wrote:
> On 2021-02-23 1:39 p.m., Laszlo Ersek wrote:
>> On 02/22/21 08:19, Ankur Arora wrote:

>>> +    UINT32 Idx;
>>> +
>>> +    for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
>>> +      UINT64 QemuSelector;
>>> +
>>> +      QemuSelector = mCpuHotEjectData->QemuSelectorMap[Idx];
>>> +
>>> +      if (QemuSelector != CPU_EJECT_QEMU_SELECTOR_INVALID) {
>>> +        //
>>> +        // This to-be-ejected-CPU has already received the BSP's SMI
>>> exit
>>> +        // signal and, will execute SmmCpuFeaturesRendezvousExit()
>>> +        // followed by this callback or is already waiting in the
>>> +        // CpuSleep() loop below.
>>> +        //
>>> +        // Tell QEMU to context-switch it out.
>>> +        //
>>> +        QemuCpuhpWriteCpuSelector (mMmCpuIo, (UINT32) QemuSelector);
>>> +        QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECT);
>>> +
>>> +        //
>>> +        // We need a compiler barrier here to ensure that the compiler
>>> +        // does not reorder the CpuStatus and QemuSelectorMap[Idx]
>>> stores.
>>> +        //
>>> +        // A store fence is not strictly necessary on x86 which has
>>> +        // TSO; however, both of these stores are in different
>>> address spaces
>>> +        // so also add a Store Fence here.
>>> +        //
>>> +        MemoryFence ();
>>
>> (6) I wonder if this compiler barrier + comment block are helpful.
>> Paraphrasing your (ex-)colleague Liran, if MMIO and IO Port accessors
>> didn't contain built-in fences, all hell would break lose. We're using
>> EFI_MM_CPU_IO_PROTOCOL for IO Port accesses. I think we should be safe
>> ordering-wise, even without an explicit compiler barrier here.
>>
>> To me personally, this particular fence only muddies the picture --
>> where we already have an acquire memory fence and a store memory fence
>> to couple with each other.
>>
>> I'd recommend removing this. (If you disagree, I'm willing to listen to
>> arguments, of course!)
> 
> You are right that we don't need a memory fence here -- given that there
> is an implicit fence due to the MMIO.
> 
> As for the compiler fence, I'm just now re-looking at handlers in
> EFI_MM_CPU_IO_PROTOCOL and they do seem to include a compiler barrier.
> 
> So I agree with you that we have all the fences that we need. However,
> I do think it's a good idea to document both of these here.

OK.

>>> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>>> b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>>> index 99988285b6a2..ddfef05ee6cf 100644
>>> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>>> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>>> @@ -472,6 +472,37 @@ SmmCpuFeaturesRendezvousExit (
>>>     // (PcdCpuMaxLogicalProcessorNumber > 1), and hot-eject is needed
>>>     // in this SMI exit (otherwise mCpuHotEjectData->Handler is not
>>> armed.)
>>>     //
>>> +  // mCpuHotEjectData itself is stable once setup so it can be
>>> +  // dereferenced without needing any synchronization,
>>> +  // but, mCpuHotEjectData->Handler is updated on the BSP in the
>>> +  // ongoing SMI iteration at two places:
>>> +  //
>>> +  // - UnplugCpus() where the BSP determines if a CPU is under ejection
>>> +  //   or not. As the comment where mCpuHotEjectData->Handler is set-up
>>> +  //   describes any such updates are guaranteed to be
>>> ordered-before the
>>> +  //   dereference below.
>>> +  //
>>> +  // - EjectCpu() (which is called via the Handler below), on the BSP
>>> +  //   updates mCpuHotEjectData->Handler once it is done with all
>>> ejections.
>>> +  //
>>> +  //   The CPU under ejection: might be executing anywhere between the
>>> +  //   "AllCpusInSync" exit loop in SmiRendezvous() to about to
>>> +  //   dereference the Handler field.
>>> +  //   Given that the BSP ensures that this store only happens after
>>> all
>>> +  //   CPUs under ejection have been ejected, this CPU would never see
>>> +  //   the after value.
>>> +  //   (Note that any CPU that is already executing the CpuSleep() loop
>>> +  //   below never raced any updates and always saw the before value.)
>>> +  //
>>> +  //   CPUs not-under ejection: might see either value of the Handler
>>> +  //   which is fine, because the Handler is a NOP for CPUs not-under
>>> +  //   ejection.
>>> +  //
>>> +  //   Lastly, note that we are also guaranteed that any dereferencing
>>> +  //   CPU only sees the before or after value and not an intermediate
>>> +  //   value. This is because mCpuHotEjectData->Handler is aligned at a
>>> +  //   natural boundary.
>>> +  //
>>>       if (mCpuHotEjectData != NULL) {
>>>       CPU_HOT_EJECT_HANDLER Handler;
>>>
>>
>> (8) I can't really put my finger on it, I just feel that repeating
>> (open-coding) this wall of text here is not really productive.
> 
> Part of the reason I wanted to document this here was to get your
> opinion on it and figure out how much of it is useful and how
> much might be overkill.
> 
>>
>> Do you think that, after you add the "acquire memory fence" comment in
>> patch #7, we could avoid most of the text here? I think we should only
>> point out (in patch #7) the "release fence" that the logic here pairs
>> with.> If you really want to present it all from both perspectives, I
>> guess I'm
>> OK with that, but then I believe we should drop the last paragraph at
>> least (see point (4)).
> 
> Rereading it after a gap of a few days and given that most of this is
> just a repeat, I'm also tending towards overkill. I think a comment
> talking about acquire/release pairing is useful. Rest of it can probably
> be met with just a pointer towards the comment in EjectCpus(). Does that
> make sense?

Yes, absolutely. Short comment + pointer to the "other half" (which has
the large comment too) seem best.

Thanks
Laszlo