[PATCH v2 0/2] qmp: Remove virtio_list & update virtio introspection

[PATCH v2 0/2] qmp: Remove virtio_list & update virtio introspection

[Jonah Palmer]

These patches update a few things related to virtio introspection via.
QMP/HMP commands.

1. Remove 'virtio_list' and instead query the QOM composition tree to
find any active & realized virtio devices.

The 'virtio_list' was duplicating information about virtio devices that
was already available in the QOM composition tree, so there was no need
to keep this list.

2. Add new transport, protocol, and device features as well as support
to introspect vhost-user-gpio devices.

Vhost-user-gpio previously had no support for introspection. Support for
introspecting its vhost-user device is now available in these patches.

New virtio transport feature:
-----------------------------
 - VIRTIO_F_RING_RESET

New vhost-user protocol feature:
--------------------------------
 - VHOST_USER_PROTOCOL_F_STATUS

New virtio device features:
---------------------------
virtio-blk:
 - VIRTIO_BLK_F_SECURE_ERASE

virtio-net:
 - VIRTIO_NET_F_NOTF_COAL
 - VIRTIO_NET_F_GUEST_USO4
 - VIRTIO_NET_F_GUEST_USO6
 - VIRTIO_NET_F_HOST_USO

virtio/vhost-user-gpio:
 - VIRTIO_GPIO_F_IRQ
 - VHOST_F_LOG_ALL
 - VHOST_USER_F_PROTOCOL_FEATURES

virtio-bt:
 - VIRTIO_BT_F_VND_HCI
 - VIRTIO_BT_F_MSFT_EXT
 - VIRTIO_BT_F_AOSP_EXT
 - VIRTIO_BT_F_CONFIG_V2

virtio-scmi:
 - VIRTIO_SCMI_F_P2A_CHANNELS
 - VIRTIO_SCMI_F_SHARED_MEMORY

v2: verify virtio devices via. 'TYPE_VIRTIO_DEVICES'
    verify path is a virtio device before checking if it's realized
    remove 'VIRTIO_BLK_F_ZONED' update (already exists)
    add cover letter

Jonah Palmer (2):
  qmp: remove virtio_list, search QOM tree instead
  qmp: update virtio feature maps, vhost-user-gpio instrospection

 hw/virtio/vhost-user-gpio.c |   7 ++
 hw/virtio/virtio-qmp.c      | 207 +++++++++++++++++++++++++++---------
 hw/virtio/virtio-qmp.h      |   8 +-
 hw/virtio/virtio.c          |   6 --
 4 files changed, 165 insertions(+), 63 deletions(-)

-- 
2.39.3

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

[Jonah Palmer]

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>
---
 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);
+    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);
+    }
 
-        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

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

[Michael S. Tsirkin]

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?

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


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

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

[Jonah Palmer]

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

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.

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

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

[Michael S. Tsirkin]

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 ?


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

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

[Jonah Palmer]

[-- 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 --]

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

[Jonah Palmer]

[-- 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 --]

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

[Daniel P. Berrangé]

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>
> ---
>  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);
> +    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;
>  }

This is all way too complicated. AFAICT, it shouldn't require
much more than this:

static int one_child(Object *child, void *opaque)
{
    VirtioInfoList **devs = opaque;
    VirtIODevice *dev = object_dynamic_cast(child, TYPE_VIRTIO_DEVICE);
    if (dev != NULL) {
        VirtioInfo *info =g_new(VirtioInfo, 1);
        info->path = g_strdup(path->str);
        info->name = g_strdup(vdev->name);
        QAPI_LIST_PREPEND(*devs, info);
    }
     
    return 0;
}


VirtioInfoList *qmp_x_query_virtio(Error **errp)
{
   VirtioInfoList *devs = NULL;
   object_child_foreach_recursive(object_get_root(),
                                  one_child,
                                  &devs);
    if (devs == NULL) {
        error_setg(errp, "No virtio devices found");
        return NULL;
    }
    return devs;
}





>  
>  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);
> +    }
>  
> -        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
> 

With 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 :|

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

[Jonah Palmer]

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

On 7/27/23 07:46, Daniel P. Berrangé 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>
>> ---
>>   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);
>> +    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;
>>   }
> This is all way too complicated. AFAICT, it shouldn't require
> much more than this:
>
> static int one_child(Object *child, void *opaque)
> {
>      VirtioInfoList **devs = opaque;
>      VirtIODevice *dev = object_dynamic_cast(child, TYPE_VIRTIO_DEVICE);
>      if (dev != NULL) {
>          VirtioInfo *info =g_new(VirtioInfo, 1);
>          info->path = g_strdup(path->str);
>          info->name = g_strdup(vdev->name);
>          QAPI_LIST_PREPEND(*devs, info);
>      }
>       
>      return 0;
> }
>
>
> VirtioInfoList *qmp_x_query_virtio(Error **errp)
> {
>     VirtioInfoList *devs = NULL;
>     object_child_foreach_recursive(object_get_root(),
>                                    one_child,
>                                    &devs);
>      if (devs == NULL) {
>          error_setg(errp, "No virtio devices found");
>          return NULL;
>      }
>      return devs;
> }
>
Ah okay, I will give this a try. Thanks!

