[Qemu-devel] Re: [PATCH 4/8] monitor: Add basic device state visualization

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 14996 bytes --]

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> This introduces device_show, a monitor command that saves the vmstate of
>> a qdev device and visualizes it. QMP is also supported. Buffers are cut
>> after 16 byte by default, but the full content can be requested via
>> '-f'. To pretty-print sub-arrays, vmstate is extended to store the start
>> index name.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Neato!  Comments inline.
> 
>> ---
>>  hw/hw.h         |    2 +
>>  hw/qdev.c       |  287 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/qdev.h       |    2 +
>>  qemu-monitor.hx |   16 +++
>>  4 files changed, 307 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/hw.h b/hw/hw.h
>> index 328b704..1ff8e40 100644
>> --- a/hw/hw.h
>> +++ b/hw/hw.h
>> @@ -299,6 +299,7 @@ enum VMStateFlags {
>>  
>>  typedef struct {
>>      const char *name;
>> +    const char *start_index;
>>      size_t offset;
>>      size_t size;
>>      size_t start;
> 
> Why is start_index a string?

It can, actually should be a symbolic constant (from e1000:
"mac_reg[RA+03]: ...", ie. "RA" is the start index here).

> 
>> @@ -414,6 +415,7 @@ extern const VMStateInfo vmstate_info_unused_buffer;
>>      .size       = sizeof(_type),                                     \
>>      .flags      = VMS_ARRAY,                                         \
>>      .offset     = vmstate_offset_sub_array(_state, _field, _type, _start), \
>> +    .start_index = (stringify(_start)),                              \
>>  }
>>  
>>  #define VMSTATE_VARRAY_INT32(_field, _state, _field_num, _version, _info, _type) {\
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index fe49e71..c989010 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -29,6 +29,9 @@
>>  #include "qdev.h"
>>  #include "sysemu.h"
>>  #include "monitor.h"
>> +#include "qjson.h"
>> +#include "qint.h"
>> +#include "qbuffer.h"
>>  
>>  static int qdev_hotplug = 0;
>>  
>> @@ -824,3 +827,287 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>      }
>>      return qdev_unplug(dev);
>>  }
>> +
>> +static int print_array_index(Monitor *mon, const char *start, int index)
>> +{
>> +    char index_str[32];
>> +    int len;
>> +
>> +    if (start) {
>> +        len = snprintf(index_str, sizeof(index_str), "[%s+%02x]", start,
>> +                       index);
>> +    } else {
>> +        len = snprintf(index_str, sizeof(index_str), "[%02x]", index);
>> +    }
>> +    monitor_printf(mon, index_str);
>> +    return len;
> 
> You go via the snprintf() detour just because monitor_printf() doesn't
> return the number of characters printed.  Wouldn't it be better to fix
> that?

Yeah, good point. Will address this.

> 
>> +}
>> +
>> +#define NAME_COLUMN_WIDTH 23
>> +
>> +static void print_field(Monitor *mon, const QDict *qfield, int indent);
>> +
>> +static void print_elem(Monitor *mon, const QObject *qelem, size_t size,
>> +                       int column_pos, int indent)
>> +{
>> +    int64_t data_size;
>> +    const void *data;
>> +    int n;
>> +
>> +    if (qobject_type(qelem) == QTYPE_QDICT) {
>> +        if (column_pos >= 0) {
>> +            monitor_printf(mon, ".\n");
>> +        }
>> +    } else {
>> +        monitor_printf(mon, ":");
>> +        column_pos++;
>> +        if (column_pos < NAME_COLUMN_WIDTH) {
>> +            monitor_printf(mon, "%*c", NAME_COLUMN_WIDTH - column_pos, ' ');
>> +        }
>> +    }
>> +
>> +    switch (qobject_type(qelem)) {
>> +    case QTYPE_QDICT:
>> +        print_field(mon, qobject_to_qdict(qelem), indent + 2);
>> +        break;
>> +    case QTYPE_QBUFFER:
>> +        data = qbuffer_get_data(qobject_to_qbuffer(qelem));
>> +        data_size = qbuffer_get_size(qobject_to_qbuffer(qelem));
>> +        for (n = 0; n < data_size; ) {
>> +            monitor_printf(mon, " %02x", *((uint8_t *)data+n));
>> +            if (++n < size) {
>> +                if (n % 16 == 0) {
>> +                    monitor_printf(mon, "\n%*c", NAME_COLUMN_WIDTH, ' ');
>> +                } else if (n % 8 == 0) {
>> +                    monitor_printf(mon, " -");
>> +                }
>> +            }
>> +        }
>> +        if (data_size < size) {
>> +            monitor_printf(mon, " ...");
>> +        }
>> +        monitor_printf(mon, "\n");
>> +        break;
>> +    case QTYPE_QINT:
>> +        monitor_printf(mon, " %0*x\n", (int)size * 2,
>> +                       (int)qint_get_int(qobject_to_qint(qelem)));
> 
> I'm confused.  Doesn't casting qint_get_int() to int lose bits?

You are right, will fix!

> 
>> +        break;
>> +    default:
>> +        assert(0);
>> +    }
>> +}
>> +
>> +static void print_field(Monitor *mon, const QDict *qfield, int indent)
>> +{
>> +    const char *name = qdict_get_str(qfield, "name");
>> +    const char *start = qdict_get_try_str(qfield, "start");
>> +    int64_t size = qdict_get_int(qfield, "size");
>> +    QList *qlist = qdict_get_qlist(qfield, "elems");
>> +    QListEntry *entry, *sub_entry;
>> +    QList *sub_list;
>> +    int elem_no = 0;
>> +
>> +    QLIST_FOREACH_ENTRY(qlist, entry) {
>> +        QObject *qelem = qlist_entry_obj(entry);
>> +        int pos = indent + strlen(name);
>> +
>> +        if (qobject_type(qelem) == QTYPE_QLIST) {
>> +            monitor_printf(mon, "%*c%s", indent, ' ', name);
>> +            pos += print_array_index(mon, start, elem_no);
>> +            sub_list = qobject_to_qlist(qelem);
>> +            QLIST_FOREACH_ENTRY(sub_list, sub_entry) {
>> +                print_elem(mon, qlist_entry_obj(sub_entry), size, pos,
>> +                           indent + 2);
>> +                pos = -1;
>> +            }
>> +        } else {
>> +            if (elem_no == 0) {
>> +                monitor_printf(mon, "%*c%s", indent, ' ', name);
>> +            } else {
>> +                pos = -1;
>> +            }
>> +            print_elem(mon, qelem, size, pos, indent);
>> +        }
>> +        elem_no++;
>> +    }
>> +}
>> +
>> +void device_user_print(Monitor *mon, const QObject *data)
>> +{
>> +    QDict *qdict = qobject_to_qdict(data);
>> +    QList *qlist = qdict_get_qlist(qdict, "fields");
>> +    QListEntry *entry;
>> +
>> +    monitor_printf(mon, "dev: %s, id \"%s\"\n",
>> +                   qdict_get_str(qdict, "device"),
>> +                   qdict_get_str(qdict, "id"));
>> +
>> +    QLIST_FOREACH_ENTRY(qlist, entry) {
>> +        print_field(mon, qobject_to_qdict(qlist_entry_obj(entry)), 2);
>> +    }
>> +}
>> +
>> +static void parse_vmstate(const VMStateDescription *vmsd, void *opaque,
>> +                          QList *qlist, int full_buffers)
>> +{
>> +    VMStateField *field = vmsd->fields;
>> +
>> +    if (vmsd->pre_save) {
>> +        vmsd->pre_save(opaque);
>> +    }
>> +    while(field->name) {
>> +        if (!field->field_exists ||
>> +            field->field_exists(opaque, vmsd->version_id)) {
>> +            void *base_addr = opaque + field->offset;
>> +            int i, n_elems = 1;
>> +            int is_array = 1;
>> +            size_t size = field->size;
>> +            QDict *qfield = qdict_new();
>> +            QList *qelems = qlist_new();
>> +
>> +            qlist_append_obj(qlist, QOBJECT(qfield));
>> +
>> +            qdict_put_obj(qfield, "name",
>> +                          QOBJECT(qstring_from_str(field->name)));
>> +            qdict_put_obj(qfield, "elems", QOBJECT(qelems));
>> +
>> +            if (field->flags & VMS_VBUFFER) {
>> +                size = *(int32_t *)(opaque + field->size_offset);
>> +                if (field->flags & VMS_MULTIPLY) {
>> +                    size *= field->size;
>> +                }
>> +            }
>> +            qdict_put_obj(qfield, "size", QOBJECT(qint_from_int(size)));
>> +            if (field->start_index) {
>> +                qdict_put_obj(qfield, "start",
>> +                              QOBJECT(qstring_from_str(field->start_index)));
>> +            }
>> +
>> +            if (field->flags & VMS_ARRAY) {
>> +                n_elems = field->num;
>> +            } else if (field->flags & VMS_VARRAY_INT32) {
>> +                n_elems = *(int32_t *)(opaque + field->num_offset);
>> +            } else if (field->flags & VMS_VARRAY_UINT16) {
>> +                n_elems = *(uint16_t *)(opaque + field->num_offset);
>> +            } else {
>> +                is_array = 0;
>> +            }
>> +            if (field->flags & VMS_POINTER) {
>> +                base_addr = *(void **)base_addr + field->start;
>> +            }
>> +            for (i = 0; i < n_elems; i++) {
>> +                void *addr = base_addr + size * i;
>> +                QList *sub_elems = qelems;
>> +                int val;
>> +
>> +                if (is_array) {
>> +                    sub_elems = qlist_new();
>> +                    qlist_append_obj(qelems, QOBJECT(sub_elems));
>> +                }
>> +                if (field->flags & VMS_ARRAY_OF_POINTER) {
>> +                    addr = *(void **)addr;
>> +                }
>> +                if (field->flags & VMS_STRUCT) {
>> +                    parse_vmstate(field->vmsd, addr, sub_elems, full_buffers);
>> +                } else {
>> +                    if (field->flags & (VMS_BUFFER | VMS_VBUFFER)) {
>> +                        if (!full_buffers && size > 16) {
>> +                            size = 16;
>> +                        }
>> +                        qlist_append_obj(sub_elems,
>> +                                         QOBJECT(qbuffer_from_data(addr,
>> +                                                                   size)));
>> +                    } else {
>> +                        switch (size) {
>> +                        case 1:
>> +                            val = *(uint8_t *)addr;
>> +                            break;
>> +                        case 2:
>> +                            val = *(uint16_t *)addr;
>> +                            break;
>> +                        case 4:
>> +                            val = *(uint32_t *)addr;
>> +                            break;
>> +                        case 8:
>> +                            val = *(uint64_t *)addr;
>> +                            break;
>> +                        default:
>> +                            assert(0);
>> +                        }
>> +                        qlist_append_obj(sub_elems,
>> +                                         QOBJECT(qint_from_int(val)));
>> +                    }
>> +                }
>> +            }
>> +        }
>> +        field++;
>> +    }
>> +}
>> +
>> +static DeviceState *qdev_find(const char *path)
>> +{
>> +    const char *dev_name;
>> +    DeviceState *dev;
>> +    char *bus_path;
>> +    BusState *bus;
>> +
>> +    dev_name = strrchr(path, '/');
>> +    if (!dev_name) {
>> +        bus = main_system_bus;
>> +        dev_name = path;
>> +    } else {
>> +        dev_name++;
>> +        bus_path = qemu_strdup(path);
>> +        bus_path[dev_name - path] = 0;
>> +
>> +        bus = qbus_find(bus_path);
>> +        qemu_free(bus_path);
>> +
>> +        if (!bus) {
>> +            /* qbus_find already reported the error */
>> +            return NULL;
>> +        }
>> +    }
>> +    dev = qbus_find_dev(bus, dev_name);
>> +    if (!dev) {
>> +        qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
>> +        if (!monitor_cur_is_qmp()) {
>> +            qbus_list_dev(bus);
>> +        }
>> +    }
>> +    return dev;
>> +}
>> +
>> +int do_device_show(Monitor *mon, const QDict *qdict, QObject **ret_data)
> 
> Command documentation missing.

