* Markus Armbruster (arm...@redhat.com) wrote:
> Jan Dakinevich <jan.dakinev...@virtuozzo.com> writes:
>
> > On 10/03/2017 05:02 PM, Eric Blake wrote:
> >> On 10/03/2017 07:47 AM, Jan Dakinevich wrote:
> >>> The command is intended for gathering virtio information such as status,
> >>> feature bits, negotiation status. It is convenient and useful for debug
> >>> purpose.
> >>>
> >>> The commands returns generic virtio information for virtio such as
> >>> common feature names and status bits names and information for all
> >>> attached to current machine devices.
> >>>
> >>> To retrieve names of device-specific features `get_feature_name'
> >>> callback in VirtioDeviceClass also was introduced.
> >>>
> >>> Cc: Denis V. Lunev <d...@virtuozzo.com>
> >>> Signed-off-by: Jan Dakinevich <jan.dakinev...@virtuozzo.com>
> >>> ---
> >>> hw/block/virtio-blk.c | 21 +++++++++
> >>> hw/char/virtio-serial-bus.c | 15 +++++++
> >>> hw/display/virtio-gpu.c | 13 ++++++
> >>> hw/net/virtio-net.c | 35 +++++++++++++++
> >>> hw/scsi/virtio-scsi.c | 16 +++++++
> >>> hw/virtio/Makefile.objs | 2 +
> >>> hw/virtio/virtio-balloon.c | 15 +++++++
> >>> hw/virtio/virtio-stub.c | 9 ++++
> >>> hw/virtio/virtio.c | 101
> >>> ++++++++++++++++++++++++++++++++++++++++++++
> >>> include/hw/virtio/virtio.h | 2 +
> >>> qapi-schema.json | 1 +
> >>> qapi/virtio.json | 94
> >>> +++++++++++++++++++++++++++++++++++++++++
> >>> 12 files changed, 324 insertions(+)
> >>> create mode 100644 hw/virtio/virtio-stub.c
> >>> create mode 100644 qapi/virtio.json
> >>
> >> This creates a new .json file, but does not touch MAINTAINERS. Our idea
> >> in splitting the .json files was to make it easier for each sub-file
> >> that needs a specific maintainer in addition to the overall *.json line
> >> for QAPI maintainers, so this may deserve a MAINTAINERS entry.
> >>
> >
> > Ok.
> >
> >>> +++ b/qapi/virtio.json
> >>> @@ -0,0 +1,94 @@
> >>> +# -*- Mode: Python -*-
> >>> +#
> >>> +
> >>> +##
> >>> +# = Virtio devices
> >>> +##
> >>> +
> >>> +{ 'include': 'common.json' }
> >>> +
> >>> +##
> >>> +# @VirtioInfoBit:
> >>> +#
> >>> +# Named virtio bit
> >>> +#
> >>> +# @bit: bit number
> >>> +#
> >>> +# @name: bit name
> >>> +#
> >>> +# Since: 2.11.0
> >>> +#
> >>> +##
> >>> +{
> >>> + 'struct': 'VirtioInfoBit',
> >>> + 'data': {
> >>> + 'bit': 'uint64',
> >>
> >> Why is this a 64-bit value? Are the values 0-63, or are they 1, 2, 4, 8,
> >> ...? The documentation on 'bit number' is rather sparse.
> >
> > I would prefer `uint' here, but I don't see generic unsigned type (may
> > be, I am mistaken). I could use uint8 here, though.
> >
> >>
> >>> + 'name': 'str'
> >>
> >> Wouldn't an enum type be better than an open-ended string?
> >>
> >
> > Bit names are not known here, they are obtained from virtio device
> > implementations.
>
> What exactly uses these bits?
>
> Why do these uses justify pass-through? By pass-through, I mean the
> messenger (QEMU) merely passes them along, without understanding them.
> Defeats introspection.
It should be noted originally it was HMP - this was just a debug command
and it's only getting the need to be introspectable because people
insisted it had a QMP version.
I think the intent is to print all flags, even ones we dont yet
understand.
Dave
> >>> + }
> >>> +}
> >>> +
> >>> +##
> >>> +# @VirtioInfoDevice:
> >>> +#
> >>> +# Information about specific virtio device
> >>> +#
> >>> +# @qom_path: QOM path of the device
> >>
> >> Please make this 'qom-path' - new interfaces should prefer '-' over '_'.
> >
> > Ok.
> >
> >>> +#
> >>> +# @feature-names: names of device-specific features
> >>> +#
> >>> +# @host-features: bitmask of features, provided by devices
> >>> +#
> >>> +# @guest-features: bitmask of features, acknowledged by guest
> >>> +#
> >>> +# @status: virtio device status bitmask
> >>> +#
> >>> +# Since: 2.11.0
> >>> +#
> >>> +##
> >>> +{
> >>> + 'struct': 'VirtioInfoDevice',
> >>> + 'data': {
> >>> + 'qom_path': 'str',
> >>> + 'feature-names': ['VirtioInfoBit'],
> >>> + 'host-features': 'uint64',
> >>> + 'guest-features': 'uint64',
> >>> + 'status': 'uint64'
> >>
> >> I'm wondering if this is the best representation (where the caller has
> >> to parse the integer and then lookup in feature-names what each bit of
> >> the integer represents). But I'm not sure I have anything better off
> >> the top of my head.
> >>
> >
> > Consider it as way to tell caller about names of supported features.
>
> "Unsigned integer interpreted as combination of well-known bit-valued
> symbols" is a fine C interface, but a pretty horrid QMP interface.
> What's wrong with doing a set the straightforward way as "array of
> enum"?
>
> >>> + }
> >>> +}
> >>> +
> >>> +##
> >>> +# @VirtioInfo:
> >>> +#
> >>> +# Information about virtio devices
> >>> +#
> >>> +# @feature-names: names of common virtio features
> >>> +#
> >>> +# @status-names: names of bits which represents virtio device status
> >>> +#
> >>> +# @devices: list of per-device virtio information
> >>> +#
> >>> +# Since: 2.11.0
> >>> +#
> >>> +##
> >>> +{
> >>> + 'struct': 'VirtioInfo',
> >>> + 'data': {
> >>> + 'feature-names': ['VirtioInfoBit'],
> >>
> >> Why is feature-names listed at two different nestings of the return value?
> >>
> >
> > These are different feature names. First names are common and predefined
> > for all devices. Second names are device-specific.
> >
> >>> + 'status-names': ['VirtioInfoBit'],
> >>> + 'devices': ['VirtioInfoDevice']
> >>> + }
> >>> +}
> >>> +
> >>> +
> >>> +##
> >>> +# @query-virtio:
> >>> +#
> >>> +# Returns generic and per-device virtio information
> >>> +#
> >>> +# Since: 2.11.0
> >>> +#
> >>> +##
> >>> +{
> >>> + 'command': 'query-virtio',
> >>> + 'returns': 'VirtioInfo'
> >>> +}
> >>>
> >>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
