subject:"Re\: \[RFC v4 6\/6\] hmp\: add x\-debug\-virtio commands"

Re: [RFC v4 6/6] hmp: add x-debug-virtio commands

2020-05-15 Thread Laurent Vivier

On 15/05/2020 17:45, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (lviv...@redhat.com) wrote:
>> On 13/05/2020 12:51, Dr. David Alan Gilbert wrote:
>>> * Laurent Vivier (lviv...@redhat.com) wrote:
 This patch implements HMP version of the virtio QMP commands

 Signed-off-by: Laurent Vivier 
>>>
>>> Reviewed-by: Dr. David Alan Gilbert 
>>>
>>> With a thought below
>>>
 ---
  Makefile|   2 +-
  Makefile.target |   7 +-
  docs/system/monitor.rst |   2 +
  hmp-commands-virtio.hx  | 160 +
  hmp-commands.hx |  10 +++
  hw/virtio/virtio.c  | 193 +++-
  include/monitor/hmp.h   |   4 +
  monitor/misc.c  |  17 
  8 files changed, 391 insertions(+), 4 deletions(-)
  create mode 100644 hmp-commands-virtio.hx

>> ...
 diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
 index 66dc2cef1b39..c3d6b783417e 100644
 --- a/hw/virtio/virtio.c
 +++ b/hw/virtio/virtio.c
