qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	qemu-devel@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: QMP introspecting device props common to a bus type
Date: Thu, 08 Apr 2021 16:59:34 +0200	[thread overview]
Message-ID: <87o8eo9609.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <YG77DnwTyCVPL3nw@redhat.com> ("Daniel P. =?utf-8?Q?Berrang?= =?utf-8?Q?=C3=A9=22's?= message of "Thu, 8 Apr 2021 13:46:06 +0100")

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Apr 08, 2021 at 01:56:28PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > When introspecting properties for devices, libvirt issues a sequence of
>> > QMP  'device-list-properties' commands, one for each device type we
>> > need info for.  The result of this command tells us about all properties
>> > possible on that specific device, which is generally just fine.
>> >
>> > Every now and then though, there are properties that are inherited from
>> > / defined by the parent class, usually props that are common to all
>> > devices attached to a given bus type.
>> >
>> > The current case in point is the "acpi-index" property that was added to
>> > the "PCI" bus type, that is a parent for any type that is a PCI dev.
>> >
>> > Generally when libvirt adds support for a property, it will enable it
>> > across all devices that can support the property. So we're enabling use
>> > of "acpi-index" across all PCI devices.
>> >
>> > The question thus becomes how should we probe for existence of the
>> > "acpi-index" property. The qemu-system-x86_64 emulator has somewhere
>> > around 150 user creatable PCI devices according to "-device help".
>> >
>> > The existance of a class hierarchy is explicitly not exposed in QMP
>> > because we consider that an internal impl detail, so we can't just
>> > query "acpi-index" on the "PCI" parent type. 
>> 
>> Not true.
>> 
>> qapi/qom.json:
>> 
>>     ##
>>     # @ObjectTypeInfo:
>>     #
>>     # This structure describes a search result from @qom-list-types
>>     #
>>     # @name: the type name found in the search
>>     #
>>     # @abstract: the type is abstract and can't be directly instantiated.
>>     #            Omitted if false. (since 2.10)
>>     #
>>     # @parent: Name of parent type, if any (since 2.10)
>>     #
>>     # Since: 1.1
>>     ##
>>     { 'struct': 'ObjectTypeInfo',
>>       'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str' } }
>> 
>>     ##
>>     # @qom-list-types:
>>     #
>>     # This command will return a list of types given search parameters
>>     #
>>     # @implements: if specified, only return types that implement this type name
>>     #
>>     # @abstract: if true, include abstract types in the results
>>     #
>>     # Returns: a list of @ObjectTypeInfo or an empty list if no results are found
>>     #
>>     # Since: 1.1
>>     ##
>>     { 'command': 'qom-list-types',
>>       'data': { '*implements': 'str', '*abstract': 'bool' },
>>       'returns': [ 'ObjectTypeInfo' ],
>>       'allow-preconfig': true }
>> 
>> Example 1:
>> 
>>     {"execute": "qom-list-types", "arguments": {"abstract": true}}
>> 
>> returns all type names with their parent type names.
>
> Ah, libvirt isn't setting abstract=true when listing types during its
> probing of QEMU capabilities, which is why I didn't see the parents.
>
>
>> > We certainly don't want to issue 'device-list-properties' over and
>> > over for all 147 devices.
>> >
>> > If we just pick one device type, say virtio-blk-pci, and query that
>> > for "acpi-index", then our code is fragile because anyone can make
>> > a QEMU build that compiles-out a specific device. This is fairly
>> > unlikely for virtio devices, but never say never.
>> >
>> > For PCI, i'm tending towards probing for the "acpi-index" property on
>> > both "pci-bridge" and "pcie-root-port", as it seems unlikely that both
>> > of those will be compiled out of QEMU while still retaining PCI support.
>> >
>> > I'm wondering if QEMU maintainers have a view on "best practice" to
>> > probe for device props that are common to specific bus types ?
>> 
>> The obvious
>> 
>>     {"execute": "device-list-properties",
>>      "arguments": {"typename": "pci-device"}}
>> 
>> fails with "Parameter 'typename' expects a non-abstract device type".
>> But its cousin qom-list-properties works:
>> 
>>     {"execute": "qom-list-properties",
>>      "arguments": {"typename": "pci-device"}}
>>     {"return": [
>>      {"name": "type", "type": "string"},
>>      {"name": "parent_bus", "type": "link<bus>"},
>>      {"name": "realized", "type": "bool"},
>>      {"name": "hotplugged", "type": "bool"},
>>      {"name": "hotpluggable", "type": "bool"},
>>      {"name": "failover_pair_id", "type": "str"},
>>      {"name": "romfile", "type": "str"},
>>      {"name": "addr", "description": "Slot and optional function number, example: 06.0 or 06", "type": "int32"},
>>      {"name": "romsize", "type": "uint32"},
>>      {"name": "x-pcie-lnksta-dllla", "description": "on/off", "type": "bool"},
>>      {"name": "rombar", "type": "uint32"},
>>      {"name": "x-pcie-extcap-init", "description": "on/off", "type": "bool"},
>>      {"name": "acpi-index", "type": "uint32"},
>>      {"name": "multifunction", "description": "on/off", "type": "bool"},
>>      {"name": "legacy-addr", "type": "str"}]}
>> 
>> Does this help?
>
> Yes, its good.
>
> Is there any reason to use 'device-list-properties' at all, given that
> 'qom-list-properties' exists and works for all types ?

Good question.

device-list-properties uses module_object_class_by_name(), requires the
result to be a concrete device type, iterates over QOM properties with
object_property_iter_init() / object_property_iter_next(), skipping
properties named "type", "realized", "hotpluggable", "hotplugged",
"parent_bus", and any whose starts with "legacy-".

Paolo, can you remind us why we skip the "legacy-FOO" properties?

qom-list-properties uses object_class_by_name(), requires an object type
(an interface won't do).  If it's abstract, it iterates with
object_class_property_iter_init() / object_property_iter_next(), else
with object_property_iter_init() / object_property_iter_next().  It
doesn't skip properties.

Looks like device-list-properties has become[*] pretty much redundant
*except* for the difference between module_object_class_by_name() and
object_class_by_name().

Gerd, you changed device-list-properties from object_class_by_name() to
module_object_class_by_name() in commit 7ab6e7fcce.  Should
qom-list-properties be changed, too?  If yes, is there any reason to use
object_class_by_name() for looking up user-provided type names in QMP
commands?


[*] "has become" because they used to be more different, if memory
serves.



  reply	other threads:[~2021-04-08 15:01 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-07 13:44 Daniel P. Berrangé
2021-04-08 11:56 ` Markus Armbruster
2021-04-08 12:46   ` Daniel P. Berrangé
2021-04-08 14:59     ` Markus Armbruster [this message]
2021-04-09  6:46       ` Gerd Hoffmann
2021-04-09  9:18         ` Markus Armbruster
2021-04-09  9:41           ` Daniel P. Berrangé
2021-04-09 14:04             ` Markus Armbruster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87o8eo9609.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=imammedo@redhat.com \
    --cc=kraxel@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --subject='Re: QMP introspecting device props common to a bus type' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).