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

[Igor Mammedov]

On Mon, 7 Feb 2022 10:36:42 +0100
Peter Krempa <pkrempa@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@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.

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.
>