From: Jonah Palmer <jonah.palmer@oracle.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: qemu-devel@nongnu.org, philmd@linaro.org, laurent@vivier.eu,
boris.ostrovsky@oracle.com, alex.bennee@linaro.org,
viresh.kumar@linaro.org, armbru@redhat.com, pbonzini@redhat.com,
berrange@redhat.com, eduardo@habkost.net
Subject: Re: [PATCH v2 1/2] qmp: remove virtio_list, search QOM tree instead
Date: Thu, 27 Jul 2023 07:20:25 -0400 [thread overview]
Message-ID: <2e44596c-337f-1210-3e69-b16d8fc5769a@oracle.com> (raw)
In-Reply-To: <006e8923-d73b-b6be-fccc-a26b3e98e811@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 14457 bytes --]
On 6/27/23 07:23, Jonah Palmer wrote:
>
>
> On 6/26/23 08:16, Michael S. Tsirkin wrote:
>> On Mon, Jun 26, 2023 at 08:08:28AM -0400, Jonah Palmer wrote:
>>> On 6/23/23 01:47, Michael S. Tsirkin wrote:
>>>
>>> On Fri, Jun 09, 2023 at 09:20:39AM -0400, Jonah Palmer wrote:
>>>
>>> The virtio_list duplicates information about virtio devices that already
>>> exist in the QOM composition tree. Instead of creating this list of
>>> realized virtio devices, search the QOM composition tree instead.
>>>
>>> This patch modifies the QMP command qmp_x_query_virtio to instead search
>>> the partial paths of '/machine/peripheral/' &
>>> '/machine/peripheral-anon/' in the QOM composition tree for virtio
>>> devices.
>>>
>>> A device is found to be a valid virtio device if (1) its canonical path
>>> is of 'TYPE_VIRTIO_DEVICE' and (2) the device has been realized.
>>>
>>> [Jonah: In the previous commit I had written that a device is found to
>>> be a valid virtio device if (1) it has a canonical path ending with
>>> 'virtio-backend'.
>>>
>>> The code now determines if it's a virtio device by appending
>>> 'virtio-backend' (if needed) to a given canonical path and then
>>> checking that path to see if the device is of type
>>> 'TYPE_VIRTIO_DEVICE'.
>>>
>>> The patch also instead now checks to make sure it's a virtio device
>>> before attempting to check whether the device is realized or not.]
>>>
>>> Signed-off-by: Jonah Palmer<jonah.palmer@oracle.com>
>>>
>>>
>>> Could one of QMP maintainers comment on this please?
>>>
>>>
>>> ---
>>> hw/virtio/virtio-qmp.c | 128 ++++++++++++++++++++++++++---------------
>>> hw/virtio/virtio-qmp.h | 8 +--
>>> hw/virtio/virtio.c | 6 --
>>> 3 files changed, 82 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
>>> index b5e1835299..e936cc8ce5 100644
>>> --- a/hw/virtio/virtio-qmp.c
>>> +++ b/hw/virtio/virtio-qmp.c
>>> @@ -668,67 +668,101 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>>> VirtioInfoList *qmp_x_query_virtio(Error **errp)
>>> {
>>> VirtioInfoList *list = NULL;
>>> - VirtioInfo *node;
>>> - VirtIODevice *vdev;
>>>
>>> - QTAILQ_FOREACH(vdev, &virtio_list, next) {
>>> - DeviceState *dev = DEVICE(vdev);
>>> - Error *err = NULL;
>>> - QObject *obj = qmp_qom_get(dev->canonical_path, "realized", &err);
>>> -
>>> - if (err == NULL) {
>>> - GString *is_realized = qobject_to_json_pretty(obj, true);
>>> - /* virtio device is NOT realized, remove it from list */
>>> - if (!strncmp(is_realized->str, "false", 4)) {
>>> - QTAILQ_REMOVE(&virtio_list, vdev, next);
>>> - } else {
>>> - node = g_new(VirtioInfo, 1);
>>> - node->path = g_strdup(dev->canonical_path);
>>> - node->name = g_strdup(vdev->name);
>>> - QAPI_LIST_PREPEND(list, node);
>>> + /* Query the QOM composition tree for virtio devices */
>>> + qmp_set_virtio_device_list("/machine/peripheral/", &list);
>>> + qmp_set_virtio_device_list("/machine/peripheral-anon/", &list);
>>>
>>> How sure are we these will forever be the only two places where virtio
>>> can live?
>>>
>>> A virtio device will always be considered a peripheral device, right?
>>> Since peripheral devices are input and/or output devices by definition.
>>>
>>> + if (list == NULL) {
>>> + error_setg(errp, "No virtio devices found");
>>> + return NULL;
>>> + }
>>> + return list;
>>> +}
>>> +
>>> +/* qmp_set_virtio_device_list:
>>> + * @ppath: An incomplete peripheral path to search from.
>>> + * @list: A list of realized virtio devices.
>>> + * Searches a given incomplete peripheral path (e.g. '/machine/peripheral/'
>>> + * or '/machine/peripheral-anon/') for realized virtio devices and adds them
>>> + * to a given list of virtio devices.
>>> + */
>>> +void qmp_set_virtio_device_list(const char *ppath, VirtioInfoList **list)
>>> +{
>>> + ObjectPropertyInfoList *plist;
>>> + VirtioInfoList *node;
>>> + Error *err = NULL;
>>> +
>>> + /* Search an incomplete path for virtio devices */
>>> + plist = qmp_qom_list(ppath, &err);
>>> + if (err == NULL) {
>>> + ObjectPropertyInfoList *start = plist;
>>> + while (plist != NULL) {
>>> + ObjectPropertyInfo *value = plist->value;
>>> + GString *path = g_string_new(ppath);
>>> + g_string_append(path, value->name);
>>> + g_string_append(path, "/virtio-backend");
>>> +
>>> + /* Determine if full path is a realized virtio device */
>>> + VirtIODevice *vdev = qmp_find_virtio_device(path->str);
>>> + if (vdev != NULL) {
>>> + node = g_new0(VirtioInfoList, 1);
>>> + node->value = g_new(VirtioInfo, 1);
>>> + node->value->path = g_strdup(path->str);
>>> + node->value->name = g_strdup(vdev->name);
>>> + QAPI_LIST_PREPEND(*list, node->value);
>>> }
>>> - g_string_free(is_realized, true);
>>> + g_string_free(path, true);
>>> + plist = plist->next;
>>> }
>>> - qobject_unref(obj);
>>> + qapi_free_ObjectPropertyInfoList(start);
>>> }
>>> -
>>> - return list;
>>> }
>>>
>>> VirtIODevice *qmp_find_virtio_device(const char *path)
>>> {
>>> - VirtIODevice *vdev;
>>> -
>>> - QTAILQ_FOREACH(vdev, &virtio_list, next) {
>>> - DeviceState *dev = DEVICE(vdev);
>>> -
>>> - if (strcmp(dev->canonical_path, path) != 0) {
>>> - continue;
>>> + Error *err = NULL;
>>> + char *basename;
>>> +
>>> + /* Append 'virtio-backend' to path if needed */
>>> + basename = g_path_get_basename(path);
>>> + if (strcmp(basename, "virtio-backend")) {
>>> + GString *temp = g_string_new(path);
>>> + char *last = strrchr(path, '/');
>>> + if (g_strcmp0(last, "/")) {
>>> + g_string_append(temp, "/virtio-backend");
>>> + } else {
>>> + g_string_append(temp, "virtio-backend");
>>> }
>>> + path = g_strdup(temp->str);
>>> + g_string_free(temp, true);
>>> + }
>>>
>>> I don't much like the string operations. We should be able to
>>> check object types instead.
>>>
>>>
>>> I don't either but in order for us to check if the object is a
>>> virtio device type, we need to use the device's path ending
>>> with '/virtio-backend'.
>>>
>>> If there's a better method to checking this though, or perhaps
>>> checking a different type, that doesn't involve string
>>> manipulation, then I'm all for it.
>> TYPE_VIRTIO_DEVICE ?
>>
>>
> Please excuse my ignorance, as I'm probably missing something here,
> but how can I verify a device is of TYPE_VIRTIO_DEVICE without using
> a canonical path ending with "/virtio-backend"?
>
> Initially, when the query begins, I use 'qmp_qom_list' to generate a
> list of devices under "/machine/peripheral-anon/" and
> "/machine/peripheral/", e.g:
>
> - "/machine/peripheral-anon/device[0]" (virtio-serial)
> - "/machine/peripheral-anon/device[1]" (virtio-scsi)
>
> Without appending "/virtio-backend" to those paths, which are virtio
> devices, checking if the path is a device of TYPE_VIRTIO_DEVICE would
> throw an error and fail via. the following:
>
> Object *obj = object_resolve_path(path, NULL);
> object_dynamic_cast(obj, TYPE_VIRTIO_DEVICE);
>
> With these path (without "/virtio-backend"), we could check if it's
> of TYPE_VIRTIO_PCI, but not all virtio devices are PCI devices
> (e.g. virtio-mmio).
>
> This can also be seen when qom-get'ing the types of each path:
> --------------------------------------------------------------
> {"execute": "qom-get",
> "arguments": {"path":"/machine/peripheral-anon/device[0]/",
> "property":"type"}}
> {
> "return": "virtio-serial-pci"
> }
>
> {"execute": "qom-get",
> "arguments": {"path":"/machine/peripheral-anon/device[0]/virtio-backend",
> "property":"type"}}
> {
> "return": "virtio-serial-device"
> }
>
> So, in short, I'm not sure how I can check if a canonical path is to a
> device of TYPE_VIRTIO_DEVICE without its path ending with "/virtio-backend".
Any updates on this?
>
>>> - Error *err = NULL;
>>> - QObject *obj = qmp_qom_get(dev->canonical_path, "realized", &err);
>>> - if (err == NULL) {
>>> - GString *is_realized = qobject_to_json_pretty(obj, true);
>>> - /* virtio device is NOT realized, remove it from list */
>>> - if (!strncmp(is_realized->str, "false", 4)) {
>>> - g_string_free(is_realized, true);
>>> - qobject_unref(obj);
>>> - QTAILQ_REMOVE(&virtio_list, vdev, next);
>>> - return NULL;
>>> - }
>>> + /* Verify the canonical path is a virtio device */
>>> + Object *obj = object_resolve_path(path, NULL);
>>> + if (!obj || !object_dynamic_cast(obj, TYPE_VIRTIO_DEVICE)) {
>>> + object_unref(obj);
>>> + return NULL;
>>> + }
>>> +
>>> + /* Verify the virtio device is realized */
>>> + QObject *qobj = qmp_qom_get(path, "realized", &err);
>>> + if (err == NULL) {
>>> + GString *is_realized = qobject_to_json_pretty(qobj, true);
>>> + if (!strncmp(is_realized->str, "false", 4)) {
>>> g_string_free(is_realized, true);
>>> - } else {
>>> - /* virtio device doesn't exist in QOM tree */
>>> - QTAILQ_REMOVE(&virtio_list, vdev, next);
>>> - qobject_unref(obj);
>>> + qobject_unref(qobj);
>>> return NULL;
>>> }
>>> - /* device exists in QOM tree & is realized */
>>> - qobject_unref(obj);
>>> - return vdev;
>>> + g_string_free(is_realized, true);
>>> + } else {
>>> + qobject_unref(qobj);
>>> + return NULL;
>>> }
>>> - return NULL;
>>> + qobject_unref(qobj);
>>> +
>>> + /* Get VirtIODevice object */
>>> + VirtIODevice *vdev = VIRTIO_DEVICE(obj);
>>> + return vdev;
>>> }
>>>
>>> VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
>>> diff --git a/hw/virtio/virtio-qmp.h b/hw/virtio/virtio-qmp.h
>>> index 8af5f5e65a..4b2b7875b4 100644
>>> --- a/hw/virtio/virtio-qmp.h
>>> +++ b/hw/virtio/virtio-qmp.h
>>> @@ -15,13 +15,7 @@
>>> #include "hw/virtio/virtio.h"
>>> #include "hw/virtio/vhost.h"
>>>
>>> -#include "qemu/queue.h"
>>> -
>>> -typedef QTAILQ_HEAD(QmpVirtIODeviceList, VirtIODevice) QmpVirtIODeviceList;
>>> -
>>> -/* QAPI list of realized VirtIODevices */
>>> -extern QmpVirtIODeviceList virtio_list;
>>> -
>>> +void qmp_set_virtio_device_list(const char *ppath, VirtioInfoList **list);
>>> VirtIODevice *qmp_find_virtio_device(const char *path);
>>> VirtioDeviceStatus *qmp_decode_status(uint8_t bitmap);
>>> VhostDeviceProtocols *qmp_decode_protocols(uint64_t bitmap);
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 295a603e58..83c5db3d26 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -45,8 +45,6 @@
>>> #include "standard-headers/linux/virtio_mem.h"
>>> #include "standard-headers/linux/virtio_vsock.h"
>>>
>>> -QmpVirtIODeviceList virtio_list;
>>> -
>>> /*
>>> * Maximum size of virtio device config space
>>> */
>>> @@ -3616,7 +3614,6 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
>>> vdev->listener.commit = virtio_memory_listener_commit;
>>> vdev->listener.name = "virtio";
>>> memory_listener_register(&vdev->listener, vdev->dma_as);
>>> - QTAILQ_INSERT_TAIL(&virtio_list, vdev, next);
>>> }
>>>
>>> static void virtio_device_unrealize(DeviceState *dev)
>>> @@ -3631,7 +3628,6 @@ static void virtio_device_unrealize(DeviceState *dev)
>>> vdc->unrealize(dev);
>>> }
>>>
>>> - QTAILQ_REMOVE(&virtio_list, vdev, next);
>>> g_free(vdev->bus_name);
>>> vdev->bus_name = NULL;
>>> }
>>> @@ -3805,8 +3801,6 @@ static void virtio_device_class_init(ObjectClass *klass, void *data)
>>> vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
>>>
>>> vdc->legacy_features |= VIRTIO_LEGACY_FEATURES;
>>> -
>>> - QTAILQ_INIT(&virtio_list);
>>> }
>>>
>>> bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
>>> --
>>> 2.39.3
>>>
[-- Attachment #2: Type: text/html, Size: 14881 bytes --]
next prev parent reply other threads:[~2023-07-27 11:28 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-09 13:20 [PATCH v2 0/2] qmp: Remove virtio_list & update virtio introspection Jonah Palmer
2023-06-09 13:20 ` [PATCH v2 1/2] qmp: remove virtio_list, search QOM tree instead Jonah Palmer
2023-06-23 5:47 ` Michael S. Tsirkin
2023-06-26 12:08 ` Jonah Palmer
2023-06-26 12:16 ` Michael S. Tsirkin
2023-06-27 11:23 ` Jonah Palmer
2023-07-27 11:20 ` Jonah Palmer [this message]
2023-07-27 11:46 ` Daniel P. Berrangé
2023-07-27 15:48 ` Jonah Palmer
2023-06-09 13:20 ` [PATCH v2 2/2] qmp: update virtio feature maps, vhost-user-gpio instrospection Jonah Palmer
2023-06-23 5:43 ` Michael S. Tsirkin
2023-06-26 12:08 ` Jonah Palmer
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=2e44596c-337f-1210-3e69-b16d8fc5769a@oracle.com \
--to=jonah.palmer@oracle.com \
--cc=alex.bennee@linaro.org \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=boris.ostrovsky@oracle.com \
--cc=eduardo@habkost.net \
--cc=laurent@vivier.eu \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=viresh.kumar@linaro.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).