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

[Daniel P. Berrangé]

On Mon, Feb 07, 2022 at 12:22:22PM +0100, Igor Mammedov wrote:
> On Mon, 7 Feb 2022 10:36:42 +0100
> Peter Krempa <pkrempa at 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 at redhat.com> wrote:
> > >   
> > > > On Sat,  5 Feb 2022 13:45:24 +0100
> > > > Philippe Mathieu-Daudé <f4bug at 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.

AFAIK, we've never defined anything about QOM paths wrt ABI one way
or the other ? In the absence of guidelines then it comes down to
what are reasonable expectations of the mgmt app. These expectations
will be influenced by what it is actually possible to acheive given
our API as exposed.

I think it is unreasonable to expect /machine/unattached to be
stable because by its very nature it is just a dumping ground
for anything where the dev hasn't put in any thought to the path
placement.  IOW, it was/is definitely a bad idea for libvirt to
rely on /machine/unattached in any way. That we're liable to be
broken is not nice, but its really our own fault - we should 
have asked for a better solution from day one here.


I think it is somewhat reasonable to expect other QOM paths to
be stable as there's been some degree of thought put into their
placement. If we don't want apps to be considering other
paths to be stable, then we need to explain exactly what they
can and can't rely on, and most importantly actually provide
a way for them to do what they need


For example, libvirt needs a QOM path to query memory balloon
stats. All libvirt knows is that it set 'id=balloon0' when
creating it on the CLI. To find the balloon device in QOM it
then looks for all paths under '/machine/peripheral', and
tries to find one called 'child<$ID>' where $ID is the id=xxx
value from the CLI.

We do the same dance when we need to find out where thue
default VGA device we created lives.

This all feels kinda silly as we're going through a dance to
dynamically find the device, but in practice it is no better
than just hardcoding it.

The problem we face in these examp\les is that as an input we
are giving QMEU a device 'id' but at runtime we're needing to
then use a QOM path instead of the 'id'. So we need a way to
translate an 'id' to a QOM path. What is the right way to do
this in a supported manner without making any assumptions
about QOM paths ?


For the CPUs cases, we don't have any 'id' on the CLI since
CPUs aren't configured that way, so we just hardcoded the
full path. You've pointed out query-hotpluggable-cpus which
is a possible solution.

In another case we're assuming that '/machine' has a property
called 'rtc-time'. IMHO it is reasonable to assume that
'/machine' as a QOM path is stable.

It isn't as simple as just saying "all QOM paths are unstable".

I struggle to come up with a good rule to explain what apps can
/ can't rely on wrt QOM paths, other than stay far away from
anything with '/unattached'.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|