[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/2] virtio: introduce `query-virtio' QMP command |
Date: |
Fri, 06 Oct 2017 07:36:37 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Jan Dakinevich <address@hidden> 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 <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.
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.
>>> + }
>>> +}
>>> +
>>> +##
>>> +# @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'
>>> +}
>>>
>>