Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Peter Krempa]

On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:
> On Mon, 7 Feb 2022 09:14:37 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Sat,  5 Feb 2022 13:45:24 +0100
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > 
> > > Previously CPUs were exposed in the QOM tree at a path
> > > 
> > >   /machine/unattached/device[nn]
> > > 
> > > where the 'nn' of the first CPU is usually zero, but can
> > > vary depending on what devices were already created.
> > > 
> > > With this change the CPUs are now at
> > > 
> > >   /machine/cpu[nn]
> > > 
> > > where the 'nn' of the first CPU is always zero.  
> > 
> > Could you add to commit message the reason behind the change?
> 
> regardless, it looks like unwarranted movement to me
> prompted by livirt accessing/expecting a QOM patch which is
> not stable ABI. I'd rather get it fixed on libvirt side.
> 
> If libvirt needs for some reason access a CPU instance,
> it should use @query-hotpluggable-cpus to get a list of CPUs
> (which includes QOM path of already present CPUs) instead of
> hard-codding some 'well-known' path as there is no any guarantee 
> that it will stay stable whatsoever.

I don't disagree with you about the use of hardcoded path, but the way
of using @query-hotpluggable-cpus is not really aligning well for how
it's being used.

To shed a bit more light, libvirt uses the following hardcoded path

#define QOM_CPU_PATH  "/machine/unattached/device[0]"

in code which is used to query CPU flags. That code doesn't care at all
which cpus are present but wants to get any of them. So yes, calling
query-hotpluggable-cpus is possible but a bit pointless.

In general the code probing cpu flags via qom-get is very cumbersome as
it ends up doing ~400 QMP calls at startup of a VM in cases when we deem
it necessary to probe the cpu fully.

It would be much better (And would sidestep the issue altoghether) if we
had a more sane interface to probe all cpu flags in one go, and ideally
the argument specifying the cpu being optional.

Libvirt can do the adjustment, but for now IMO the path to the first cpu
(/machine/unattached/device[0]) became de-facto ABI by the virtue that
it was used by libvirt and if I remember correctly it was suggested by
the folks dealing with the CPU when the code was added originally.

Even if we change it in libvirt right away, changing qemu will break
forward compatibility. While we don't guarantee it, it still creates
user grief.