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

["Igor Mammedov" <imammedo@redhat.com>]

On Thu, 10 Dec 2020 12:08:13 -0800
"Ankur Arora" <ankur.a.arora@oracle.com> 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
> >   
> 
> 
> 
> 
>