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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 9/12/22 09:54, Jonah Palmer wrote:
> 
> 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.

And 3 years later I hit this niche command while trying to remove a
legacy field...

commit 4987e5bf2e9262094fd89d2b8e4d5bd6c4c7312f

+    /**
+     * @use_started: true if the @started flag should be used to check
+     * the current state of the VirtIO device. Otherwise status bits
+     * should be checked for a current status of the device.
+     * @use_started is only set via QMP and defaults to true for all
+     * modern machines (since 4.1).
+     */

(we are removing all machines up to 4.0)

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