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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Laurent Vivier (lvivier@redhat.com) wrote:
> On 13/05/2020 12:51, Dr. David Alan Gilbert wrote:
> > * Laurent Vivier (lvivier@redhat.com) wrote:
> >> This patch implements HMP version of the virtio QMP commands
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > 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 / dgilbert@redhat.com / Manchester, UK