>> ...
 @@ -4033,6 +4092,92 @@ VirtioStatus *qmp_x_debug_virtio_status(const char* 
 path, Error **errp)
  return status;
  }
  
 +#define DUMP_FEATURES(type, field)
  \
 +do {  
  \
 +type##FeatureList *list = features->device->u.field.data; 
  \
 +if (list) {   
  \
 +monitor_printf(mon, "");  
  \
 +while (list) {
  \
 +monitor_printf(mon, "%s", 
 type##Feature_str(list->value)); \
 +list = list->next;
  \
 +if (list != NULL) {   
  \
 +monitor_printf(mon, ", ");
  \
 +} 
  \
 +} 
  \
 +monitor_printf(mon, "\n");
  \
 +} 
  \
 +} while (0)
>>>
>>> It feels like you should be able to have an array of Feature_str's
>>> indexed by VIRTIO_DEVICE_FEATURE_KIND_ enum, so that when a new
>>> VIRTIO_DEVICE_FEATURE_KIND is added you don't need to fix this up.
>>
>> I don't understand what you mean here.
> 
> Instead of the switch below, I'm thinking you could have something like:
> 
> if (features->device->type < something_MAX) {
> features_str = anarray[features->device->type];
> 
> 
> monitor_printf(mon, "%s", features_str(list->value));
> 
> }
> 
> with 'anarray' somewhere more central, so we don't have to keep
> these switch structures and macros spread around.

OK, I tried that, but in fact we need to know the type of the list to be
able to scan it (the "type##FeatureList": VirtoSerialFeatureList,
VirtioBlkFeatureList, ...), except if we introduce a generic feature
list node (for "next " and "value"). But it becomes more complicated,
because we also need to generate the "anarray" somewhere.

[Note: I've changed the legacy enum to a flat enum as proposed by Eric
in v3]

Thanks,
Laurent

Re: [RFC v4 6/6] hmp: add x-debug-virtio commands

2020-05-15 Thread Dr. David Alan Gilbert

* Laurent Vivier (lviv...@redhat.com) wrote:
> On 13/05/2020 12:51, Dr. David Alan Gilbert wrote:
> > * Laurent Vivier (lviv...@redhat.com) wrote:
> >> This patch implements HMP version of the virtio QMP commands
> >>
> >> Signed-off-by: Laurent Vivier 
> > 
> > Reviewed-by: Dr. David Alan Gilbert 
> > 
> > With a thought below
> > 
> >> ---
> >>  Makefile|   2 +-
> >>  Makefile.target |   7 +-
> >>  docs/system/monitor.rst |   2 +
> >>  hmp-commands-virtio.hx  | 160 +
> >>  hmp-commands.hx |  10 +++
> >>  hw/virtio/virtio.c  | 193 +++-
> >>  include/monitor/hmp.h   |   4 +
> >>  monitor/misc.c  |  17 
> >>  8 files changed, 391 insertions(+), 4 deletions(-)
> >>  create mode 100644 hmp-commands-virtio.hx
> >>
> ...
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 66dc2cef1b39..c3d6b783417e 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> ...
> >> @@ -4033,6 +4092,92 @@ VirtioStatus *qmp_x_debug_virtio_status(const char* 
> >> path, Error **errp)
> >>  return status;
> >>  }
> >>  
> >> +#define DUMP_FEATURES(type, field)
> >>  \
> >> +do {  
> >>  \
> >> +type##FeatureList *list = features->device->u.field.data; 
> >>  \
> >> +if (list) {   
> >>  \
> >> +monitor_printf(mon, "");  
> >>  \
> >> +while (list) {
> >>  \
> >> +monitor_printf(mon, "%s", 
> >> type##Feature_str(list->value)); \
> >> +list = list->next;
> >>  \
> >> +if (list != NULL) {   
> >>  \
> >> +monitor_printf(mon, ", ");
> >>  \
> >> +} 
> >>  \
> >> +} 
> >>  \
> >> +monitor_printf(mon, "\n");
> >>  \
> >> +} 
> >>  \
> >> +} while (0)
> > 
> > It feels like you should be able to have an array of Feature_str's
> > indexed by VIRTIO_DEVICE_FEATURE_KIND_ enum, so that when a new
> > VIRTIO_DEVICE_FEATURE_KIND is added you don't need to fix this up.
> 
> I don't understand what you mean here.

Instead of the switch below, I'm thinking you could have something like:

if (features->device->type < something_MAX) {
features_str = anarray[features->device->type];


monitor_printf(mon, "%s", features_str(list->value));

}

with 'anarray' somewhere more central, so we don't have to keep
these switch structures and macros spread around.

Dave

> >> +
> >> +static void hmp_virtio_dump_features(Monitor *mon,
> >> + VirtioStatusFeatures *features)
> >> +{
> >> +VirtioTransportFeatureList *transport_list = features->transport;
> >> +while (transport_list) {
> >> +monitor_printf(mon, "%s",
> >> +   VirtioTransportFeature_str(transport_list->value));
> >> +transport_list = transport_list->next;
> >> +if (transport_list != NULL) {
> >> +monitor_printf(mon, ", ");
> >> +}
> >> +}
> >> +monitor_printf(mon, "\n");
> >> +switch (features->device->type) {
> >> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_SERIAL:
> >> +DUMP_FEATURES(VirtioSerial, virtio_serial);
> >> +break;
> >> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_BLK:
> >> +DUMP_FEATURES(VirtioBlk, virtio_blk);
> >> +break;
> >> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_GPU:
> >> +DUMP_FEATURES(VirtioGpu, virtio_gpu);
> >> +break;
> >> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_NET:
> >> +DUMP_FEATURES(VirtioNet, virtio_net);
> >> +break;
> >> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_SCSI:
> >> +DUMP_FEATURES(VirtioScsi, virtio_scsi);
> >> +break;
> >> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_BALLOON:
> >> +DUMP_FEATURES(VirtioBalloon, virtio_balloon);
> >> +break;
> >> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_IOMMU:
> >> +DUMP_FEATURES(VirtioIommu, virtio_iommu);
> >> +break;
> >> +default:
> >> +g_assert_not_reached();
> >> +}
> >> +if (features->unknown) {
> >> +monitor_printf(mon, "
> >> unknown(0x%016"PRIx64")\n", \
> >> +   features->unknown);
> >> +}
> >> +}
> ...
> 
> Thanks,
> Laurent
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester,

Re: [RFC v4 6/6] hmp: add x-debug-virtio commands

2020-05-15 Thread Laurent Vivier

On 13/05/2020 12:51, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (lviv...@redhat.com) wrote:
>> This patch implements HMP version of the virtio QMP commands
>>
>> Signed-off-by: Laurent Vivier 
> 
> Reviewed-by: Dr. David Alan Gilbert 
> 
> With a thought below
> 
>> ---
>>  Makefile|   2 +-
>>  Makefile.target |   7 +-
>>  docs/system/monitor.rst |   2 +
>>  hmp-commands-virtio.hx  | 160 +
>>  hmp-commands.hx |  10 +++
>>  hw/virtio/virtio.c  | 193 +++-
>>  include/monitor/hmp.h   |   4 +
>>  monitor/misc.c  |  17 
>>  8 files changed, 391 insertions(+), 4 deletions(-)
>>  create mode 100644 hmp-commands-virtio.hx
>>
...
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 66dc2cef1b39..c3d6b783417e 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
...
>> @@ -4033,6 +4092,92 @@ VirtioStatus *qmp_x_debug_virtio_status(const char* 
>> path, Error **errp)
>>  return status;
>>  }
>>  
>> +#define DUMP_FEATURES(type, field) \
>> +do {   \
>> +type##FeatureList *list = features->device->u.field.data;  \
>> +if (list) {\
>> +monitor_printf(mon, "");   \
>> +while (list) { \
>> +monitor_printf(mon, "%s", type##Feature_str(list->value)); \
>> +list = list->next; \
>> +if (list != NULL) {\
>> +monitor_printf(mon, ", "); \
>> +}  \
>> +}  \
>> +monitor_printf(mon, "\n"); \
>> +}  \
>> +} while (0)
> 
> It feels like you should be able to have an array of Feature_str's
> indexed by VIRTIO_DEVICE_FEATURE_KIND_ enum, so that when a new
> VIRTIO_DEVICE_FEATURE_KIND is added you don't need to fix this up.

I don't understand what you mean here.

>> +
>> +static void hmp_virtio_dump_features(Monitor *mon,
>> + VirtioStatusFeatures *features)
>> +{
>> +VirtioTransportFeatureList *transport_list = features->transport;
>> +while (transport_list) {
>> +monitor_printf(mon, "%s",
>> +   VirtioTransportFeature_str(transport_list->value));
>> +transport_list = transport_list->next;
>> +if (transport_list != NULL) {
>> +monitor_printf(mon, ", ");
>> +}
>> +}
>> +monitor_printf(mon, "\n");
>> +switch (features->device->type) {
>> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_SERIAL:
>> +DUMP_FEATURES(VirtioSerial, virtio_serial);
>> +break;
>> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_BLK:
>> +DUMP_FEATURES(VirtioBlk, virtio_blk);
>> +break;
>> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_GPU:
>> +DUMP_FEATURES(VirtioGpu, virtio_gpu);
>> +break;
>> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_NET:
>> +DUMP_FEATURES(VirtioNet, virtio_net);
>> +break;
>> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_SCSI:
>> +DUMP_FEATURES(VirtioScsi, virtio_scsi);
>> +break;
>> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_BALLOON:
>> +DUMP_FEATURES(VirtioBalloon, virtio_balloon);
>> +break;
>> +case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_IOMMU:
>> +DUMP_FEATURES(VirtioIommu, virtio_iommu);
>> +break;
>> +default:
>> +g_assert_not_reached();
>> +}
>> +if (features->unknown) {
>> +monitor_printf(mon, "
>> unknown(0x%016"PRIx64")\n", \
>> +   features->unknown);
>> +}
>> +}
...

Thanks,
Laurent

Re: [RFC v4 6/6] hmp: add x-debug-virtio commands

2020-05-13 Thread Dr. David Alan Gilbert

* Laurent Vivier (lviv...@redhat.com) wrote:
> This patch implements HMP version of the virtio QMP commands
> 
> Signed-off-by: Laurent Vivier 

Reviewed-by: Dr. David Alan Gilbert 

With a thought below

> ---
>  Makefile|   2 +-
>  Makefile.target |   7 +-
>  docs/system/monitor.rst |   2 +
>  hmp-commands-virtio.hx  | 160 +
>  hmp-commands.hx |  10 +++
>  hw/virtio/virtio.c  | 193 +++-
>  include/monitor/hmp.h   |   4 +
>  monitor/misc.c  |  17 
>  8 files changed, 391 insertions(+), 4 deletions(-)
>  create mode 100644 hmp-commands-virtio.hx
> 
> diff --git a/Makefile b/Makefile
> index 34275f57c9cb..feb300ebb2d4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1099,7 +1099,7 @@ $(MANUAL_BUILDDIR)/interop/index.html: $(call 
> manual-deps,interop)
>  $(MANUAL_BUILDDIR)/specs/index.html: $(call manual-deps,specs)
>   $(call build-manual,specs,html)
>  
> -$(MANUAL_BUILDDIR)/system/index.html: $(call manual-deps,system) 
> $(SRC_PATH)/hmp-commands.hx $(SRC_PATH)/hmp-commands-info.hx 
> $(SRC_PATH)/qemu-options.hx
> +$(MANUAL_BUILDDIR)/system/index.html: $(call manual-deps,system) 
> $(SRC_PATH)/hmp-commands.hx $(SRC_PATH)/hmp-commands-info.hx 
> $(SRC_PATH)/qemu-options.hx $(SRC_PATH)/hmp-commands-virtio.hx
>   $(call build-manual,system,html)
>  
>  $(MANUAL_BUILDDIR)/tools/index.html: $(call manual-deps,tools) 
> $(SRC_PATH)/qemu-img-cmds.hx $(SRC_PATH)/docs/qemu-option-trace.rst.inc
> diff --git a/Makefile.target b/Makefile.target
> index 8ed1eba95b9c..66d3ff9bc350 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -171,7 +171,7 @@ else
>  obj-y += hw/$(TARGET_BASE_ARCH)/
>  endif
>  
> -generated-files-y += hmp-commands.h hmp-commands-info.h
> +generated-files-y += hmp-commands.h hmp-commands-info.h hmp-commands-virtio.h
>  generated-files-y += config-devices.h
>  
>  endif # CONFIG_SOFTMMU
> @@ -220,10 +220,13 @@ hmp-commands.h: $(SRC_PATH)/hmp-commands.hx 
> $(SRC_PATH)/scripts/hxtool
>  hmp-commands-info.h: $(SRC_PATH)/hmp-commands-info.hx 
> $(SRC_PATH)/scripts/hxtool
>   $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > 
> $@,"GEN","$(TARGET_DIR)$@")
>  
> +hmp-commands-virtio.h: $(SRC_PATH)/hmp-commands-virtio.hx 
> $(SRC_PATH)/scripts/hxtool
> + $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > 
> $@,"GEN","$(TARGET_DIR)$@")
> +
>  clean: clean-target
>   rm -f *.a *~ $(PROGS)
>   rm -f $(shell find . -name '*.[od]')
> - rm -f hmp-commands.h gdbstub-xml.c
> + rm -f hmp-commands.h hmp-commands-virtio.h gdbstub-xml.c
>   rm -f trace/generated-helpers.c trace/generated-helpers.c-timestamp
>  ifdef CONFIG_TRACE_SYSTEMTAP
>   rm -f *.stp
> diff --git a/docs/system/monitor.rst b/docs/system/monitor.rst
> index 0bcd5da21644..985c3f51ffe7 100644
> --- a/docs/system/monitor.rst
> +++ b/docs/system/monitor.rst
> @@ -21,6 +21,8 @@ The following commands are available:
>  
>  .. hxtool-doc:: hmp-commands.hx
>  
> +.. hxtool-doc:: hmp-commands-virtio.hx
> +
>  .. hxtool-doc:: hmp-commands-info.hx
>  
>  Integer expressions
> diff --git a/hmp-commands-virtio.hx b/hmp-commands-virtio.hx
> new file mode 100644
> index ..14cb14bfcc70
> --- /dev/null
> +++ b/hmp-commands-virtio.hx
> @@ -0,0 +1,160 @@
> +HXCOMM Use DEFHEADING() to define headings in both help text and rST.
> +HXCOMM Text between SRST and ERST is copied to the rST version and
> +HXCOMM discarded from C version.
> +HXCOMM DEF(command, args, callback, arg_string, help) is used to construct
> +HXCOMM monitor info commands
> +HXCOMM HXCOMM can be used for comments, discarded from both rST and C.
> +HXCOMM
> +HXCOMM In this file, generally SRST fragments should have two extra
> +HXCOMM spaces of indent, so that the documentation list item for 
> "x-debug-virtio cmd"
> +HXCOMM appears inside the documentation list item for the top level
> +HXCOMM "x-debug-virtio" documentation entry. The exception is the first SRST
> +HXCOMM fragment that defines that top level entry.
> +
> +SRST
> +``x-debug-virtio`` *subcommand*
> +  Show various information about virtio.
> +
> +  Example:
> +
> +  List all sub-commands::
> +
> +(qemu) x-debug-virtio
> +x-debug-virtio query  -- List all available virtio devices
> +x-debug-virtio status path -- Display status of a given virtio device
> +x-debug-virtio queue-status path queue -- Display status of a given 
> virtio queue
> +x-debug-virtio queue-element path queue [index] -- Display element of a 
> given virtio queue
> +
> +ERST
> +
> +{
> +.name   = "query",
> +.args_type  = "",
> +.params = "",
> +.help   = "List all available virtio devices",
> +.cmd= hmp_x_debug_virtio_query,
> +.flags  = "p",
> +},
> +
> +SRST
> +  ``x-debug-virtio query``
> +List all available virtio devices
> +
> +Example:
> +
> +List all

Re: [RFC v4 6/6] hmp: add x-debug-virtio commands

Re: [RFC v4 6/6] hmp: add x-debug-virtio commands

Re: [RFC v4 6/6] hmp: add x-debug-virtio commands

Re: [RFC v4 6/6] hmp: add x-debug-virtio commands

4 matches

Site Navigation

Mail list logo

Footer information