On Thu, 10 Dec 2020 12:08:13 -0800
"Ankur Arora" <ankur.a.arora@...> wrote:

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.

I don't like implementing workarounds on hw side for guest software sake.
(it's occasionally done but only if there is no way to fix guest side,
for example closed sources OS. So there shall be very good reason to do so)

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