On 10/03/2017 07:29 PM, Dr. David Alan Gilbert wrote: > * Jan Dakinevich (jan.dakinev...@virtuozzo.com) 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 <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. >> >>>> + } >>>> +} >>>> + >>>> +## >>>> +# @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 / dgilb...@redhat.com / Manchester, UK > -- Best regards Jan Dakinevich