[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/2] virtio: introduce `query-vi
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/2] virtio: introduce `query-virtio' QMP command |
Date: |
Tue, 3 Oct 2017 17:29:22 +0100 |
User-agent: |
Mutt/1.9.0 (2017-09-02) |
* Jan Dakinevich (address@hidden) wrote:
>
>
> 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 <address@hidden>
> >> Signed-off-by: Jan Dakinevich <address@hidden>
> >> ---
> >> 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.
>
> >> + }
> >> +}
> >> +
> >> +##
> >> +# @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.
>
> >> + }
> >> +}
> >> +
> >> +##
> >> +# @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.
If you can turn these into enums (union'd enums?) then you might
be able to get rid of a lot of your array filling/naming conversion
boilerplate. (Not sure if it's worth it, but it's worth looking).
Dave
> >> + 'status-names': ['VirtioInfoBit'],
> >> + 'devices': ['VirtioInfoDevice']
> >> + }
> >> +}
> >> +
> >> +
> >> +##
> >> +# @query-virtio:
> >> +#
> >> +# Returns generic and per-device virtio information
> >> +#
> >> +# Since: 2.11.0
> >> +#
> >> +##
> >> +{
> >> + 'command': 'query-virtio',
> >> + 'returns': 'VirtioInfo'
> >> +}
> >>
> >
>
> --
> Best regards
> Jan Dakinevich
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK