| Message ID | 1660220684-24909-2-git-send-email-jonah.palmer@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hmp,qmp: Add commands to introspect virtio devices | expand |
Hi, On 11/8/22 14:24, Jonah Palmer wrote: > From: Laurent Vivier <lvivier@redhat.com> > > This new command lists all the instances of VirtIODevices with > their canonical QOM path and name. > > [Jonah: @virtio_list duplicates information that already exists in > the QOM composition tree. However, extracting necessary information > from this tree seems to be a bit convoluted. > > Instead, we still create our own list of realized virtio devices > but use @qmp_qom_get with the device's canonical QOM path to confirm > that the device exists and is realized. If the device exists but > is actually not realized, then we remove it from our list (for > synchronicity to the QOM composition tree). > > Also, the QMP command @x-query-virtio is redundant as @qom-list > and @qom-get are sufficient to search '/machine/' for realized > virtio devices. However, @x-query-virtio is much more convenient > in listing realized virtio devices.] > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> > --- > hw/virtio/meson.build | 2 ++ > hw/virtio/virtio-stub.c | 14 ++++++++ > hw/virtio/virtio.c | 44 ++++++++++++++++++++++++ > include/hw/virtio/virtio.h | 1 + > qapi/meson.build | 1 + > qapi/qapi-schema.json | 1 + > qapi/virtio.json | 68 ++++++++++++++++++++++++++++++++++++++ > tests/qtest/qmp-cmd-test.c | 1 + > 8 files changed, 132 insertions(+) > create mode 100644 hw/virtio/virtio-stub.c > create mode 100644 qapi/virtio.json > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 5d607aeaa0..bdfa82e9c0 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -13,12 +13,18 @@ > > #include "qemu/osdep.h" > #include "qapi/error.h" > +#include "qapi/qmp/qdict.h" > +#include "qapi/qapi-commands-virtio.h" > +#include "qapi/qapi-commands-qom.h" > +#include "qapi/qapi-visit-virtio.h" > +#include "qapi/qmp/qjson.h" > #include "cpu.h" > #include "trace.h" > #include "qemu/error-report.h" > #include "qemu/log.h" > #include "qemu/main-loop.h" > #include "qemu/module.h" > +#include "qom/object_interfaces.h" > #include "hw/virtio/virtio.h" > #include "migration/qemu-file-types.h" > #include "qemu/atomic.h" > @@ -29,6 +35,9 @@ > #include "sysemu/runstate.h" > #include "standard-headers/linux/virtio_ids.h" > > +/* QAPI list of realized VirtIODevices */ > +static QTAILQ_HEAD(, VirtIODevice) virtio_list; > + > /* > * The alignment to use between consumer and producer parts of vring. > * x86 pagesize again. This is the default, used by transports like PCI > @@ -3698,6 +3707,7 @@ 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) > @@ -3712,6 +3722,7 @@ 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; > } > @@ -3885,6 +3896,8 @@ 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) > @@ -3895,6 +3908,37 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev) > return virtio_bus_ioeventfd_enabled(vbus); > } > > +VirtioInfoList *qmp_x_query_virtio(Error **errp) > +{ > + VirtioInfoList *list = NULL; > + VirtioInfoList *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 */ Why not check dev->realized instead of calling qmp_qom_get() & qobject_to_json_pretty()? > + if (!strncmp(is_realized->str, "false", 4)) { > + QTAILQ_REMOVE(&virtio_list, vdev, next); > + } else { > + node = g_new0(VirtioInfoList, 1); > + node->value = g_new(VirtioInfo, 1); > + node->value->path = g_strdup(dev->canonical_path); > + node->value->name = g_strdup(vdev->name); > + QAPI_LIST_PREPEND(list, node->value); > + } > + g_string_free(is_realized, true); > + } > + qobject_unref(obj); > + } > + > + return list; > +}
On 11/30/22 11:16, Philippe Mathieu-Daudé wrote: > Hi, > > On 11/8/22 14:24, Jonah Palmer wrote: >> From: Laurent Vivier <lvivier@redhat.com> >> >> This new command lists all the instances of VirtIODevices with >> their canonical QOM path and name. >> >> [Jonah: @virtio_list duplicates information that already exists in >> the QOM composition tree. However, extracting necessary information >> from this tree seems to be a bit convoluted. >> >> Instead, we still create our own list of realized virtio devices >> but use @qmp_qom_get with the device's canonical QOM path to confirm >> that the device exists and is realized. If the device exists but >> is actually not realized, then we remove it from our list (for >> synchronicity to the QOM composition tree). >> >> Also, the QMP command @x-query-virtio is redundant as @qom-list >> and @qom-get are sufficient to search '/machine/' for realized >> virtio devices. However, @x-query-virtio is much more convenient >> in listing realized virtio devices.] >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> >> --- >> hw/virtio/meson.build | 2 ++ >> hw/virtio/virtio-stub.c | 14 ++++++++ >> hw/virtio/virtio.c | 44 ++++++++++++++++++++++++ >> include/hw/virtio/virtio.h | 1 + >> qapi/meson.build | 1 + >> qapi/qapi-schema.json | 1 + >> qapi/virtio.json | 68 ++++++++++++++++++++++++++++++++++++++ >> tests/qtest/qmp-cmd-test.c | 1 + >> 8 files changed, 132 insertions(+) >> create mode 100644 hw/virtio/virtio-stub.c >> create mode 100644 qapi/virtio.json > >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 5d607aeaa0..bdfa82e9c0 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -13,12 +13,18 @@ >> #include "qemu/osdep.h" >> #include "qapi/error.h" >> +#include "qapi/qmp/qdict.h" >> +#include "qapi/qapi-commands-virtio.h" >> +#include "qapi/qapi-commands-qom.h" >> +#include "qapi/qapi-visit-virtio.h" >> +#include "qapi/qmp/qjson.h" >> #include "cpu.h" >> #include "trace.h" >> #include "qemu/error-report.h" >> #include "qemu/log.h" >> #include "qemu/main-loop.h" >> #include "qemu/module.h" >> +#include "qom/object_interfaces.h" >> #include "hw/virtio/virtio.h" >> #include "migration/qemu-file-types.h" >> #include "qemu/atomic.h" >> @@ -29,6 +35,9 @@ >> #include "sysemu/runstate.h" >> #include "standard-headers/linux/virtio_ids.h" >> +/* QAPI list of realized VirtIODevices */ >> +static QTAILQ_HEAD(, VirtIODevice) virtio_list; >> + >> /* >> * The alignment to use between consumer and producer parts of vring. >> * x86 pagesize again. This is the default, used by transports like >> PCI >> @@ -3698,6 +3707,7 @@ 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) >> @@ -3712,6 +3722,7 @@ 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; >> } >> @@ -3885,6 +3896,8 @@ 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) >> @@ -3895,6 +3908,37 @@ bool >> virtio_device_ioeventfd_enabled(VirtIODevice *vdev) >> return virtio_bus_ioeventfd_enabled(vbus); >> } >> +VirtioInfoList *qmp_x_query_virtio(Error **errp) >> +{ >> + VirtioInfoList *list = NULL; >> + VirtioInfoList *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 */ > > Why not check dev->realized instead of calling qmp_qom_get() & > qobject_to_json_pretty()? This check queries the QOM composition tree to check that the device actually exists and is realized. In other words, we just want to confirm with the QOM composition tree for the device. This was done to have some kind of synchronicity between @virtio_list and the QOM composition tree, since the list duplicates information that already exists in the tree. This check was recommended in v10 and added in v11 of this patch series. > >> + if (!strncmp(is_realized->str, "false", 4)) { >> + QTAILQ_REMOVE(&virtio_list, vdev, next); >> + } else { >> + node = g_new0(VirtioInfoList, 1); >> + node->value = g_new(VirtioInfo, 1); >> + node->value->path = g_strdup(dev->canonical_path); >> + node->value->name = g_strdup(vdev->name); >> + QAPI_LIST_PREPEND(list, node->value); >> + } >> + g_string_free(is_realized, true); >> + } >> + qobject_unref(obj); >> + } >> + >> + return list; >> +}
On 2/12/22 13:23, Jonah Palmer wrote: > > On 11/30/22 11:16, Philippe Mathieu-Daudé wrote: >> Hi, >> >> On 11/8/22 14:24, Jonah Palmer wrote: >>> From: Laurent Vivier <lvivier@redhat.com> >>> >>> This new command lists all the instances of VirtIODevices with >>> their canonical QOM path and name. >>> >>> [Jonah: @virtio_list duplicates information that already exists in >>> the QOM composition tree. However, extracting necessary information >>> from this tree seems to be a bit convoluted. >>> >>> Instead, we still create our own list of realized virtio devices >>> but use @qmp_qom_get with the device's canonical QOM path to confirm >>> that the device exists and is realized. If the device exists but >>> is actually not realized, then we remove it from our list (for >>> synchronicity to the QOM composition tree). >>> >>> Also, the QMP command @x-query-virtio is redundant as @qom-list >>> and @qom-get are sufficient to search '/machine/' for realized >>> virtio devices. However, @x-query-virtio is much more convenient >>> in listing realized virtio devices.] >>> >>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> >>> --- >>> hw/virtio/meson.build | 2 ++ >>> hw/virtio/virtio-stub.c | 14 ++++++++ >>> hw/virtio/virtio.c | 44 ++++++++++++++++++++++++ >>> include/hw/virtio/virtio.h | 1 + >>> qapi/meson.build | 1 + >>> qapi/qapi-schema.json | 1 + >>> qapi/virtio.json | 68 ++++++++++++++++++++++++++++++++++++++ >>> tests/qtest/qmp-cmd-test.c | 1 + >>> 8 files changed, 132 insertions(+) >>> create mode 100644 hw/virtio/virtio-stub.c >>> create mode 100644 qapi/virtio.json >> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index 5d607aeaa0..bdfa82e9c0 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -13,12 +13,18 @@ >>> #include "qemu/osdep.h" >>> #include "qapi/error.h" >>> +#include "qapi/qmp/qdict.h" >>> +#include "qapi/qapi-commands-virtio.h" >>> +#include "qapi/qapi-commands-qom.h" >>> +#include "qapi/qapi-visit-virtio.h" >>> +#include "qapi/qmp/qjson.h" >>> #include "cpu.h" >>> #include "trace.h" >>> #include "qemu/error-report.h" >>> #include "qemu/log.h" >>> #include "qemu/main-loop.h" >>> #include "qemu/module.h" >>> +#include "qom/object_interfaces.h" >>> #include "hw/virtio/virtio.h" >>> #include "migration/qemu-file-types.h" >>> #include "qemu/atomic.h" >>> @@ -29,6 +35,9 @@ >>> #include "sysemu/runstate.h" >>> #include "standard-headers/linux/virtio_ids.h" >>> +/* QAPI list of realized VirtIODevices */ >>> +static QTAILQ_HEAD(, VirtIODevice) virtio_list; >>> + >>> /* >>> * The alignment to use between consumer and producer parts of vring. >>> * x86 pagesize again. This is the default, used by transports like >>> PCI >>> @@ -3698,6 +3707,7 @@ 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) >>> @@ -3712,6 +3722,7 @@ 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; >>> } >>> @@ -3885,6 +3896,8 @@ 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) >>> @@ -3895,6 +3908,37 @@ bool >>> virtio_device_ioeventfd_enabled(VirtIODevice *vdev) >>> return virtio_bus_ioeventfd_enabled(vbus); >>> } >>> +VirtioInfoList *qmp_x_query_virtio(Error **errp) >>> +{ >>> + VirtioInfoList *list = NULL; >>> + VirtioInfoList *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 */ >> >> Why not check dev->realized instead of calling qmp_qom_get() & >> qobject_to_json_pretty()? > > This check queries the QOM composition tree to check that the device actually exists and is > realized. In other words, we just want to confirm with the QOM composition tree for the device. > > This was done to have some kind of synchronicity between @virtio_list and the QOM composition > tree, since the list duplicates information that already exists in the tree. > > This check was recommended in v10 and added in v11 of this patch series. Thanks, I found Markus comments: v10: https://lore.kernel.org/qemu-devel/87ee6ogbiw.fsf@dusky.pond.sub.org/ v11: https://lore.kernel.org/qemu-devel/87ee4abtdu.fsf@pond.sub.org/ Having the justification from v11 in the code rather than the commit description could help. >> >>> + if (!strncmp(is_realized->str, "false", 4)) { >>> + QTAILQ_REMOVE(&virtio_list, vdev, next); >>> + } else { >>> + node = g_new0(VirtioInfoList, 1); >>> + node->value = g_new(VirtioInfo, 1); >>> + node->value->path = g_strdup(dev->canonical_path); >>> + node->value->name = g_strdup(vdev->name); >>> + QAPI_LIST_PREPEND(list, node->value); >>> + } >>> + g_string_free(is_realized, true); >>> + } >>> + qobject_unref(obj); >>> + } >>> + >>> + return list; >>> +}
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 2/12/22 13:23, Jonah Palmer wrote: >> On 11/30/22 11:16, Philippe Mathieu-Daudé wrote: >>> Hi, >>> >>> On 11/8/22 14:24, Jonah Palmer wrote: >>>> From: Laurent Vivier <lvivier@redhat.com> >>>> >>>> This new command lists all the instances of VirtIODevices with >>>> their canonical QOM path and name. >>>> >>>> [Jonah: @virtio_list duplicates information that already exists in >>>> the QOM composition tree. However, extracting necessary information >>>> from this tree seems to be a bit convoluted. >>>> >>>> Instead, we still create our own list of realized virtio devices >>>> but use @qmp_qom_get with the device's canonical QOM path to confirm >>>> that the device exists and is realized. If the device exists but >>>> is actually not realized, then we remove it from our list (for >>>> synchronicity to the QOM composition tree). How could this happen? >>>> >>>> Also, the QMP command @x-query-virtio is redundant as @qom-list >>>> and @qom-get are sufficient to search '/machine/' for realized >>>> virtio devices. However, @x-query-virtio is much more convenient >>>> in listing realized virtio devices.] >>>> >>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> >>>> --- >>>> hw/virtio/meson.build | 2 ++ >>>> hw/virtio/virtio-stub.c | 14 ++++++++ >>>> hw/virtio/virtio.c | 44 ++++++++++++++++++++++++ >>>> include/hw/virtio/virtio.h | 1 + >>>> qapi/meson.build | 1 + >>>> qapi/qapi-schema.json | 1 + >>>> qapi/virtio.json | 68 ++++++++++++++++++++++++++++++++++++++ >>>> tests/qtest/qmp-cmd-test.c | 1 + >>>> 8 files changed, 132 insertions(+) >>>> create mode 100644 hw/virtio/virtio-stub.c >>>> create mode 100644 qapi/virtio.json >>> >>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>> index 5d607aeaa0..bdfa82e9c0 100644 >>>> --- a/hw/virtio/virtio.c >>>> +++ b/hw/virtio/virtio.c >>>> @@ -13,12 +13,18 @@ >>>> #include "qemu/osdep.h" >>>> #include "qapi/error.h" >>>> +#include "qapi/qmp/qdict.h" >>>> +#include "qapi/qapi-commands-virtio.h" >>>> +#include "qapi/qapi-commands-qom.h" >>>> +#include "qapi/qapi-visit-virtio.h" >>>> +#include "qapi/qmp/qjson.h" >>>> #include "cpu.h" >>>> #include "trace.h" >>>> #include "qemu/error-report.h" >>>> #include "qemu/log.h" >>>> #include "qemu/main-loop.h" >>>> #include "qemu/module.h" >>>> +#include "qom/object_interfaces.h" >>>> #include "hw/virtio/virtio.h" >>>> #include "migration/qemu-file-types.h" >>>> #include "qemu/atomic.h" >>>> @@ -29,6 +35,9 @@ >>>> #include "sysemu/runstate.h" >>>> #include "standard-headers/linux/virtio_ids.h" >>>> +/* QAPI list of realized VirtIODevices */ >>>> +static QTAILQ_HEAD(, VirtIODevice) virtio_list; >>>> + >>>> /* >>>> * The alignment to use between consumer and producer parts of vring. >>>> * x86 pagesize again. This is the default, used by transports like PCI >>>> @@ -3698,6 +3707,7 @@ 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) >>>> @@ -3712,6 +3722,7 @@ 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; >>>> } >>>> @@ -3885,6 +3896,8 @@ 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) >>>> @@ -3895,6 +3908,37 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev) >>>> return virtio_bus_ioeventfd_enabled(vbus); >>>> } >>>> +VirtioInfoList *qmp_x_query_virtio(Error **errp) >>>> +{ >>>> + VirtioInfoList *list = NULL; >>>> + VirtioInfoList *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 */ >>> >>> Why not check dev->realized instead of calling qmp_qom_get() & qobject_to_json_pretty()? >> >> This check queries the QOM composition tree to check that the device actually exists and is >> realized. In other words, we just want to confirm with the QOM composition tree for the device. Again, how could this happen? If @virtio_list isn't reliable, why have it in the first place? >> This was done to have some kind of synchronicity between @virtio_list and the QOM composition >> tree, since the list duplicates information that already exists in the tree. >> This check was recommended in v10 and added in v11 of this patch series. > > Thanks, I found Markus comments: > > v10: > https://lore.kernel.org/qemu-devel/87ee6ogbiw.fsf@dusky.pond.sub.org/ My recommendation there was to *delete* virtio_list and search the QOM composition tree instead: @virtio_list duplicates information that exists in the QOM composition tree. It needs to stay in sync. You could search the composition tree instead. The QOM composition tree is the source of truth. This above is about the command's implementation, and ... > v11: > https://lore.kernel.org/qemu-devel/87ee4abtdu.fsf@pond.sub.org/ > > Having the justification from v11 in the code rather than the commit > description could help. ... this part is about why the command could be useful. [...]
On 12/2/22 10:21, Markus Armbruster wrote: > Philippe Mathieu-Daudé<philmd@linaro.org> writes: > >> On 2/12/22 13:23, Jonah Palmer wrote: >>> On 11/30/22 11:16, Philippe Mathieu-Daudé wrote: >>>> Hi, >>>> >>>> On 11/8/22 14:24, Jonah Palmer wrote: >>>>> From: Laurent Vivier<lvivier@redhat.com> >>>>> >>>>> This new command lists all the instances of VirtIODevices with >>>>> their canonical QOM path and name. >>>>> >>>>> [Jonah: @virtio_list duplicates information that already exists in >>>>> the QOM composition tree. However, extracting necessary information >>>>> from this tree seems to be a bit convoluted. >>>>> >>>>> Instead, we still create our own list of realized virtio devices >>>>> but use @qmp_qom_get with the device's canonical QOM path to confirm >>>>> that the device exists and is realized. If the device exists but >>>>> is actually not realized, then we remove it from our list (for >>>>> synchronicity to the QOM composition tree). > How could this happen? > >>>>> Also, the QMP command @x-query-virtio is redundant as @qom-list >>>>> and @qom-get are sufficient to search '/machine/' for realized >>>>> virtio devices. However, @x-query-virtio is much more convenient >>>>> in listing realized virtio devices.] >>>>> >>>>> Signed-off-by: Laurent Vivier<lvivier@redhat.com> >>>>> Signed-off-by: Jonah Palmer<jonah.palmer@oracle.com> >>>>> --- >>>>> hw/virtio/meson.build | 2 ++ >>>>> hw/virtio/virtio-stub.c | 14 ++++++++ >>>>> hw/virtio/virtio.c | 44 ++++++++++++++++++++++++ >>>>> include/hw/virtio/virtio.h | 1 + >>>>> qapi/meson.build | 1 + >>>>> qapi/qapi-schema.json | 1 + >>>>> qapi/virtio.json | 68 ++++++++++++++++++++++++++++++++++++++ >>>>> tests/qtest/qmp-cmd-test.c | 1 + >>>>> 8 files changed, 132 insertions(+) >>>>> create mode 100644 hw/virtio/virtio-stub.c >>>>> create mode 100644 qapi/virtio.json >>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>>> index 5d607aeaa0..bdfa82e9c0 100644 >>>>> --- a/hw/virtio/virtio.c >>>>> +++ b/hw/virtio/virtio.c >>>>> @@ -13,12 +13,18 @@ >>>>> #include "qemu/osdep.h" >>>>> #include "qapi/error.h" >>>>> +#include "qapi/qmp/qdict.h" >>>>> +#include "qapi/qapi-commands-virtio.h" >>>>> +#include "qapi/qapi-commands-qom.h" >>>>> +#include "qapi/qapi-visit-virtio.h" >>>>> +#include "qapi/qmp/qjson.h" >>>>> #include "cpu.h" >>>>> #include "trace.h" >>>>> #include "qemu/error-report.h" >>>>> #include "qemu/log.h" >>>>> #include "qemu/main-loop.h" >>>>> #include "qemu/module.h" >>>>> +#include "qom/object_interfaces.h" >>>>> #include "hw/virtio/virtio.h" >>>>> #include "migration/qemu-file-types.h" >>>>> #include "qemu/atomic.h" >>>>> @@ -29,6 +35,9 @@ >>>>> #include "sysemu/runstate.h" >>>>> #include "standard-headers/linux/virtio_ids.h" >>>>> +/* QAPI list of realized VirtIODevices */ >>>>> +static QTAILQ_HEAD(, VirtIODevice) virtio_list; >>>>> + >>>>> /* >>>>> * The alignment to use between consumer and producer parts of vring. >>>>> * x86 pagesize again. This is the default, used by transports like PCI >>>>> @@ -3698,6 +3707,7 @@ 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) >>>>> @@ -3712,6 +3722,7 @@ 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; >>>>> } >>>>> @@ -3885,6 +3896,8 @@ 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) >>>>> @@ -3895,6 +3908,37 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev) >>>>> return virtio_bus_ioeventfd_enabled(vbus); >>>>> } >>>>> +VirtioInfoList *qmp_x_query_virtio(Error **errp) >>>>> +{ >>>>> + VirtioInfoList *list = NULL; >>>>> + VirtioInfoList *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 */ >>>> Why not check dev->realized instead of calling qmp_qom_get() & qobject_to_json_pretty()? >>> This check queries the QOM composition tree to check that the device actually exists and is >>> realized. In other words, we just want to confirm with the QOM composition tree for the device. > Again, how could this happen? > > If @virtio_list isn't reliable, why have it in the first place? Honestly, I'm not sure how this even could happen, since the @virtio_list is managed at the realization and unrealization of a virtio device. Given this, I do feel as though the list is reliable, although this might just benaïve of me to say. After giving this a second look, the @virtio_list is only really needed to provide a nice list of all realized virtio devices on the system, along with their full canonical paths. If the user can find the canonical path of a virtio device by searching the QOM composition tree, and assuming we can get a @VirtioDevice object (in code) from this canonical path, then the rest of the QMP/HMP features of these patches could be done by solely by searching the QOM composition tree. However, I think having this list of realized virtio devices as a subset of the QOM composition tree is worth its weight, since the user no longer has to search the entire tree to find virtio devices and piece together their canonical paths. Of course, if we're somehow able to generate a similar list in code by searching the QOM composition tree, then there would be no need for this @virtio_list. However, it's really not clear how, or if, such a list could be generated by parsing the QOM composition tree. >>> This was done to have some kind of synchronicity between @virtio_list and the QOM composition >>> tree, since the list duplicates information that already exists in the tree. >>> This check was recommended in v10 and added in v11 of this patch series. >> Thanks, I found Markus comments: >> >> v10: >> https://urldefense.com/v3/__https://lore.kernel.org/qemu-devel/87ee6ogbiw.fsf@dusky.pond.sub.org/__;!!ACWV5N9M2RV99hQ!LqeLFhE8PtTTg_qKRuP9Kgz5pyTNZLhYeRzp4a2oS8c3D5W8-irZoBW0L_Lf1ozm3qYidYhuVrjxjsAw$ > My recommendation there was to *delete* virtio_list and search the QOM > composition tree instead: > > @virtio_list duplicates information that exists in the QOM composition > tree. It needs to stay in sync. You could search the composition tree > instead. > > The QOM composition tree is the source of truth. > > This above is about the command's implementation, and ... > >> v11: >> https://urldefense.com/v3/__https://lore.kernel.org/qemu-devel/87ee4abtdu.fsf@pond.sub.org/__;!!ACWV5N9M2RV99hQ!LqeLFhE8PtTTg_qKRuP9Kgz5pyTNZLhYeRzp4a2oS8c3D5W8-irZoBW0L_Lf1ozm3qYidYhuVmVCuHjV$ >> >> Having the justification from v11 in the code rather than the commit >> description could help. > ... this part is about why the command could be useful. > > [...] >
Jonah Palmer <jonah.palmer@oracle.com> writes: > On 12/2/22 10:21, Markus Armbruster wrote: >> Philippe Mathieu-Daudé<philmd@linaro.org> writes: >> >>> On 2/12/22 13:23, Jonah Palmer wrote: >>>> On 11/30/22 11:16, Philippe Mathieu-Daudé wrote: >>>>> Hi, >>>>> >>>>> On 11/8/22 14:24, Jonah Palmer wrote: >>>>>> From: Laurent Vivier<lvivier@redhat.com> >>>>>> >>>>>> This new command lists all the instances of VirtIODevices with >>>>>> their canonical QOM path and name. >>>>>> >>>>>> [Jonah: @virtio_list duplicates information that already exists in >>>>>> the QOM composition tree. However, extracting necessary information >>>>>> from this tree seems to be a bit convoluted. >>>>>> >>>>>> Instead, we still create our own list of realized virtio devices >>>>>> but use @qmp_qom_get with the device's canonical QOM path to confirm >>>>>> that the device exists and is realized. If the device exists but >>>>>> is actually not realized, then we remove it from our list (for >>>>>> synchronicity to the QOM composition tree). >> >> How could this happen? >> >>>>>> Also, the QMP command @x-query-virtio is redundant as @qom-list >>>>>> and @qom-get are sufficient to search '/machine/' for realized >>>>>> virtio devices. However, @x-query-virtio is much more convenient >>>>>> in listing realized virtio devices.] >>>>>> >>>>>> Signed-off-by: Laurent Vivier<lvivier@redhat.com> >>>>>> Signed-off-by: Jonah Palmer<jonah.palmer@oracle.com> >>>>>> --- >>>>>> hw/virtio/meson.build | 2 ++ >>>>>> hw/virtio/virtio-stub.c | 14 ++++++++ >>>>>> hw/virtio/virtio.c | 44 ++++++++++++++++++++++++ >>>>>> include/hw/virtio/virtio.h | 1 + >>>>>> qapi/meson.build | 1 + >>>>>> qapi/qapi-schema.json | 1 + >>>>>> qapi/virtio.json | 68 ++++++++++++++++++++++++++++++++++++++ >>>>>> tests/qtest/qmp-cmd-test.c | 1 + >>>>>> 8 files changed, 132 insertions(+) >>>>>> create mode 100644 hw/virtio/virtio-stub.c >>>>>> create mode 100644 qapi/virtio.json >>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>>>> index 5d607aeaa0..bdfa82e9c0 100644 >>>>>> --- a/hw/virtio/virtio.c >>>>>> +++ b/hw/virtio/virtio.c >>>>>> @@ -13,12 +13,18 @@ >>>>>> #include "qemu/osdep.h" >>>>>> #include "qapi/error.h" >>>>>> +#include "qapi/qmp/qdict.h" >>>>>> +#include "qapi/qapi-commands-virtio.h" >>>>>> +#include "qapi/qapi-commands-qom.h" >>>>>> +#include "qapi/qapi-visit-virtio.h" >>>>>> +#include "qapi/qmp/qjson.h" >>>>>> #include "cpu.h" >>>>>> #include "trace.h" >>>>>> #include "qemu/error-report.h" >>>>>> #include "qemu/log.h" >>>>>> #include "qemu/main-loop.h" >>>>>> #include "qemu/module.h" >>>>>> +#include "qom/object_interfaces.h" >>>>>> #include "hw/virtio/virtio.h" >>>>>> #include "migration/qemu-file-types.h" >>>>>> #include "qemu/atomic.h" >>>>>> @@ -29,6 +35,9 @@ >>>>>> #include "sysemu/runstate.h" >>>>>> #include "standard-headers/linux/virtio_ids.h" >>>>>> +/* QAPI list of realized VirtIODevices */ >>>>>> +static QTAILQ_HEAD(, VirtIODevice) virtio_list; >>>>>> + >>>>>> /* >>>>>> * The alignment to use between consumer and producer parts of vring. >>>>>> * x86 pagesize again. This is the default, used by transports like PCI >>>>>> @@ -3698,6 +3707,7 @@ 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) >>>>>> @@ -3712,6 +3722,7 @@ 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; >>>>>> } >>>>>> @@ -3885,6 +3896,8 @@ 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) >>>>>> @@ -3895,6 +3908,37 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev) >>>>>> return virtio_bus_ioeventfd_enabled(vbus); >>>>>> } >>>>>> +VirtioInfoList *qmp_x_query_virtio(Error **errp) >>>>>> +{ >>>>>> + VirtioInfoList *list = NULL; >>>>>> + VirtioInfoList *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 */ >>>>> >>>>> Why not check dev->realized instead of calling qmp_qom_get() & qobject_to_json_pretty()? >>>> >>>> This check queries the QOM composition tree to check that the device actually exists and is >>>> realized. In other words, we just want to confirm with the QOM composition tree for the device. >> >> Again, how could this happen? >> >> If @virtio_list isn't reliable, why have it in the first place? > > Honestly, I'm not sure how this even could happen, since the @virtio_list is managed at the realization > and unrealization of a virtio device. Given this, I do feel as though the list is reliable, although > this might just benaïve of me to say. After giving this a second look, the @virtio_list is only really needed to provide a nice list of all realized virtio devices > on the system, along with their full canonical paths. If the user can find the canonical path of a virtio device by searching the QOM > composition tree, and assuming we can get a @VirtioDevice object (in code) from this canonical path, then the rest of the QMP/HMP features of > these patches could be done by solely by searching the QOM composition tree. However, I think having this list of realized virtio devices as a > subset of the QOM composition tree is worth its weight, since the user no longer has to search the entire tree to find virtio devices and piece > together their canonical paths. Of course, if we're somehow able to generate a similar list in code by searching the QOM composition tree, > then there would be no need for this @virtio_list. However, it's really not clear how, or if, such a list could be generated by parsing the QOM > composition tree. I'm not debating whether to have the command right now. I'm debating the introduction of variable @virtio_list. Please consider... >>>> This was done to have some kind of synchronicity between @virtio_list and the QOM composition >>>> tree, since the list duplicates information that already exists in the tree. >>>> This check was recommended in v10 and added in v11 of this patch series. >>> >>> Thanks, I found Markus comments: >>> >>> v10: >>> https://urldefense.com/v3/__https://lore.kernel.org/qemu-devel/87ee6ogbiw.fsf@dusky.pond.sub.org/__;!!ACWV5N9M2RV99hQ!LqeLFhE8PtTTg_qKRuP9Kgz5pyTNZLhYeRzp4a2oS8c3D5W8-irZoBW0L_Lf1ozm3qYidYhuVrjxjsAw$ ... this: >> My recommendation there was to *delete* virtio_list and search the QOM >> composition tree instead: >> >> @virtio_list duplicates information that exists in the QOM composition >> tree. It needs to stay in sync. You could search the composition tree >> instead. >> >> The QOM composition tree is the source of truth. Let me tell you a tale of two patches. One created a niche QMP command. It also modified core infrastructure to keep additional state. State that needed to be kept consistent with existing state forever. Consistency was not entirely obvious. The command examined this new state. The examination was simple. The other one created a niche QMP command, and nothing more. The command examined state without changing it. The examination was less simple. One of the two patches had a much easier time in review. Which one could it be... Please give the other approach a serious try. [...]
On 12/7/22 08:22, Markus Armbruster wrote: > Jonah Palmer<jonah.palmer@oracle.com> writes: > >> On 12/2/22 10:21, Markus Armbruster wrote: >>> Philippe Mathieu-Daudé<philmd@linaro.org> writes: >>> >>>> On 2/12/22 13:23, Jonah Palmer wrote: >>>>> On 11/30/22 11:16, Philippe Mathieu-Daudé wrote: >>>>>> Hi, >>>>>> >>>>>> On 11/8/22 14:24, Jonah Palmer wrote: >>>>>>> From: Laurent Vivier<lvivier@redhat.com> >>>>>>> >>>>>>> This new command lists all the instances of VirtIODevices with >>>>>>> their canonical QOM path and name. >>>>>>> >>>>>>> [Jonah: @virtio_list duplicates information that already exists in >>>>>>> the QOM composition tree. However, extracting necessary information >>>>>>> from this tree seems to be a bit convoluted. >>>>>>> >>>>>>> Instead, we still create our own list of realized virtio devices >>>>>>> but use @qmp_qom_get with the device's canonical QOM path to confirm >>>>>>> that the device exists and is realized. If the device exists but >>>>>>> is actually not realized, then we remove it from our list (for >>>>>>> synchronicity to the QOM composition tree). >>> How could this happen? >>> >>>>>>> Also, the QMP command @x-query-virtio is redundant as @qom-list >>>>>>> and @qom-get are sufficient to search '/machine/' for realized >>>>>>> virtio devices. However, @x-query-virtio is much more convenient >>>>>>> in listing realized virtio devices.] >>>>>>> >>>>>>> Signed-off-by: Laurent Vivier<lvivier@redhat.com> >>>>>>> Signed-off-by: Jonah Palmer<jonah.palmer@oracle.com> >>>>>>> --- >>>>>>> hw/virtio/meson.build | 2 ++ >>>>>>> hw/virtio/virtio-stub.c | 14 ++++++++ >>>>>>> hw/virtio/virtio.c | 44 ++++++++++++++++++++++++ >>>>>>> include/hw/virtio/virtio.h | 1 + >>>>>>> qapi/meson.build | 1 + >>>>>>> qapi/qapi-schema.json | 1 + >>>>>>> qapi/virtio.json | 68 ++++++++++++++++++++++++++++++++++++++ >>>>>>> tests/qtest/qmp-cmd-test.c | 1 + >>>>>>> 8 files changed, 132 insertions(+) >>>>>>> create mode 100644 hw/virtio/virtio-stub.c >>>>>>> create mode 100644 qapi/virtio.json >>>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>>>>> index 5d607aeaa0..bdfa82e9c0 100644 >>>>>>> --- a/hw/virtio/virtio.c >>>>>>> +++ b/hw/virtio/virtio.c >>>>>>> @@ -13,12 +13,18 @@ >>>>>>> #include "qemu/osdep.h" >>>>>>> #include "qapi/error.h" >>>>>>> +#include "qapi/qmp/qdict.h" >>>>>>> +#include "qapi/qapi-commands-virtio.h" >>>>>>> +#include "qapi/qapi-commands-qom.h" >>>>>>> +#include "qapi/qapi-visit-virtio.h" >>>>>>> +#include "qapi/qmp/qjson.h" >>>>>>> #include "cpu.h" >>>>>>> #include "trace.h" >>>>>>> #include "qemu/error-report.h" >>>>>>> #include "qemu/log.h" >>>>>>> #include "qemu/main-loop.h" >>>>>>> #include "qemu/module.h" >>>>>>> +#include "qom/object_interfaces.h" >>>>>>> #include "hw/virtio/virtio.h" >>>>>>> #include "migration/qemu-file-types.h" >>>>>>> #include "qemu/atomic.h" >>>>>>> @@ -29,6 +35,9 @@ >>>>>>> #include "sysemu/runstate.h" >>>>>>> #include "standard-headers/linux/virtio_ids.h" >>>>>>> +/* QAPI list of realized VirtIODevices */ >>>>>>> +static QTAILQ_HEAD(, VirtIODevice) virtio_list; >>>>>>> + >>>>>>> /* >>>>>>> * The alignment to use between consumer and producer parts of vring. >>>>>>> * x86 pagesize again. This is the default, used by transports like PCI >>>>>>> @@ -3698,6 +3707,7 @@ 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) >>>>>>> @@ -3712,6 +3722,7 @@ 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; >>>>>>> } >>>>>>> @@ -3885,6 +3896,8 @@ 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) >>>>>>> @@ -3895,6 +3908,37 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev) >>>>>>> return virtio_bus_ioeventfd_enabled(vbus); >>>>>>> } >>>>>>> +VirtioInfoList *qmp_x_query_virtio(Error **errp) >>>>>>> +{ >>>>>>> + VirtioInfoList *list = NULL; >>>>>>> + VirtioInfoList *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 */ >>>>>> Why not check dev->realized instead of calling qmp_qom_get() & qobject_to_json_pretty()? >>>>> This check queries the QOM composition tree to check that the device actually exists and is >>>>> realized. In other words, we just want to confirm with the QOM composition tree for the device. >>> Again, how could this happen? >>> >>> If @virtio_list isn't reliable, why have it in the first place? >> Honestly, I'm not sure how this even could happen, since the @virtio_list is managed at the realization >> and unrealization of a virtio device. Given this, I do feel as though the list is reliable, although >> this might just benaïve of me to say. After giving this a second look, the @virtio_list is only really needed to provide a nice list of all realized virtio devices >> on the system, along with their full canonical paths. If the user can find the canonical path of a virtio device by searching the QOM >> composition tree, and assuming we can get a @VirtioDevice object (in code) from this canonical path, then the rest of the QMP/HMP features of >> these patches could be done by solely by searching the QOM composition tree. However, I think having this list of realized virtio devices as a >> subset of the QOM composition tree is worth its weight, since the user no longer has to search the entire tree to find virtio devices and piece >> together their canonical paths. Of course, if we're somehow able to generate a similar list in code by searching the QOM composition tree, >> then there would be no need for this @virtio_list. However, it's really not clear how, or if, such a list could be generated by parsing the QOM >> composition tree. > I'm not debating whether to have the command right now. I'm debating > the introduction of variable @virtio_list. Please consider... > >>>>> This was done to have some kind of synchronicity between @virtio_list and the QOM composition >>>>> tree, since the list duplicates information that already exists in the tree. >>>>> This check was recommended in v10 and added in v11 of this patch series. >>>> Thanks, I found Markus comments: >>>> >>>> v10: >>>> https://urldefense.com/v3/__https://lore.kernel.org/qemu-devel/87ee6ogbiw.fsf@dusky.pond.sub.org/__;!!ACWV5N9M2RV99hQ!LqeLFhE8PtTTg_qKRuP9Kgz5pyTNZLhYeRzp4a2oS8c3D5W8-irZoBW0L_Lf1ozm3qYidYhuVrjxjsAw$ > ... this: > >>> My recommendation there was to *delete* virtio_list and search the QOM >>> composition tree instead: >>> >>> @virtio_list duplicates information that exists in the QOM composition >>> tree. It needs to stay in sync. You could search the composition tree >>> instead. >>> >>> The QOM composition tree is the source of truth. > Let me tell you a tale of two patches. > > One created a niche QMP command. It also modified core infrastructure > to keep additional state. State that needed to be kept consistent with > existing state forever. Consistency was not entirely obvious. The > command examined this new state. The examination was simple. > > The other one created a niche QMP command, and nothing more. The > command examined state without changing it. The examination was > less simple. > > One of the two patches had a much easier time in review. Which one > could it be... > > Please give the other approach a serious try. Ah, okay. I see where you're getting at. Sure, I can give this approach a try and see what I can do with it. Just FYI, I wont be able to get to this right away until sometime early next year. Jonah > > [...] >
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build index 7e8877fd64..e16f1b22d4 100644 --- a/hw/virtio/meson.build +++ b/hw/virtio/meson.build @@ -60,4 +60,6 @@ virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) specific_ss.add_all(when: 'CONFIG_VIRTIO', if_true: virtio_ss) softmmu_ss.add_all(when: 'CONFIG_VIRTIO', if_true: softmmu_virtio_ss) softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: files('vhost-stub.c')) +softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: files('virtio-stub.c')) softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('vhost-stub.c')) +softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('virtio-stub.c')) diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c new file mode 100644 index 0000000000..05a81edc92 --- /dev/null +++ b/hw/virtio/virtio-stub.c @@ -0,0 +1,14 @@ +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "qapi/qapi-commands-virtio.h" + +static void *qmp_virtio_unsupported(Error **errp) +{ + error_setg(errp, "Virtio is disabled"); + return NULL; +} + +VirtioInfoList *qmp_x_query_virtio(Error **errp) +{ + return qmp_virtio_unsupported(errp); +} diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 5d607aeaa0..bdfa82e9c0 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -13,12 +13,18 @@ #include "qemu/osdep.h" #include "qapi/error.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qapi-commands-virtio.h" +#include "qapi/qapi-commands-qom.h" +#include "qapi/qapi-visit-virtio.h" +#include "qapi/qmp/qjson.h" #include "cpu.h" #include "trace.h" #include "qemu/error-report.h" #include "qemu/log.h" #include "qemu/main-loop.h" #include "qemu/module.h" +#include "qom/object_interfaces.h" #include "hw/virtio/virtio.h" #include "migration/qemu-file-types.h" #include "qemu/atomic.h" @@ -29,6 +35,9 @@ #include "sysemu/runstate.h" #include "standard-headers/linux/virtio_ids.h" +/* QAPI list of realized VirtIODevices */ +static QTAILQ_HEAD(, VirtIODevice) virtio_list; + /* * The alignment to use between consumer and producer parts of vring. * x86 pagesize again. This is the default, used by transports like PCI @@ -3698,6 +3707,7 @@ 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) @@ -3712,6 +3722,7 @@ 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; } @@ -3885,6 +3896,8 @@ 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) @@ -3895,6 +3908,37 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev) return virtio_bus_ioeventfd_enabled(vbus); } +VirtioInfoList *qmp_x_query_virtio(Error **errp) +{ + VirtioInfoList *list = NULL; + VirtioInfoList *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_new0(VirtioInfoList, 1); + node->value = g_new(VirtioInfo, 1); + node->value->path = g_strdup(dev->canonical_path); + node->value->name = g_strdup(vdev->name); + QAPI_LIST_PREPEND(list, node->value); + } + g_string_free(is_realized, true); + } + qobject_unref(obj); + } + + return list; +} + static const TypeInfo virtio_device_info = { .name = TYPE_VIRTIO_DEVICE, .parent = TYPE_DEVICE, diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index db1c0ddf6b..375eb5671b 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -110,6 +110,7 @@ struct VirtIODevice bool use_guest_notifier_mask; AddressSpace *dma_as; QLIST_HEAD(, VirtQueue) *vector_queues; + QTAILQ_ENTRY(VirtIODevice) next; }; struct VirtioDeviceClass { diff --git a/qapi/meson.build b/qapi/meson.build index fd5c93d643..c35ba874d2 100644 --- a/qapi/meson.build +++ b/qapi/meson.build @@ -49,6 +49,7 @@ qapi_all_modules = [ 'stats', 'trace', 'transaction', + 'virtio', 'yank', ] if have_system diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json index 92d7ecc52c..f000b90744 100644 --- a/qapi/qapi-schema.json +++ b/qapi/qapi-schema.json @@ -94,3 +94,4 @@ { 'include': 'acpi.json' } { 'include': 'pci.json' } { 'include': 'stats.json' } +{ 'include': 'virtio.json' } diff --git a/qapi/virtio.json b/qapi/virtio.json new file mode 100644 index 0000000000..03896e423f --- /dev/null +++ b/qapi/virtio.json @@ -0,0 +1,68 @@ +# -*- Mode: Python -*- +# vim: filetype=python +# + +## +# = Virtio devices +## + +## +# @VirtioInfo: +# +# Basic information about a given VirtIODevice +# +# @path: The VirtIODevice's canonical QOM path +# +# @name: Name of the VirtIODevice +# +# Since: 7.1 +# +## +{ 'struct': 'VirtioInfo', + 'data': { 'path': 'str', + 'name': 'str' } } + +## +# @x-query-virtio: +# +# Returns a list of all realized VirtIODevices +# +# Features: +# @unstable: This command is meant for debugging. +# +# Returns: List of gathered VirtIODevices +# +# Since: 7.1 +# +# Example: +# +# -> { "execute": "x-query-virtio" } +# <- { "return": [ +# { +# "name": "virtio-input", +# "path": "/machine/peripheral-anon/device[4]/virtio-backend" +# }, +# { +# "name": "virtio-crypto", +# "path": "/machine/peripheral/crypto0/virtio-backend" +# }, +# { +# "name": "virtio-scsi", +# "path": "/machine/peripheral-anon/device[2]/virtio-backend" +# }, +# { +# "name": "virtio-net", +# "path": "/machine/peripheral-anon/device[1]/virtio-backend" +# }, +# { +# "name": "virtio-serial", +# "path": "/machine/peripheral-anon/device[0]/virtio-backend" +# } +# ] +# } +# +## + +{ 'command': 'x-query-virtio', + 'returns': [ 'VirtioInfo' ], + 'features': [ 'unstable' ] } diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c index af00712458..897e4e937b 100644 --- a/tests/qtest/qmp-cmd-test.c +++ b/tests/qtest/qmp-cmd-test.c @@ -103,6 +103,7 @@ static bool query_is_ignored(const char *cmd) "query-gic-capabilities", /* arm */ /* Success depends on target-specific build configuration: */ "query-pci", /* CONFIG_PCI */ + "x-query-virtio", /* CONFIG_VIRTIO */ /* Success depends on launching SEV guest */ "query-sev-launch-measure", /* Success depends on Host or Hypervisor SEV support */