In the make (depends on QMP doc infrastructure).

> 
>> +{
>> +    const char *path = qdict_get_str(qdict, "path");
>> +    const VMStateDescription *vmsd;
>> +    DeviceState *dev;
>> +    QList *qlist;
>> +
>> +    dev = qdev_find_recursive(main_system_bus, path);
>> +    if (!dev) {
>> +        dev = qdev_find(path);
>> +        if (!dev) {
>> +            return -1;
>> +        }
>> +    }
> 
> This is inconsistent with how do_device_del() looks up devices by ID.
> Let's agree on one lookup, put it into a function, and use it
> everywhere.

Will adjust device_del to the same scheme.

BTW, qdev path completion will also be available for both commands in my
next series.

> 
>> +
>> +    vmsd = dev->info->vmsd;
>> +    if (!vmsd) {
>> +        qerror_report(QERR_UNDEFINED_ERROR);
>> +        if (!monitor_cur_is_qmp()) {
>> +            error_printf_unless_qmp("Device '%s' does not support state "
>> +                                    "dumping\n", path);
>> +        }
> 
> Please use a suitable QERR_.  Define one if necessary.

Already fixed in my tree (forgot this bit for v1). Just this redundant
qmp check was still present.

> 
>> +        return -1;
>> +    }
>> +
>> +    *ret_data = qobject_from_jsonf("{ 'device': %s, 'id': %s }",
>> +                                   dev->info->name, dev->id ? : "");
>> +    qlist = qlist_new();
>> +    parse_vmstate(vmsd, dev, qlist, qdict_get_int(qdict, "full"));
>> +    qdict_put_obj(qobject_to_qdict(*ret_data), "fields", QOBJECT(qlist));
>> +
>> +    return 0;
>> +}
>> diff --git a/hw/qdev.h b/hw/qdev.h
>> index d8fbc73..f8436ec 100644
>> --- a/hw/qdev.h
>> +++ b/hw/qdev.h
>> @@ -177,6 +177,8 @@ void do_info_qtree(Monitor *mon);
>>  void do_info_qdm(Monitor *mon);
>>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
>>  int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>> +void device_user_print(Monitor *mon, const QObject *data);
>> +int do_device_show(Monitor *mon, const QDict *qdict, QObject **ret_data);
>>  
>>  /*** qdev-properties.c ***/
>>  
>> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
>> index a8f194c..449f012 100644
>> --- a/qemu-monitor.hx
>> +++ b/qemu-monitor.hx
>> @@ -599,6 +599,22 @@ Remove device @var{id}.
>>  ETEXI
>>  
>>      {
>> +        .name       = "device_show",
>> +        .args_type  = "path:s,full:-f",
>> +        .params     = "device [-f]",
>> +        .help       = "show device state (specify -f for full buffer dumping)",
>> +        .user_print = device_user_print,
>> +        .mhandler.cmd_new = do_device_show,
>> +    },
>> +
>> +STEXI
>> +@item device_show @var{id} [@code{-f}]
>> +
>> +Show state of device @var{id} in a human-readable form. Buffers are cut after
>> +16 bytes unless a full dump is requested via @code{-f} 
>> +ETEXI
>> +
>> +    {
>>          .name       = "cpu",
>>          .args_type  = "index:i",
>>          .params     = "index",
> 
> 

Thanks for the comments!

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]