Re: [RFC PATCH 0/5] support CPU hot-unplug

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

On 2020-12-10 1:21 a.m., Laszlo Ersek wrote:
> Hi Ankur,
> 
> On 12/08/20 06:34, Ankur Arora wrote:
>> [ Resending to the correct edk2 alias this time. ]
>>
>> Hi,
>>
>> This series adds support for CPU hot-unplug with OVMF.
>>
>> Please see this in conjunction with the QEMU v2 series posted here:
>>    https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
>>
>> In particular, would be glad for comments on Patch 4, specifically
>> where we should be ejecting the CPU.
>>
>> Right now the ejection happens in UnplugCpus() (called from
>> CpuHotplugMmi()):
>>    +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
>>    +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
>>
>> That is way too early -- given that the actual unplug will happen
>> in SmmCpuUpdate() and given that the BSPHandler() would have waited
>> for the APs multiple times before then.
>>
>> Another possibility is that the actual ejection be deferred to the
>> _EJ0 method after the return from the SMI. But, that seems like a
>> hack. Additionally, Igor points out here that this approach has problems:
>>    https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/
>>
>> Please review.
> 
> thanks for the patches; I'm confirming I've got them.
> 
> I'll need a non-trivial amount of time before I come to these patches
> (and to the QEMU patches posted by Igor).
> 
> I'm working very busily on
> <https://bugzilla.tianocore.org/show_bug.cgi?id=3097> and my brain is
> full of other stuff.

Thanks for letting me know. I empathize with not wanting to context
switch out all of your built up virtio-fs/ARM state.

> 
> We had the reverse situation earlier this year, I think, when -- in
> relation to hotplug -- Igor was occupied with a more pressing QEMU
> development (NUMA I think?), for a significant amount of time.
> 
> What's important is that I want to do both Igor's patches and your
> patches *justice*, with my review, and at this time I just cannot sit
> down with them alone for a day. These patches deserve a deep looking-at,
> rather than a skim, and I cannot afford the former at the moment. I
> prefer doing a (hopefully) thorough review, later, to rushing a review now.

I'll look forward to it. Anyway I think a deep look at these patches might
be wasted at the current stage. In particular there's a glaring hole in this
patch set which is how to handle the actual unplug (setting of
QEMU_CPUHP_STAT_EJECTED).

That's one thing I would be glad for a comment on: not right away, please
come back to this when you have thinking room.

So the problem is that my current approach -- setting QEMU_CPUHP_STAT_EJECTED
via the CpuHotplugMmi() handler definitely doesn't work given that it removes
an AP immediately while the SMI handler is still using it.

The two alternatives are:
  - do this in SmmCpuFeaturesLib::SmmCpuFeaturesRendezvousExit() while exiting
    SMI. That means that the only thing we will not do on the AP being unplugged
    is restoring debug registers and a bunch of MSRs. Which AFAICS would be
    okay, since the next time this AP is plugged in it will start from a clean
    slate anyway.

  - Qemu marks the hot-unplug when QEMU_CPUHP_STAT_EJECTED is set and defers it
    until the SMI exit.

AFAICS, both ought to work. But, assuming it works (I haven't tried it out yet)
the first seems cleaner.

Ankur


> 
> I hope that's tolerable.
> 
> Thank you,
> Laszlo
>