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 <lviv...@redhat.com>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilb...@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
