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

[Laurent Vivier <lvivier@redhat.com>]

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.

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