Re: [Qemu-devel] [PATCH v4 7/8] qmp: Support abstract classes on device-list-properties

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> On Mon, Nov 07, 2016 at 03:40:57PM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <address@hidden> writes:
>> 
>> > On Mon, Nov 07, 2016 at 09:09:58AM +0100, Markus Armbruster wrote:
>> >> Eduardo Habkost <address@hidden> writes:
>> >> 
>> >> > On Fri, Nov 04, 2016 at 04:45:17PM +0100, Markus Armbruster wrote:
>> >> >> Eduardo Habkost <address@hidden> writes:
>> >> >> 
>> >> >> > (CCing libvirt people, as I forgot to CC them)
>> >> >> >
>> >> >> > On Mon, Oct 31, 2016 at 03:07:23PM +0100, Igor Mammedov wrote:
>> >> >> >> On Fri, 28 Oct 2016 23:48:06 -0200
>> >> >> >> Eduardo Habkost <address@hidden> wrote:
>> >> >> >> 
>> >> >> >> > When an abstract class is used on device-list-properties, we can
>> >> >> >> > simply return the class properties registered for the class.
>> >> >> >> > 
>> >> >> >> > This will be useful if management software needs to query for
>> >> >> >> > supported options that apply to all devices of a given type (e.g.
>> >> >> >> > options supported by all CPU models, options supported by all PCI
>> >> >> >> > devices).
>> >> >> >> Patch looks fine to me but I'm not qmp interface guru
>> >> >> >> so I'd leave review up to maintainers.
>> >> >> >> 
>> >> >> >> One question though,
>> >> >> >> How would management software discover typename of abstract class?
>> >> >> >
>> >> >> > It depends on the use case. On some cases, management may already
>> >> >> > have bus-specific logic that will know what's the base type it
>> >> >> > needs to query (e.g. it may query "pci-device" to find out if all
>> >> >> > PCI devices support a given option). On other cases, it may be
>> >> >> > discovered using other commands.
>> >> >> 
>> >> >> The stated purpose of this feature is to let management software "query
>> >> >> for supported options that apply to all devices of a given type".  I
>> >> >> suspect that when management software has a notion of "a given type", 
>> >> >> it
>> >> >> knows its name.
>> >> >> 
>> >> >> Will management software go fishing for subtype relationships beyond 
>> >> >> the
>> >> >> types it knows?  I doubt it.  Of course, management software developers
>> >> >> are welcome to educate me :)
>> >> >> 
>> >> >> > For the CPU case, I will propose adding the base QOM CPU typename
>> >> >> > in the query-target command.
>> >> >> 
>> >> >> Does this type name vary?  If yes, can you give examples?
>> >> >
>> >> > It does. x86-specific CPU properties are on the x86_64-cpu and
>> >> > i386-cpu classes. arm-specific CPU properties are on the arm-cpu
>> >> > class.
>> >> 
>> >> I see we have concrete CPUs (such as "Westmere-x86_64-cpu"), which are
>> >> subtypes of an abstract CPU (such as "x86_64-cpu"), which is a subtype
>> >> of "cpu", which is a subtype of "device", which is a subtype of
>> >> "object".
>> >> 
>> >> The chain "cpu" - "device" - "object" is fixed and well-known.
>> >> 
>> >> The link from there to the concrete CPU varies.  Whether it could be
>> >> considered well-known or not is debatable.
>> >> 
>> >> My true question is: should we have a special purpose interface to get
>> >> the abstract supertype of concrete CPU types, or should be have general
>> >> purpose means to introspect the subtype hierarchy?
>> >> 
>> >> Note that we have the latter already, although in a rather cumbersome
>> >> form:
>> >> 
>> >>     { "execute": "qom-list-types",
>> >>       "arguments": { "implements": T, "abstract": true } }
>> >> 
>> >> lists all subtypes of T.  You can filter out the concrete subtypes by
>> >> subtracting the same query with "abstract": false.  Start with the
>> >> type you're interested in, find all its abstract supertypes.  If you
>> >> need to know more, repeat for the types you found.
>> >
>> > Looks cumbersome, because I don't see a way to find all
>> > supertypes of a given type without walking the whole tree
>> > starting from "object" (is there one?). But it could be improved
>> > a bit if we added a "implements" field to ObjectTypeInfo.
>> 
>> My point is: we can skip discussing whether we should expose the subtype
>> relation, because we already do.
>
> Correct. My only problem is that it seems to add extra
> assumptions to the code (e.g. that there's only one abstract CPU
> type). But if libvirt is careful, it doesn't need to make any
> assumptions: it can explore the type hierarchy and confirm that
> the assumptions are correct.
>
>> 
>> > But, maybe we should take a step back: my original goal was to
>> > let libvirt know which properties are supported by any CPU model
>> > when using "-cpu".
>> 
>> Why is that useful?
>
> libvirt wants to know if the QEMU binary supports a given -cpu
> option (normally CPU features that can be enabled/disabled using
> "+foo"/"-foo").

The obvious way to check whether a specific CPU supports it is to
introspect that CPU.

The obvious way to check whether all CPUs of interest support it
(assuming that is a productive question) is to introspect all CPUs of
interest.  Works regardless of whether the option is inherited, which is
really an implementation detail.

>> >                    Maybe we should make QEMU do the QOM->option
>> > translation and implement "-cpu" support in
>> > query-command-line-options? We already do something similar with
>> > "-machine".
>> 
>> query-command-line-options is flawed, and the less it's used, the
>> happier I am.
>
> No problem, let's just forget my suggestion.  :)
>
>> 
>> Real command line introspection would be nice, but getting it will
>> involve cleaning up the unholy mess our command line has become.  Don't
>> hold your breath.
>> 
>> My preferred workaround is to find something equivalent to introspect
>> via QMP.
>
> Sounds good to me.
>
>> 
>> That said, if you come up with a patch that solves a real problem now, I
>> won't object to it just because it involves query-command-line-options.
>
> I think I will keep the current approach. Changing
> query-command-line-option would probably be more complex and
> solve only the -cpu problem. Making QOM introspection better
> looks simpler and will probably be useful for other use cases.

Makes sense.

> I am still tempted to try to make QOM->query-command-line-option
> mapping easier and simpler, but it doesn't seem to be the answer
> to the current problem.