Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine
On Mon, 7 Feb 2022 10:36:42 +0100
Peter Krempa <pkrempa(a)redhat.com> wrote:
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(a)redhat.com> wrote:
>
> > On Sat, 5 Feb 2022 13:45:24 +0100
> > Philippe Mathieu-Daudé <f4bug(a)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.
Even though query-hotpluggable-cpus is cumbersome
it still lets you avoid hardcodding QOM path and let you
get away with keeping "_400 QMP calls" probing while
something better comes along.
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.
I would've
argued against that back then as well,
there weren't any guarantee and I wouldn't like precedent of
QOM abuse becoming de-facto ABI.
Note: this patch breaks this so called ABI as well and introduces
yet another harcodded path without any stability guarantee whatsoever.
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.