Re: [PATCH v15 2/6] qmp: add QMP command x-query-virtio-status

[Thomas Huth <thuth@redhat.com>]

On 02/05/2025 00.09, Philippe Mathieu-Daudé wrote:
> Hello,
> 
> (patch merged as commit 90c066cd682).
> 
> On 11/8/22 14:24, Jonah Palmer wrote:
>> From: Laurent Vivier <lvivier@redhat.com>
>>
>> This new command shows the status of a VirtIODevice, including
>> its corresponding vhost device's status (if active).
>>
>> Next patch will improve output by decoding feature bits, including
>> vhost device's feature bits (backend, protocol, acked, and features).
>> Also will decode status bits of a VirtIODevice.
>>
>> [Jonah: From patch v12; added a check to @virtio_device_find to ensure
>>   synchronicity between @virtio_list and the devices in the QOM
>>   composition tree.]
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   hw/virtio/virtio-stub.c |   5 +
>>   hw/virtio/virtio.c      | 104 +++++++++++++++++++
>>   qapi/virtio.json        | 222 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 331 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
>> index 05a81edc92..0b432e8de7 100644
>> --- a/hw/virtio/virtio-stub.c
>> +++ b/hw/virtio/virtio-stub.c
>> @@ -12,3 +12,8 @@ VirtioInfoList *qmp_x_query_virtio(Error **errp)
>>   {
>>       return qmp_virtio_unsupported(errp);
>>   }
>> +
>> +VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
>> +{
>> +    return qmp_virtio_unsupported(errp);
>> +}
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index bdfa82e9c0..3e0a484660 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -3939,6 +3939,110 @@ VirtioInfoList *qmp_x_query_virtio(Error **errp)
>>       return list;
>>   }
>> +static VirtIODevice *virtio_device_find(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;
>> +        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;
>> +            }
>> +            g_string_free(is_realized, true);
>> +        } else {
>> +            /* virtio device doesn't exist in QOM tree */
>> +            QTAILQ_REMOVE(&virtio_list, vdev, next);
>> +            qobject_unref(obj);
>> +            return NULL;
>> +        }
>> +        /* device exists in QOM tree & is realized */
>> +        qobject_unref(obj);
>> +        return vdev;
>> +    }
>> +    return NULL;
>> +}
>> +
>> +VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
>> +{
>> +    VirtIODevice *vdev;
>> +    VirtioStatus *status;
>> +
>> +    vdev = virtio_device_find(path);
>> +    if (vdev == NULL) {
>> +        error_setg(errp, "Path %s is not a VirtIODevice", path);
>> +        return NULL;
>> +    }
>> +
>> +    status = g_new0(VirtioStatus, 1);
>> +    status->name = g_strdup(vdev->name);
>> +    status->device_id = vdev->device_id;
>> +    status->vhost_started = vdev->vhost_started;
>> +    status->guest_features = vdev->guest_features;
>> +    status->host_features = vdev->host_features;
>> +    status->backend_features = vdev->backend_features;
>> +
>> +    switch (vdev->device_endian) {
>> +    case VIRTIO_DEVICE_ENDIAN_LITTLE:
>> +        status->device_endian = g_strdup("little");
>> +        break;
>> +    case VIRTIO_DEVICE_ENDIAN_BIG:
>> +        status->device_endian = g_strdup("big");
>> +        break;
>> +    default:
>> +        status->device_endian = g_strdup("unknown");
>> +        break;
>> +    }
>> +
>> +    status->num_vqs = virtio_get_num_queues(vdev);
>> +    status->status = vdev->status;
>> +    status->isr = vdev->isr;
>> +    status->queue_sel = vdev->queue_sel;
>> +    status->vm_running = vdev->vm_running;
>> +    status->broken = vdev->broken;
>> +    status->disabled = vdev->disabled;
>> +    status->use_started = vdev->use_started;
>> +    status->started = vdev->started;
>> +    status->start_on_kick = vdev->start_on_kick;
>> +    status->disable_legacy_check = vdev->disable_legacy_check;
>> +    status->bus_name = g_strdup(vdev->bus_name);
>> +    status->use_guest_notifier_mask = vdev->use_guest_notifier_mask;
>> +    status->has_vhost_dev = vdev->vhost_started;
>> +
>> +    if (vdev->vhost_started) {
>> +        VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>> +        struct vhost_dev *hdev = vdc->get_vhost(vdev);
>> +
>> +        status->vhost_dev = g_new0(VhostStatus, 1);
>> +        status->vhost_dev->n_mem_sections = hdev->n_mem_sections;
>> +        status->vhost_dev->n_tmp_sections = hdev->n_tmp_sections;
>> +        status->vhost_dev->nvqs = hdev->nvqs;
>> +        status->vhost_dev->vq_index = hdev->vq_index;
>> +        status->vhost_dev->features = hdev->features;
>> +        status->vhost_dev->acked_features = hdev->acked_features;
>> +        status->vhost_dev->backend_features = hdev->backend_features;
>> +        status->vhost_dev->protocol_features = hdev->protocol_features;
>> +        status->vhost_dev->max_queues = hdev->max_queues;
>> +        status->vhost_dev->backend_cap = hdev->backend_cap;
>> +        status->vhost_dev->log_enabled = hdev->log_enabled;
>> +        status->vhost_dev->log_size = hdev->log_size;
>> +    }
>> +
>> +    return status;
>> +}
>> +
>>   static const TypeInfo virtio_device_info = {
>>       .name = TYPE_VIRTIO_DEVICE,
>>       .parent = TYPE_DEVICE,
>> diff --git a/qapi/virtio.json b/qapi/virtio.json
>> index 03896e423f..c86b3bc635 100644
>> --- a/qapi/virtio.json
>> +++ b/qapi/virtio.json
>> @@ -66,3 +66,225 @@
>>   { 'command': 'x-query-virtio',
>>     'returns': [ 'VirtioInfo' ],
>>     'features': [ 'unstable' ] }
> 
> When a structure is only used by an unstable command, is it considered
> unstable too? IOW is it OK to modify it?

IMHO yes. If there is no other user of the struct, why should it considered 
to be stable if the only command that uses it is unstable?

  Thomas