Jonah

>
>
>
>>   
>>   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);
>> +    }
>>   
>> -        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
>>
> With regards,
> Daniel

[-- Attachment #2: Type: text/html, Size: 11190 bytes --]

[PATCH v2 2/2] qmp: update virtio feature maps, vhost-user-gpio instrospection

[Jonah Palmer]

Add new virtio transport feature to transport feature map:
 - VIRTIO_F_RING_RESET

Add new vhost-user protocol feature to vhost-user protocol feature map
and enumeration:
 - VHOST_USER_PROTOCOL_F_STATUS

Add new virtio device features for several virtio devices to their
respective feature mappings:

virtio-blk:
 - VIRTIO_BLK_F_SECURE_ERASE

virtio-net:
 - VIRTIO_NET_F_NOTF_COAL
 - VIRTIO_NET_F_GUEST_USO4
 - VIRTIO_NET_F_GUEST_USO6
 - VIRTIO_NET_F_HOST_USO

virtio/vhost-user-gpio:
 - VIRTIO_GPIO_F_IRQ
 - VHOST_F_LOG_ALL
 - VHOST_USER_F_PROTOCOL_FEATURES

virtio-bt:
 - VIRTIO_BT_F_VND_HCI
 - VIRTIO_BT_F_MSFT_EXT
 - VIRTIO_BT_F_AOSP_EXT
 - VIRTIO_BT_F_CONFIG_V2

virtio-scmi:
 - VIRTIO_SCMI_F_P2A_CHANNELS
 - VIRTIO_SCMI_F_SHARED_MEMORY

Add support for introspection on vhost-user-gpio devices.

Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/vhost-user-gpio.c |  7 ++++
 hw/virtio/virtio-qmp.c      | 79 +++++++++++++++++++++++++++++++++++--
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index d6927b610a..e88ca5370f 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -205,6 +205,12 @@ static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
     vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
 }
 
+static struct vhost_dev *vu_gpio_get_vhost(VirtIODevice *vdev)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    return &gpio->vhost_dev;
+}
+
 static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
 {
     virtio_delete_queue(gpio->command_vq);
@@ -413,6 +419,7 @@ static void vu_gpio_class_init(ObjectClass *klass, void *data)
     vdc->get_config = vu_gpio_get_config;
     vdc->set_status = vu_gpio_set_status;
     vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
+    vdc->get_vhost = vu_gpio_get_vhost;
 }
 
 static const TypeInfo vu_gpio_info = {
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index e936cc8ce5..140c420d87 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -53,6 +53,7 @@ enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
     VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
     VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
+    VHOST_USER_PROTOCOL_F_STATUS = 16,
     VHOST_USER_PROTOCOL_F_MAX
 };
 
@@ -79,6 +80,8 @@ static const qmp_virtio_feature_map_t virtio_transport_map[] = {
             "VIRTIO_F_ORDER_PLATFORM: Memory accesses ordered by platform"),
     FEATURE_ENTRY(VIRTIO_F_SR_IOV, \
             "VIRTIO_F_SR_IOV: Device supports single root I/O virtualization"),
+    FEATURE_ENTRY(VIRTIO_F_RING_RESET, \
+            "VIRTIO_F_RING_RESET: Driver can reset individual VQs"),
     /* Virtio ring transport features */
     FEATURE_ENTRY(VIRTIO_RING_F_INDIRECT_DESC, \
             "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported"),
@@ -134,6 +137,9 @@ static const qmp_virtio_feature_map_t vhost_user_protocol_map[] = {
     FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, \
             "VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS: Configuration for "
             "memory slots supported"),
+    FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_STATUS, \
+            "VHOST_USER_PROTOCOL_F_STATUS: Querying and notifying back-end "
+            "device statuses supported"),
     { -1, "" }
 };
 
@@ -176,6 +182,8 @@ static const qmp_virtio_feature_map_t virtio_blk_feature_map[] = {
             "VIRTIO_BLK_F_DISCARD: Discard command supported"),
     FEATURE_ENTRY(VIRTIO_BLK_F_WRITE_ZEROES, \
             "VIRTIO_BLK_F_WRITE_ZEROES: Write zeroes command supported"),
+    FEATURE_ENTRY(VIRTIO_BLK_F_SECURE_ERASE, \
+            "VIRTIO_BLK_F_SECURE_ERASE: Secure erase supported"),
     FEATURE_ENTRY(VIRTIO_BLK_F_ZONED, \
             "VIRTIO_BLK_F_ZONED: Zoned block devices"),
 #ifndef VIRTIO_BLK_NO_LEGACY
