qemu-block
[Top][All Lists]
Advanced

[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: Jan Dakinevich
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/2] virtio: introduce `query-virtio' QMP command
Date: Wed, 4 Oct 2017 17:26:58 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0


On 10/03/2017 07:29 PM, Dr. David Alan Gilbert wrote:
> * 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).
> 

I would be happy to drop this boilerplate, but how enum could help here?
To respond my requirement it should be something like set, not enum.
Even so, having set, I would have been needed to declare mapping between
names in set type and bit numbers within feature bitmask.

> 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
> 

-- 
Best regards
Jan Dakinevich



reply via email to

[Prev in Thread] Current Thread [Next in Thread]