Re: [PATCH v2 1/2] qmp: remove virtio_list, search QOM tree instead

[Jonah Palmer <jonah.palmer@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 14044 bytes --]


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

>>          -        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: 14496 bytes --]