@@ -299,6 +307,14 @@ static const qmp_virtio_feature_map_t virtio_net_feature_map[] = {
     FEATURE_ENTRY(VIRTIO_NET_F_CTRL_MAC_ADDR, \
             "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control "
             "channel"),
+    FEATURE_ENTRY(VIRTIO_NET_F_NOTF_COAL, \
+            "VIRTIO_NET_F_NOTF_COAL: Device supports coalescing notifications"),
+    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO4, \
+            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv4"),
+    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO6, \
+            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv6"),
+    FEATURE_ENTRY(VIRTIO_NET_F_HOST_USO, \
+            "VIRTIO_NET_F_HOST_USO: Device can receive USO"),
     FEATURE_ENTRY(VIRTIO_NET_F_HASH_REPORT, \
             "VIRTIO_NET_F_HASH_REPORT: Hash reporting supported"),
     FEATURE_ENTRY(VIRTIO_NET_F_RSS, \
@@ -469,6 +485,48 @@ static const qmp_virtio_feature_map_t virtio_rng_feature_map[] = {
 };
 #endif
 
+/* virtio/vhost-gpio features mapping */
+#ifdef CONFIG_VIRTIO_GPIO
+static const qmp_virtio_feature_map_t virtio_gpio_feature_map[] = {
+    FEATURE_ENTRY(VIRTIO_GPIO_F_IRQ, \
+            "VIRTIO_GPIO_F_IRQ: Device supports interrupts on GPIO lines"),
+    FEATURE_ENTRY(VHOST_F_LOG_ALL, \
+            "VHOST_F_LOG_ALL: Logging write descriptors supported"),
+    FEATURE_ENTRY(VHOST_USER_F_PROTOCOL_FEATURES, \
+            "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features "
+            "negotiation supported"),
+    { -1, "" }
+};
+#endif
+
+/* virtio-bluetooth features mapping */
+#ifdef CONFIG_VIRTIO_BT
+static const qmp_virtio_feature_map_t virtio_bt_feature_map[] = {
+    FEATURE_ENTRY(VIRTIO_BT_F_VND_HCI, \
+            "VIRTIO_BT_F_VND_HCI: Vendor command supported"),
+    FEATURE_ENTRY(VIRTIO_BT_F_MSFT_EXT, \
+            "VIRTIO_BT_F_MSFT_EXT: MSFT vendor supported"),
+    FEATURE_ENTRY(VIRTIO_BT_F_AOSP_EXT, \
+            "VIRTIO_BT_F_AOSP_EXT: AOSP vendor supported"),
+    FEATURE_ENTRY(VIRTIO_BT_F_CONFIG_V2, \
+            "VIRTIO_BT_F_CONFIG_V2: Using v2 configuration"),
+    { -1, "" }
+};
+#endif
+
+/* virtio-scmi features mapping */
+#ifdef CONFIG_VIRTIO_SCMI
+static const qmp_virtio_feature_map_t virtio_scmi_feature_map[] = {
+    FEATURE_ENTRY(VIRTIO_SCMI_F_P2A_CHANNELS, \
+            "VIRTIO_SCMI_F_P2A_CHANNELS: SCMI notifications or delayed "
+            "responses implemented"),
+    FEATURE_ENTRY(VIRTIO_SCMI_F_SHARED_MEMORY, \
+            "VIRTIO_SCMI_F_SHARED_MEMORY: SCMI shared memory region statistics "
+            "implemented"),
+    { -1, "" }
+};
+#endif
+
 #define CONVERT_FEATURES(type, map, is_status, bitmap)   \
     ({                                                   \
         type *list = NULL;                               \
@@ -625,6 +683,24 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
         features->dev_features =
             CONVERT_FEATURES(strList, virtio_rng_feature_map, 0, bitmap);
         break;
+#endif
+#ifdef CONFIG_VIRTIO_GPIO
+    case VIRTIO_ID_GPIO:
+        features->dev_features =
+            CONVERT_FEATURES(strList, virtio_gpio_feature_map, 0, bitmap);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_BT
+    case VIRTIO_ID_BT:
+        features->dev_features =
+            CONVERT_FEATURES(strList, virtio_bt_feature_map, 0, bitmap);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_SCMI
+    case VIRTIO_ID_SCMI:
+        features->dev_features =
+            CONVERT_FEATURES(strList, virtio_scmi_feature_map, 0, bitmap);
+        break;
 #endif
     /* No features */
     case VIRTIO_ID_9P:
@@ -640,18 +716,15 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
     case VIRTIO_ID_SIGNAL_DIST:
     case VIRTIO_ID_PSTORE:
     case VIRTIO_ID_SOUND:
-    case VIRTIO_ID_BT:
     case VIRTIO_ID_RPMB:
     case VIRTIO_ID_VIDEO_ENCODER:
     case VIRTIO_ID_VIDEO_DECODER:
-    case VIRTIO_ID_SCMI:
     case VIRTIO_ID_NITRO_SEC_MOD:
     case VIRTIO_ID_WATCHDOG:
     case VIRTIO_ID_CAN:
     case VIRTIO_ID_DMABUF:
     case VIRTIO_ID_PARAM_SERV:
     case VIRTIO_ID_AUDIO_POLICY:
-    case VIRTIO_ID_GPIO:
         break;
     default:
         g_assert_not_reached();
-- 
2.39.3

Re: [PATCH v2 2/2] qmp: update virtio feature maps, vhost-user-gpio instrospection

[Michael S. Tsirkin]

On Fri, Jun 09, 2023 at 09:20:40AM -0400, Jonah Palmer wrote:
> Add new virtio transport feature to transport feature map:
>  - VIRTIO_F_RING_RESET
> 
> Add new vhost-user protocol feature to vhost-user protocol feature map
> and enumeration:
>  - VHOST_USER_PROTOCOL_F_STATUS
> 
> Add new virtio device features for several virtio devices to their
> respective feature mappings:
> 
> virtio-blk:
>  - VIRTIO_BLK_F_SECURE_ERASE
> 
> virtio-net:
>  - VIRTIO_NET_F_NOTF_COAL
>  - VIRTIO_NET_F_GUEST_USO4
>  - VIRTIO_NET_F_GUEST_USO6
>  - VIRTIO_NET_F_HOST_USO
> 
> virtio/vhost-user-gpio:
>  - VIRTIO_GPIO_F_IRQ
>  - VHOST_F_LOG_ALL
>  - VHOST_USER_F_PROTOCOL_FEATURES
> 
> virtio-bt:
>  - VIRTIO_BT_F_VND_HCI
>  - VIRTIO_BT_F_MSFT_EXT
>  - VIRTIO_BT_F_AOSP_EXT
>  - VIRTIO_BT_F_CONFIG_V2
> 
> virtio-scmi:
>  - VIRTIO_SCMI_F_P2A_CHANNELS
>  - VIRTIO_SCMI_F_SHARED_MEMORY
> 
> Add support for introspection on vhost-user-gpio devices.
> 
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>

Thanks for the patch! Some comments:


> ---
>  hw/virtio/vhost-user-gpio.c |  7 ++++
>  hw/virtio/virtio-qmp.c      | 79 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 83 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index d6927b610a..e88ca5370f 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -205,6 +205,12 @@ static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
>      vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
>  }
>  
> +static struct vhost_dev *vu_gpio_get_vhost(VirtIODevice *vdev)
> +{
> +    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
> +    return &gpio->vhost_dev;
> +}
> +
>  static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
>  {
>      virtio_delete_queue(gpio->command_vq);
> @@ -413,6 +419,7 @@ static void vu_gpio_class_init(ObjectClass *klass, void *data)
>      vdc->get_config = vu_gpio_get_config;
>      vdc->set_status = vu_gpio_set_status;
>      vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
> +    vdc->get_vhost = vu_gpio_get_vhost;
>  }
>  
>  static const TypeInfo vu_gpio_info = {
> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
> index e936cc8ce5..140c420d87 100644
> --- a/hw/virtio/virtio-qmp.c
> +++ b/hw/virtio/virtio-qmp.c
> @@ -53,6 +53,7 @@ enum VhostUserProtocolFeature {
>      VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
>      VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
>      VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
> +    VHOST_USER_PROTOCOL_F_STATUS = 16,
>      VHOST_USER_PROTOCOL_F_MAX
>  };
>

OMG I just realized that by now we have accumulated each value
in 4 places! This is really badly asking to be moved
to a header. Not sure what to do about the document yet
but that will at least get us down to two.
  
> @@ -79,6 +80,8 @@ static const qmp_virtio_feature_map_t virtio_transport_map[] = {
>              "VIRTIO_F_ORDER_PLATFORM: Memory accesses ordered by platform"),
>      FEATURE_ENTRY(VIRTIO_F_SR_IOV, \
>              "VIRTIO_F_SR_IOV: Device supports single root I/O virtualization"),
> +    FEATURE_ENTRY(VIRTIO_F_RING_RESET, \
> +            "VIRTIO_F_RING_RESET: Driver can reset individual VQs"),
>      /* Virtio ring transport features */
>      FEATURE_ENTRY(VIRTIO_RING_F_INDIRECT_DESC, \
>              "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported"),
> @@ -134,6 +137,9 @@ static const qmp_virtio_feature_map_t vhost_user_protocol_map[] = {
>      FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, \
>              "VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS: Configuration for "
>              "memory slots supported"),
> +    FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_STATUS, \
> +            "VHOST_USER_PROTOCOL_F_STATUS: Querying and notifying back-end "
> +            "device statuses supported"),

status - there's only one per device

>      { -1, "" }
>  };
>  
> @@ -176,6 +182,8 @@ static const qmp_virtio_feature_map_t virtio_blk_feature_map[] = {
>              "VIRTIO_BLK_F_DISCARD: Discard command supported"),
>      FEATURE_ENTRY(VIRTIO_BLK_F_WRITE_ZEROES, \
>              "VIRTIO_BLK_F_WRITE_ZEROES: Write zeroes command supported"),
> +    FEATURE_ENTRY(VIRTIO_BLK_F_SECURE_ERASE, \
> +            "VIRTIO_BLK_F_SECURE_ERASE: Secure erase supported"),
>      FEATURE_ENTRY(VIRTIO_BLK_F_ZONED, \
>              "VIRTIO_BLK_F_ZONED: Zoned block devices"),
>  #ifndef VIRTIO_BLK_NO_LEGACY

> @@ -299,6 +307,14 @@ static const qmp_virtio_feature_map_t virtio_net_feature_map[] = {
>      FEATURE_ENTRY(VIRTIO_NET_F_CTRL_MAC_ADDR, \
>              "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control "
>              "channel"),
> +    FEATURE_ENTRY(VIRTIO_NET_F_NOTF_COAL, \
> +            "VIRTIO_NET_F_NOTF_COAL: Device supports coalescing notifications"),
> +    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO4, \
> +            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv4"),
> +    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO6, \
> +            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv6"),
> +    FEATURE_ENTRY(VIRTIO_NET_F_HOST_USO, \
> +            "VIRTIO_NET_F_HOST_USO: Device can receive USO"),
>      FEATURE_ENTRY(VIRTIO_NET_F_HASH_REPORT, \
>              "VIRTIO_NET_F_HASH_REPORT: Hash reporting supported"),
>      FEATURE_ENTRY(VIRTIO_NET_F_RSS, \
> @@ -469,6 +485,48 @@ static const qmp_virtio_feature_map_t virtio_rng_feature_map[] = {
>  };
>  #endif
>  
> +/* virtio/vhost-gpio features mapping */
> +#ifdef CONFIG_VIRTIO_GPIO

Where's this coming from?

> +static const qmp_virtio_feature_map_t virtio_gpio_feature_map[] = {
> +    FEATURE_ENTRY(VIRTIO_GPIO_F_IRQ, \
> +            "VIRTIO_GPIO_F_IRQ: Device supports interrupts on GPIO lines"),
> +    FEATURE_ENTRY(VHOST_F_LOG_ALL, \
> +            "VHOST_F_LOG_ALL: Logging write descriptors supported"),
> +    FEATURE_ENTRY(VHOST_USER_F_PROTOCOL_FEATURES, \
> +            "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features "
> +            "negotiation supported"),
> +    { -1, "" }
> +};
> +#endif
> +
> +/* virtio-bluetooth features mapping */
> +#ifdef CONFIG_VIRTIO_BT

Where's this coming from?


> +static const qmp_virtio_feature_map_t virtio_bt_feature_map[] = {
> +    FEATURE_ENTRY(VIRTIO_BT_F_VND_HCI, \
> +            "VIRTIO_BT_F_VND_HCI: Vendor command supported"),
> +    FEATURE_ENTRY(VIRTIO_BT_F_MSFT_EXT, \
> +            "VIRTIO_BT_F_MSFT_EXT: MSFT vendor supported"),
> +    FEATURE_ENTRY(VIRTIO_BT_F_AOSP_EXT, \
> +            "VIRTIO_BT_F_AOSP_EXT: AOSP vendor supported"),
> +    FEATURE_ENTRY(VIRTIO_BT_F_CONFIG_V2, \
> +            "VIRTIO_BT_F_CONFIG_V2: Using v2 configuration"),
> +    { -1, "" }
> +};
> +#endif
> +
> +/* virtio-scmi features mapping */
> +#ifdef CONFIG_VIRTIO_SCMI

Where's this coming from?

> +static const qmp_virtio_feature_map_t virtio_scmi_feature_map[] = {
> +    FEATURE_ENTRY(VIRTIO_SCMI_F_P2A_CHANNELS, \
> +            "VIRTIO_SCMI_F_P2A_CHANNELS: SCMI notifications or delayed "
> +            "responses implemented"),
> +    FEATURE_ENTRY(VIRTIO_SCMI_F_SHARED_MEMORY, \
> +            "VIRTIO_SCMI_F_SHARED_MEMORY: SCMI shared memory region statistics "
> +            "implemented"),
> +    { -1, "" }
> +};
> +#endif
> +
>  #define CONVERT_FEATURES(type, map, is_status, bitmap)   \
>      ({                                                   \
>          type *list = NULL;                               \
> @@ -625,6 +683,24 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>          features->dev_features =
>              CONVERT_FEATURES(strList, virtio_rng_feature_map, 0, bitmap);
>          break;
> +#endif
> +#ifdef CONFIG_VIRTIO_GPIO
> +    case VIRTIO_ID_GPIO:
> +        features->dev_features =
> +            CONVERT_FEATURES(strList, virtio_gpio_feature_map, 0, bitmap);
> +        break;
> +#endif
> +#ifdef CONFIG_VIRTIO_BT
> +    case VIRTIO_ID_BT:
> +        features->dev_features =
> +            CONVERT_FEATURES(strList, virtio_bt_feature_map, 0, bitmap);
> +        break;
> +#endif
> +#ifdef CONFIG_VIRTIO_SCMI
> +    case VIRTIO_ID_SCMI:
> +        features->dev_features =
> +            CONVERT_FEATURES(strList, virtio_scmi_feature_map, 0, bitmap);
> +        break;
>  #endif
>      /* No features */
>      case VIRTIO_ID_9P:
> @@ -640,18 +716,15 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>      case VIRTIO_ID_SIGNAL_DIST:
>      case VIRTIO_ID_PSTORE:
>      case VIRTIO_ID_SOUND:
> -    case VIRTIO_ID_BT:
>      case VIRTIO_ID_RPMB:
>      case VIRTIO_ID_VIDEO_ENCODER:
>      case VIRTIO_ID_VIDEO_DECODER:
> -    case VIRTIO_ID_SCMI:
>      case VIRTIO_ID_NITRO_SEC_MOD:
>      case VIRTIO_ID_WATCHDOG:
>      case VIRTIO_ID_CAN:
>      case VIRTIO_ID_DMABUF:
>      case VIRTIO_ID_PARAM_SERV:
>      case VIRTIO_ID_AUDIO_POLICY:
> -    case VIRTIO_ID_GPIO:
>          break;
>      default:
>          g_assert_not_reached();
> -- 
> 2.39.3

Re: [PATCH v2 2/2] qmp: update virtio feature maps, vhost-user-gpio instrospection

[Jonah Palmer]

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

On 6/23/23 01:43, Michael S. Tsirkin wrote:

> On Fri, Jun 09, 2023 at 09:20:40AM -0400, Jonah Palmer wrote:
>> Add new virtio transport feature to transport feature map:
>>   - VIRTIO_F_RING_RESET
>>
>> Add new vhost-user protocol feature to vhost-user protocol feature map
>> and enumeration:
>>   - VHOST_USER_PROTOCOL_F_STATUS
>>
>> Add new virtio device features for several virtio devices to their
>> respective feature mappings:
>>
>> virtio-blk:
>>   - VIRTIO_BLK_F_SECURE_ERASE
>>
>> virtio-net:
>>   - VIRTIO_NET_F_NOTF_COAL
>>   - VIRTIO_NET_F_GUEST_USO4
>>   - VIRTIO_NET_F_GUEST_USO6
>>   - VIRTIO_NET_F_HOST_USO
>>
>> virtio/vhost-user-gpio:
>>   - VIRTIO_GPIO_F_IRQ
>>   - VHOST_F_LOG_ALL
>>   - VHOST_USER_F_PROTOCOL_FEATURES
>>
>> virtio-bt:
>>   - VIRTIO_BT_F_VND_HCI
>>   - VIRTIO_BT_F_MSFT_EXT
>>   - VIRTIO_BT_F_AOSP_EXT
>>   - VIRTIO_BT_F_CONFIG_V2
>>
>> virtio-scmi:
>>   - VIRTIO_SCMI_F_P2A_CHANNELS
>>   - VIRTIO_SCMI_F_SHARED_MEMORY
>>
>> Add support for introspection on vhost-user-gpio devices.
>>
>> Signed-off-by: Jonah Palmer<jonah.palmer@oracle.com>
> Thanks for the patch! Some comments:
>
>
>> ---
>>   hw/virtio/vhost-user-gpio.c |  7 ++++
>>   hw/virtio/virtio-qmp.c      | 79 +++++++++++++++++++++++++++++++++++--
>>   2 files changed, 83 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>> index d6927b610a..e88ca5370f 100644
>> --- a/hw/virtio/vhost-user-gpio.c
>> +++ b/hw/virtio/vhost-user-gpio.c
>> @@ -205,6 +205,12 @@ static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
>>       vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
>>   }
>>   
>> +static struct vhost_dev *vu_gpio_get_vhost(VirtIODevice *vdev)
>> +{
>> +    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
>> +    return &gpio->vhost_dev;
>> +}
>> +
>>   static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
>>   {
>>       virtio_delete_queue(gpio->command_vq);
>> @@ -413,6 +419,7 @@ static void vu_gpio_class_init(ObjectClass *klass, void *data)
>>       vdc->get_config = vu_gpio_get_config;
>>       vdc->set_status = vu_gpio_set_status;
>>       vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
>> +    vdc->get_vhost = vu_gpio_get_vhost;
>>   }
>>   
>>   static const TypeInfo vu_gpio_info = {
>> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
>> index e936cc8ce5..140c420d87 100644
>> --- a/hw/virtio/virtio-qmp.c
>> +++ b/hw/virtio/virtio-qmp.c
>> @@ -53,6 +53,7 @@ enum VhostUserProtocolFeature {
>>       VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
>>       VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
>>       VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
>> +    VHOST_USER_PROTOCOL_F_STATUS = 16,
>>       VHOST_USER_PROTOCOL_F_MAX
>>   };
>>
> OMG I just realized that by now we have accumulated each value
> in 4 places! This is really badly asking to be moved
> to a header. Not sure what to do about the document yet
> but that will at least get us down to two.
>    

I can add this in another patch as a part of this series.

>> @@ -79,6 +80,8 @@ static const qmp_virtio_feature_map_t virtio_transport_map[] = {
>>               "VIRTIO_F_ORDER_PLATFORM: Memory accesses ordered by platform"),
>>       FEATURE_ENTRY(VIRTIO_F_SR_IOV, \
>>               "VIRTIO_F_SR_IOV: Device supports single root I/O virtualization"),
>> +    FEATURE_ENTRY(VIRTIO_F_RING_RESET, \
>> +            "VIRTIO_F_RING_RESET: Driver can reset individual VQs"),
>>       /* Virtio ring transport features */
>>       FEATURE_ENTRY(VIRTIO_RING_F_INDIRECT_DESC, \
>>               "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported"),
>> @@ -134,6 +137,9 @@ static const qmp_virtio_feature_map_t vhost_user_protocol_map[] = {
>>       FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, \
>>               "VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS: Configuration for "
>>               "memory slots supported"),
>> +    FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_STATUS, \
>> +            "VHOST_USER_PROTOCOL_F_STATUS: Querying and notifying back-end "
>> +            "device statuses supported"),
> status - there's only one per device

Will change, thanks!

>>       { -1, "" }
>>   };
>>   
>> @@ -176,6 +182,8 @@ static const qmp_virtio_feature_map_t virtio_blk_feature_map[] = {
>>               "VIRTIO_BLK_F_DISCARD: Discard command supported"),
>>       FEATURE_ENTRY(VIRTIO_BLK_F_WRITE_ZEROES, \
>>               "VIRTIO_BLK_F_WRITE_ZEROES: Write zeroes command supported"),
>> +    FEATURE_ENTRY(VIRTIO_BLK_F_SECURE_ERASE, \
>> +            "VIRTIO_BLK_F_SECURE_ERASE: Secure erase supported"),
>>       FEATURE_ENTRY(VIRTIO_BLK_F_ZONED, \
>>               "VIRTIO_BLK_F_ZONED: Zoned block devices"),
>>   #ifndef VIRTIO_BLK_NO_LEGACY
>> @@ -299,6 +307,14 @@ static const qmp_virtio_feature_map_t virtio_net_feature_map[] = {
>>       FEATURE_ENTRY(VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>               "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control "
>>               "channel"),
>> +    FEATURE_ENTRY(VIRTIO_NET_F_NOTF_COAL, \
>> +            "VIRTIO_NET_F_NOTF_COAL: Device supports coalescing notifications"),
>> +    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO4, \
>> +            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv4"),
>> +    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO6, \
>> +            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv6"),
>> +    FEATURE_ENTRY(VIRTIO_NET_F_HOST_USO, \
>> +            "VIRTIO_NET_F_HOST_USO: Device can receive USO"),
>>       FEATURE_ENTRY(VIRTIO_NET_F_HASH_REPORT, \
>>               "VIRTIO_NET_F_HASH_REPORT: Hash reporting supported"),
>>       FEATURE_ENTRY(VIRTIO_NET_F_RSS, \
>> @@ -469,6 +485,48 @@ static const qmp_virtio_feature_map_t virtio_rng_feature_map[] = {
>>   };
>>   #endif
>>   
>> +/* virtio/vhost-gpio features mapping */
>> +#ifdef CONFIG_VIRTIO_GPIO
> Where's this coming from?
>
Ah, this should probably be CONFIG_VHOST_USER_GPIO instead I believe.

As for the other two...

>> +static const qmp_virtio_feature_map_t virtio_gpio_feature_map[] = {
>> +    FEATURE_ENTRY(VIRTIO_GPIO_F_IRQ, \
>> +            "VIRTIO_GPIO_F_IRQ: Device supports interrupts on GPIO lines"),
>> +    FEATURE_ENTRY(VHOST_F_LOG_ALL, \
>> +            "VHOST_F_LOG_ALL: Logging write descriptors supported"),
>> +    FEATURE_ENTRY(VHOST_USER_F_PROTOCOL_FEATURES, \
>> +            "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features "
>> +            "negotiation supported"),
>> +    { -1, "" }
>> +};
>> +#endif
>> +
>> +/* virtio-bluetooth features mapping */
>> +#ifdef CONFIG_VIRTIO_BT
> Where's this coming from?
>
This is coming from... nowhere!

My apologies. I forgot to check if any of these virtio devices were
actually being declared as objects (via. OBJECT_DECLARE_SIMPLE_TYPE).

I will revert these changes for virtio-bt & virtio-scmi and be more
careful to check for these in the future. Thank you for pointing this
out.

>> +static const qmp_virtio_feature_map_t virtio_bt_feature_map[] = {
>> +    FEATURE_ENTRY(VIRTIO_BT_F_VND_HCI, \
>> +            "VIRTIO_BT_F_VND_HCI: Vendor command supported"),
>> +    FEATURE_ENTRY(VIRTIO_BT_F_MSFT_EXT, \
>> +            "VIRTIO_BT_F_MSFT_EXT: MSFT vendor supported"),
>> +    FEATURE_ENTRY(VIRTIO_BT_F_AOSP_EXT, \
>> +            "VIRTIO_BT_F_AOSP_EXT: AOSP vendor supported"),
>> +    FEATURE_ENTRY(VIRTIO_BT_F_CONFIG_V2, \
>> +            "VIRTIO_BT_F_CONFIG_V2: Using v2 configuration"),
>> +    { -1, "" }
>> +};
>> +#endif
>> +
>> +/* virtio-scmi features mapping */
>> +#ifdef CONFIG_VIRTIO_SCMI
> Where's this coming from?

Will revert this. See comment above.

>> +static const qmp_virtio_feature_map_t virtio_scmi_feature_map[] = {
>> +    FEATURE_ENTRY(VIRTIO_SCMI_F_P2A_CHANNELS, \
>> +            "VIRTIO_SCMI_F_P2A_CHANNELS: SCMI notifications or delayed "
>> +            "responses implemented"),
>> +    FEATURE_ENTRY(VIRTIO_SCMI_F_SHARED_MEMORY, \
>> +            "VIRTIO_SCMI_F_SHARED_MEMORY: SCMI shared memory region statistics "
>> +            "implemented"),
>> +    { -1, "" }
>> +};
>> +#endif
>> +
>>   #define CONVERT_FEATURES(type, map, is_status, bitmap)   \
>>       ({                                                   \
>>           type *list = NULL;                               \
>> @@ -625,6 +683,24 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>>           features->dev_features =
>>               CONVERT_FEATURES(strList, virtio_rng_feature_map, 0, bitmap);
>>           break;
>> +#endif
>> +#ifdef CONFIG_VIRTIO_GPIO
>> +    case VIRTIO_ID_GPIO:
>> +        features->dev_features =
>> +            CONVERT_FEATURES(strList, virtio_gpio_feature_map, 0, bitmap);
>> +        break;
>> +#endif
>> +#ifdef CONFIG_VIRTIO_BT
>> +    case VIRTIO_ID_BT:
>> +        features->dev_features =
>> +            CONVERT_FEATURES(strList, virtio_bt_feature_map, 0, bitmap);
>> +        break;
>> +#endif
>> +#ifdef CONFIG_VIRTIO_SCMI
>> +    case VIRTIO_ID_SCMI:
>> +        features->dev_features =
>> +            CONVERT_FEATURES(strList, virtio_scmi_feature_map, 0, bitmap);
>> +        break;
>>   #endif
>>       /* No features */
>>       case VIRTIO_ID_9P:
>> @@ -640,18 +716,15 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>>       case VIRTIO_ID_SIGNAL_DIST:
>>       case VIRTIO_ID_PSTORE:
>>       case VIRTIO_ID_SOUND:
>> -    case VIRTIO_ID_BT:
>>       case VIRTIO_ID_RPMB:
>>       case VIRTIO_ID_VIDEO_ENCODER:
>>       case VIRTIO_ID_VIDEO_DECODER:
>> -    case VIRTIO_ID_SCMI:
>>       case VIRTIO_ID_NITRO_SEC_MOD:
>>       case VIRTIO_ID_WATCHDOG:
>>       case VIRTIO_ID_CAN:
>>       case VIRTIO_ID_DMABUF:
>>       case VIRTIO_ID_PARAM_SERV:
>>       case VIRTIO_ID_AUDIO_POLICY:
>> -    case VIRTIO_ID_GPIO:
>>           break;
>>       default:
>>           g_assert_not_reached();
>> -- 
>> 2.39.3

[-- Attachment #2: Type: text/html, Size: 11977 bytes --]