Message ID | 1635334909-31614-1-git-send-email-jonah.palmer@oracle.com (mailing list archive) |
---|---|
Headers | show |
Series | hmp,qmp: Add commands to introspect virtio devices | expand |
On Wed, Oct 27, 2021 at 07:41:41AM -0400, Jonah Palmer wrote: > This series introduces new QMP/HMP commands to dump the status of a > virtio device at different levels. > > [Jonah: Rebasing previous patchset from Oct. 5 (v7). Original patches > are from Laurent Vivier from May 2020. > > Rebase from v7 to v8 includes an additional assert to make sure > we're not returning NULL in virtio_id_to_name(). Rebase also > includes minor additions/edits to qapi/virtio.json.] > > 1. Main command > > HMP Only: > > virtio [subcommand] > > Example: > > List all sub-commands: > > (qemu) virtio > virtio query -- List all available virtio devices > virtio status path -- Display status of a given virtio device > virtio queue-status path queue -- Display status of a given virtio queue > virtio vhost-queue-status path queue -- Display status of a given vhost queue > virtio queue-element path queue [index] -- Display element of a given virtio queue I don't see a compelling reason why these are setup as sub-commands under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query' naming just feels needlessly different from the current QEMU practices. IMHO they should just be "info" subcommands for HMP. ie info virtio -- List all available virtio devices info virtio-status path -- Display status of a given virtio device info virtio-queue-status path queue -- Display status of a given virtio queue info virtio-vhost-queue-status path queue -- Display status of a given vhost queue info virtio-queue-element path queue [index] -- Display element of a given virtio queue While the corresponding QMP commands ought to be x-query-virtio x-query-virtio-status x-query-virtio-queue-status x-query-virtio-vhost-queue-status x-query-virtio-queue-element Regards, Daniel
On 27.10.21 13:41, Jonah Palmer wrote: > From: Laurent Vivier <lvivier@redhat.com> > > Display feature names instead of bitmaps for host, guest, and > backend for VirtIODevice. > > Display status names instead of bitmaps for VirtIODevice. > > Display feature names instead of bitmaps for backend, protocol, > acked, and features (hdev->features) for vhost devices. > > Decode features according to device type. Decode status > according to configuration status bitmap (config_status_map). > Decode vhost user protocol features according to vhost user > protocol bitmap (vhost_user_protocol_map). > > Transport features are on the first line. Undecoded bits > (if any) are stored in a separate field. Vhost device field > wont show if there's no vhost active for a given VirtIODevice. > > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> > --- > hw/block/virtio-blk.c | 28 ++ > hw/char/virtio-serial-bus.c | 11 + > hw/display/virtio-gpu-base.c | 18 +- > hw/input/virtio-input.c | 11 +- > hw/net/virtio-net.c | 47 ++++ > hw/scsi/virtio-scsi.c | 17 ++ > hw/virtio/vhost-user-fs.c | 10 + > hw/virtio/vhost-vsock-common.c | 10 + > hw/virtio/virtio-balloon.c | 14 + > hw/virtio/virtio-crypto.c | 10 + > hw/virtio/virtio-iommu.c | 14 + > hw/virtio/virtio.c | 273 ++++++++++++++++++- > include/hw/virtio/vhost.h | 3 + > include/hw/virtio/virtio.h | 17 ++ > qapi/virtio.json | 577 ++++++++++++++++++++++++++++++++++++++--- Any particular reason we're not handling virtio-mem? (there is only a single feature bit so far, a second one to be introduced soon)
On 27/10/2021 13:59, David Hildenbrand wrote: > On 27.10.21 13:41, Jonah Palmer wrote: >> From: Laurent Vivier <lvivier@redhat.com> >> >> Display feature names instead of bitmaps for host, guest, and >> backend for VirtIODevice. >> >> Display status names instead of bitmaps for VirtIODevice. >> >> Display feature names instead of bitmaps for backend, protocol, >> acked, and features (hdev->features) for vhost devices. >> >> Decode features according to device type. Decode status >> according to configuration status bitmap (config_status_map). >> Decode vhost user protocol features according to vhost user >> protocol bitmap (vhost_user_protocol_map). >> >> Transport features are on the first line. Undecoded bits >> (if any) are stored in a separate field. Vhost device field >> wont show if there's no vhost active for a given VirtIODevice. >> >> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> >> --- >> hw/block/virtio-blk.c | 28 ++ >> hw/char/virtio-serial-bus.c | 11 + >> hw/display/virtio-gpu-base.c | 18 +- >> hw/input/virtio-input.c | 11 +- >> hw/net/virtio-net.c | 47 ++++ >> hw/scsi/virtio-scsi.c | 17 ++ >> hw/virtio/vhost-user-fs.c | 10 + >> hw/virtio/vhost-vsock-common.c | 10 + >> hw/virtio/virtio-balloon.c | 14 + >> hw/virtio/virtio-crypto.c | 10 + >> hw/virtio/virtio-iommu.c | 14 + >> hw/virtio/virtio.c | 273 ++++++++++++++++++- >> include/hw/virtio/vhost.h | 3 + >> include/hw/virtio/virtio.h | 17 ++ >> qapi/virtio.json | 577 ++++++++++++++++++++++++++++++++++++++--- > > Any particular reason we're not handling virtio-mem? > > (there is only a single feature bit so far, a second one to be > introduced soon) > I think this is because the v1 of the series has been written in March 2020 and it has not been update when virtio-mem has been added (June 2020). Thanks, Laurent
On 10/27/21 07:55, Daniel P. Berrangé wrote: > On Wed, Oct 27, 2021 at 07:41:41AM -0400, Jonah Palmer wrote: >> This series introduces new QMP/HMP commands to dump the status of a >> virtio device at different levels. >> >> [Jonah: Rebasing previous patchset from Oct. 5 (v7). Original patches >> are from Laurent Vivier from May 2020. >> >> Rebase from v7 to v8 includes an additional assert to make sure >> we're not returning NULL in virtio_id_to_name(). Rebase also >> includes minor additions/edits to qapi/virtio.json.] >> >> 1. Main command >> >> HMP Only: >> >> virtio [subcommand] >> >> Example: >> >> List all sub-commands: >> >> (qemu) virtio >> virtio query -- List all available virtio devices >> virtio status path -- Display status of a given virtio device >> virtio queue-status path queue -- Display status of a given virtio queue >> virtio vhost-queue-status path queue -- Display status of a given vhost queue >> virtio queue-element path queue [index] -- Display element of a given virtio queue > I don't see a compelling reason why these are setup as sub-commands > under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query' > naming just feels needlessly different from the current QEMU practices. > > IMHO they should just be "info" subcommands for HMP. ie > > info virtio -- List all available virtio devices > info virtio-status path -- Display status of a given virtio device > info virtio-queue-status path queue -- Display status of a given virtio queue > info virtio-vhost-queue-status path queue -- Display status of a given vhost queue > info virtio-queue-element path queue [index] -- Display element of a given virtio queue > > While the corresponding QMP commands ought to be > > x-query-virtio > x-query-virtio-status > x-query-virtio-queue-status > x-query-virtio-vhost-queue-status > x-query-virtio-queue-element > > > Regards, > Daniel Sure, I don't mind changing it to this if this is what others would prefer. If there aren't any objections, I'll switch it to this in the next revision. Jonah
On 10/27/21 08:18, Laurent Vivier wrote: > On 27/10/2021 13:59, David Hildenbrand wrote: >> On 27.10.21 13:41, Jonah Palmer wrote: >>> From: Laurent Vivier <lvivier@redhat.com> >>> >>> Display feature names instead of bitmaps for host, guest, and >>> backend for VirtIODevice. >>> >>> Display status names instead of bitmaps for VirtIODevice. >>> >>> Display feature names instead of bitmaps for backend, protocol, >>> acked, and features (hdev->features) for vhost devices. >>> >>> Decode features according to device type. Decode status >>> according to configuration status bitmap (config_status_map). >>> Decode vhost user protocol features according to vhost user >>> protocol bitmap (vhost_user_protocol_map). >>> >>> Transport features are on the first line. Undecoded bits >>> (if any) are stored in a separate field. Vhost device field >>> wont show if there's no vhost active for a given VirtIODevice. >>> >>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> >>> --- >>> hw/block/virtio-blk.c | 28 ++ >>> hw/char/virtio-serial-bus.c | 11 + >>> hw/display/virtio-gpu-base.c | 18 +- >>> hw/input/virtio-input.c | 11 +- >>> hw/net/virtio-net.c | 47 ++++ >>> hw/scsi/virtio-scsi.c | 17 ++ >>> hw/virtio/vhost-user-fs.c | 10 + >>> hw/virtio/vhost-vsock-common.c | 10 + >>> hw/virtio/virtio-balloon.c | 14 + >>> hw/virtio/virtio-crypto.c | 10 + >>> hw/virtio/virtio-iommu.c | 14 + >>> hw/virtio/virtio.c | 273 ++++++++++++++++++- >>> include/hw/virtio/vhost.h | 3 + >>> include/hw/virtio/virtio.h | 17 ++ >>> qapi/virtio.json | 577 >>> ++++++++++++++++++++++++++++++++++++++--- >> >> Any particular reason we're not handling virtio-mem? >> >> (there is only a single feature bit so far, a second one to be >> introduced soon) >> > > I think this is because the v1 of the series has been written in March > 2020 and it has not been update when virtio-mem has been added (June > 2020). > > Thanks, > Laurent Oops, I think I just might've missed this device. I can add in support for virtio-mem in the next revision! Jonah > >
On 28.10.21 09:56, Jonah Palmer wrote: > On 10/27/21 08:18, Laurent Vivier wrote: >> On 27/10/2021 13:59, David Hildenbrand wrote: >>> On 27.10.21 13:41, Jonah Palmer wrote: >>>> From: Laurent Vivier <lvivier@redhat.com> >>>> >>>> Display feature names instead of bitmaps for host, guest, and >>>> backend for VirtIODevice. >>>> >>>> Display status names instead of bitmaps for VirtIODevice. >>>> >>>> Display feature names instead of bitmaps for backend, protocol, >>>> acked, and features (hdev->features) for vhost devices. >>>> >>>> Decode features according to device type. Decode status >>>> according to configuration status bitmap (config_status_map). >>>> Decode vhost user protocol features according to vhost user >>>> protocol bitmap (vhost_user_protocol_map). >>>> >>>> Transport features are on the first line. Undecoded bits >>>> (if any) are stored in a separate field. Vhost device field >>>> wont show if there's no vhost active for a given VirtIODevice. >>>> >>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> >>>> --- >>>> hw/block/virtio-blk.c | 28 ++ >>>> hw/char/virtio-serial-bus.c | 11 + >>>> hw/display/virtio-gpu-base.c | 18 +- >>>> hw/input/virtio-input.c | 11 +- >>>> hw/net/virtio-net.c | 47 ++++ >>>> hw/scsi/virtio-scsi.c | 17 ++ >>>> hw/virtio/vhost-user-fs.c | 10 + >>>> hw/virtio/vhost-vsock-common.c | 10 + >>>> hw/virtio/virtio-balloon.c | 14 + >>>> hw/virtio/virtio-crypto.c | 10 + >>>> hw/virtio/virtio-iommu.c | 14 + >>>> hw/virtio/virtio.c | 273 ++++++++++++++++++- >>>> include/hw/virtio/vhost.h | 3 + >>>> include/hw/virtio/virtio.h | 17 ++ >>>> qapi/virtio.json | 577 >>>> ++++++++++++++++++++++++++++++++++++++--- >>> >>> Any particular reason we're not handling virtio-mem? >>> >>> (there is only a single feature bit so far, a second one to be >>> introduced soon) >>> >> >> I think this is because the v1 of the series has been written in March >> 2020 and it has not been update when virtio-mem has been added (June >> 2020). >> >> Thanks, >> Laurent > > Oops, I think I just might've missed this device. I can add in support for virtio-mem > in the next revision! Cool, thanks! I consider the introspection interface very valuable!
* Jonah Palmer (jonah.palmer@oracle.com) wrote: > > On 10/27/21 07:55, Daniel P. Berrangé wrote: > > On Wed, Oct 27, 2021 at 07:41:41AM -0400, Jonah Palmer wrote: > > > This series introduces new QMP/HMP commands to dump the status of a > > > virtio device at different levels. > > > > > > [Jonah: Rebasing previous patchset from Oct. 5 (v7). Original patches > > > are from Laurent Vivier from May 2020. > > > > > > Rebase from v7 to v8 includes an additional assert to make sure > > > we're not returning NULL in virtio_id_to_name(). Rebase also > > > includes minor additions/edits to qapi/virtio.json.] > > > > > > 1. Main command > > > > > > HMP Only: > > > > > > virtio [subcommand] > > > > > > Example: > > > > > > List all sub-commands: > > > > > > (qemu) virtio > > > virtio query -- List all available virtio devices > > > virtio status path -- Display status of a given virtio device > > > virtio queue-status path queue -- Display status of a given virtio queue > > > virtio vhost-queue-status path queue -- Display status of a given vhost queue > > > virtio queue-element path queue [index] -- Display element of a given virtio queue > > I don't see a compelling reason why these are setup as sub-commands > > under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query' > > naming just feels needlessly different from the current QEMU practices. > > > > IMHO they should just be "info" subcommands for HMP. ie > > > > info virtio -- List all available virtio devices > > info virtio-status path -- Display status of a given virtio device > > info virtio-queue-status path queue -- Display status of a given virtio queue > > info virtio-vhost-queue-status path queue -- Display status of a given vhost queue > > info virtio-queue-element path queue [index] -- Display element of a given virtio queue > > > > While the corresponding QMP commands ought to be > > > > x-query-virtio > > x-query-virtio-status > > x-query-virtio-queue-status > > x-query-virtio-vhost-queue-status > > x-query-virtio-queue-element > > > > > > Regards, > > Daniel > > Sure, I don't mind changing it to this if this is what others would prefer. > If there aren't any objections, I'll switch it to this in the next revision. I agree with Dan that there's no need for the extra level; however you could do it all in one HMP command I think: info virtio -- list all available virtio devices info virtio path -- Display status of a given virtio device info virtio path queue -- Display status of a given virtio queue info virtio -h path queue -- Display status of a given vhost queue info virtio -e path queue [index] -- Display element of a given virtio queue It wouldn't need to change the qmp commands underneath unless it made sense. Dave > Jonah
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Oct 27, 2021 at 07:41:41AM -0400, Jonah Palmer wrote: >> This series introduces new QMP/HMP commands to dump the status of a >> virtio device at different levels. >> >> [Jonah: Rebasing previous patchset from Oct. 5 (v7). Original patches >> are from Laurent Vivier from May 2020. >> >> Rebase from v7 to v8 includes an additional assert to make sure >> we're not returning NULL in virtio_id_to_name(). Rebase also >> includes minor additions/edits to qapi/virtio.json.] >> >> 1. Main command >> >> HMP Only: >> >> virtio [subcommand] >> >> Example: >> >> List all sub-commands: >> >> (qemu) virtio >> virtio query -- List all available virtio devices >> virtio status path -- Display status of a given virtio device >> virtio queue-status path queue -- Display status of a given virtio queue >> virtio vhost-queue-status path queue -- Display status of a given vhost queue >> virtio queue-element path queue [index] -- Display element of a given virtio queue > > I don't see a compelling reason why these are setup as sub-commands > under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query' > naming just feels needlessly different from the current QEMU practices. > > IMHO they should just be "info" subcommands for HMP. ie > > info virtio -- List all available virtio devices > info virtio-status path -- Display status of a given virtio device > info virtio-queue-status path queue -- Display status of a given virtio queue > info virtio-vhost-queue-status path queue -- Display status of a given vhost queue > info virtio-queue-element path queue [index] -- Display element of a given virtio queue I agree with Dan (but I'm not the maintainer). > While the corresponding QMP commands ought to be > > x-query-virtio > x-query-virtio-status > x-query-virtio-queue-status > x-query-virtio-vhost-queue-status > x-query-virtio-queue-element I agree with Dan (and I am the maintainer). The x- is not strictly required anymore (see commit a3c45b3e62 'qapi: New special feature flag "unstable"'). I lean towards keeping it here, because we don't plan to stabilize these commands.
This series increases total size (text + data + bss) by 134KiB for me. QAPI clearly was not designed for space efficiency. Still, it's a drop in the bucket. If debugging commands ever become a burden for certain use cases, we can think about making them compile-time optional.
On 11/5/21 03:26, Markus Armbruster wrote: > Daniel P. Berrangé<berrange@redhat.com> writes: > >> On Wed, Oct 27, 2021 at 07:41:41AM -0400, Jonah Palmer wrote: >>> This series introduces new QMP/HMP commands to dump the status of a >>> virtio device at different levels. >>> >>> [Jonah: Rebasing previous patchset from Oct. 5 (v7). Original patches >>> are from Laurent Vivier from May 2020. >>> >>> Rebase from v7 to v8 includes an additional assert to make sure >>> we're not returning NULL in virtio_id_to_name(). Rebase also >>> includes minor additions/edits to qapi/virtio.json.] >>> >>> 1. Main command >>> >>> HMP Only: >>> >>> virtio [subcommand] >>> >>> Example: >>> >>> List all sub-commands: >>> >>> (qemu) virtio >>> virtio query -- List all available virtio devices >>> virtio status path -- Display status of a given virtio device >>> virtio queue-status path queue -- Display status of a given virtio queue >>> virtio vhost-queue-status path queue -- Display status of a given vhost queue >>> virtio queue-element path queue [index] -- Display element of a given virtio queue >> I don't see a compelling reason why these are setup as sub-commands >> under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query' >> naming just feels needlessly different from the current QEMU practices. >> >> IMHO they should just be "info" subcommands for HMP. ie >> >> info virtio -- List all available virtio devices >> info virtio-status path -- Display status of a given virtio device >> info virtio-queue-status path queue -- Display status of a given virtio queue >> info virtio-vhost-queue-status path queue -- Display status of a given vhost queue >> info virtio-queue-element path queue [index] -- Display element of a given virtio queue > I agree with Dan (but I'm not the maintainer). I do like this format a bit better than Dave's recommendation. Feels a bit more intuitive to understand what the commands should be doing, but I'm not sure if this is just because I'm new to these things. I'd like to format it like above if that's okay. > >> While the corresponding QMP commands ought to be >> >> x-query-virtio >> x-query-virtio-status >> x-query-virtio-queue-status >> x-query-virtio-vhost-queue-status >> x-query-virtio-queue-element > I agree with Dan (and I am the maintainer). > > The x- is not strictly required anymore (see commit a3c45b3e62 'qapi: > New special feature flag "unstable"'). I lean towards keeping it here, > because we don't plan to stabilize these commands. Ok! I'll keep the 'x-' in and change them to the above. Thank you for the comments!! Jonah