Re: [Qemu-devel] [PATCH 3/5] PCI: Convert pci_info() to QObject

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> The returned QObject is a QList of all buses. Each bus is
> represented by a QDict, which has a key with a QList of all
> PCI devices attached to it. Each device is represented by
> a QDict.
>
> IMPORTANT: support for printing PCI bridge information and
> its devices is NOT part of this commit, it's going to be
> added by next commits.
>
> Finally, as has happended to other complex conversions,
> it's hard to split this commit as part of it are new
> functions which are called by each other.
>
> Signed-off-by: Luiz Capitulino <address@hidden>

Looks pretty good.  A few remarks follow inline.

> ---
>  hw/pci.c  |  308 
> +++++++++++++++++++++++++++++++++++++++++++++----------------
>  hw/pci.h  |    4 +-
>  monitor.c |    3 +-
>  3 files changed, 234 insertions(+), 81 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 08e51f8..8275ceb 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -27,6 +27,7 @@
>  #include "net.h"
>  #include "sysemu.h"
>  #include "loader.h"
> +#include "qemu-objects.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -1075,119 +1076,268 @@ static const pci_class_desc 
> pci_class_descriptions[] =
>      { 0, NULL}
>  };
>  
> -static void pci_info_device(PCIBus *bus, PCIDevice *d)
> +static void pci_for_each_device_under_bus(PCIBus *bus,
> +                                          void (*fn)(PCIBus *b, PCIDevice 
> *d))
>  {
> -    Monitor *mon = cur_mon;
> -    int i, class;
> -    PCIIORegion *r;
> -    const pci_class_desc *desc;
> +    PCIDevice *d;
> +    int devfn;
>  
> -    monitor_printf(mon, "  Bus %2d, device %3d, function %d:\n",
> -                   pci_bus_num(d->bus),
> -                   PCI_SLOT(d->devfn), PCI_FUNC(d->devfn));
> -    class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> +    for(devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
> +        d = bus->devices[devfn];
> +        if (d) {
> +            fn(bus, d);
> +        }
> +    }
> +}
> +
> +void pci_for_each_device(PCIBus *bus, int bus_num,
> +                         void (*fn)(PCIBus *b, PCIDevice *d))
> +{
> +    bus = pci_find_bus(bus, bus_num);
> +
> +    if (bus) {
> +        pci_for_each_device_under_bus(bus, fn);
> +    }
> +}
> +
> +static void pci_device_print(Monitor *mon, QDict *device)
> +{
> +    QDict *qdict;
> +    QListEntry *entry;
> +    uint64_t addr, size;
> +
> +    monitor_printf(mon, "  Bus %2" PRId64 ", ", qdict_get_int(device, 
> "bus"));
> +    monitor_printf(mon, "device %3" PRId64 ", function %" PRId64 ":\n",
> +                        qdict_get_int(device, "slot"),
> +                        qdict_get_int(device, "function"));
>      monitor_printf(mon, "    ");
> +
> +    qdict = qdict_get_qdict(device, "class_info");
> +    if (qdict_haskey(qdict, "desc")) {
> +        monitor_printf(mon, "%s", qdict_get_str(qdict, "desc"));
> +    } else {
> +        monitor_printf(mon, "Class %04" PRId64, qdict_get_int(qdict, 
> "class"));
> +    }
> +
> +    qdict = qdict_get_qdict(device, "id");
> +    monitor_printf(mon, ": PCI device %04" PRIx64 ":%04" PRIx64 "\n",
> +                        qdict_get_int(qdict, "device"),
> +                        qdict_get_int(qdict, "vendor"));
> +
> +    if (qdict_haskey(device, "irq")) {
> +        monitor_printf(mon, "      IRQ %" PRId64 ".\n",
> +                            qdict_get_int(device, "irq"));
> +    }
> +
> +    /* TODO: PCI bridge info */
> +
> +    QLIST_FOREACH_ENTRY(qdict_get_qlist(device, "regions"), entry) {
> +        qdict = qobject_to_qdict(qlist_entry_obj(entry));
> +        monitor_printf(mon, "      BAR%d: ", (int) qdict_get_int(qdict, 
> "bar"));
> +
> +        addr = qdict_get_int(qdict, "address");
> +        size = qdict_get_int(qdict, "size");
> +
> +        if (!strcmp(qdict_get_str(qdict, "type"), "io")) {
> +            monitor_printf(mon, "I/O at 0x%04"FMT_PCIBUS
> +                                " [0x%04"FMT_PCIBUS"].\n",
> +                                addr, addr + size - 1);
> +        } else {
> +            monitor_printf(mon, "%d bit%s memory at 0x%08"FMT_PCIBUS
> +                               " [0x%08"FMT_PCIBUS"].\n",
> +                                qdict_get_bool(qdict, "mem_type_64") ? 64 : 
> 32,
> +                                qdict_get_bool(qdict, "prefetch") ?
> +                                " prefetchable" : "", addr, addr + size - 1);
> +        }
> +    }
> +
> +    monitor_printf(mon, "      id \"%s\"\n", qdict_get_str(device, 
> "qdev_id"));
> +
> +    /* TODO: PCI bridge devices */
> +}
> +
> +void do_pci_info_print(Monitor *mon, const QObject *data)
> +{
> +    QListEntry *bus, *dev;
> +
> +    QLIST_FOREACH_ENTRY(qobject_to_qlist(data), bus) {
> +        QDict *qdict = qobject_to_qdict(qlist_entry_obj(bus));
> +        QLIST_FOREACH_ENTRY(qdict_get_qlist(qdict, "devices"), dev) {
> +            pci_device_print(mon, qobject_to_qdict(qlist_entry_obj(dev)));
> +        }
> +    }
> +}
> +
> +static QObject *pci_get_dev_class(const PCIDevice *dev)
> +{
> +    int class;
> +    const char *str = "";
> +    const pci_class_desc *desc;
> +
> +    class = pci_get_word(dev->config + PCI_CLASS_DEVICE);
>      desc = pci_class_descriptions;
>      while (desc->desc && class != desc->class)
>          desc++;
> +
>      if (desc->desc) {
[...]
> +        str = desc->desc;
> +    }
> +
> +    return qobject_from_jsonf("{ 'desc': %s, 'class': %d }", str, class);
> +}

This yields 'desc': '' for unknown class.  I find that a bit unnatural.
If we don't have a desc, put none into the dictionary.

> +
> +static QObject *pci_get_dev_id(const PCIDevice *dev)
> +{
> +    return qobject_from_jsonf("{ 'device': %d, 'vendor': %d }",
> +                              pci_get_word(dev->config + PCI_VENDOR_ID),
> +                              pci_get_word(dev->config + PCI_DEVICE_ID));
> +}
> +
> +static QObject *pci_get_regions_list(const PCIDevice *dev)
> +{
> +    int i;
> +    QList *regions_list;
> +
> +    regions_list = qlist_new();
> +
> +    for (i = 0; i < PCI_NUM_REGIONS; i++) {
> +        const PCIIORegion *r = &dev->io_regions[i];
>          if (r->size != 0) {

You're struggling with long lines in this function.  "if (!r->size)
continue" avoids nesting.  Matter of taste, of course.

> -            monitor_printf(mon, "      BAR%d: ", i);
> +            QObject *obj;
>              if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> -                monitor_printf(mon, "I/O at 0x%04"FMT_PCIBUS
> -                               " [0x%04"FMT_PCIBUS"].\n",
> -                               r->addr, r->addr + r->size - 1);
> +                obj = qobject_from_jsonf("{ 'bar': %d, 'type': 'io', "
> +                                         "'address': %" PRId64 ", "
> +                                         "'size': %" PRId64 " }",
> +                                         i, r->addr, r->size);
>              } else {
> -                const char *type = r->type & PCI_BASE_ADDRESS_MEM_TYPE_64 ?
> -                    "64 bit" : "32 bit";
> -                const char *prefetch =
> -                    r->type & PCI_BASE_ADDRESS_MEM_PREFETCH ?
> -                    " prefetchable" : "";
> -
> -                monitor_printf(mon, "%s%s memory at 0x%08"FMT_PCIBUS
> -                               " [0x%08"FMT_PCIBUS"].\n",
> -                               type, prefetch,
> -                               r->addr, r->addr + r->size - 1);
> +                int mem_type_64 = r->type & PCI_BASE_ADDRESS_MEM_TYPE_64;
> +
> +                obj = qobject_from_jsonf("{ 'bar': %d, 'type': 'memory', "
> +                                         "'mem_type_64': %i, 'prefetch': %i, 
> "
> +                                         "'address': %" PRId64 ", "
> +                                         "'size': %" PRId64 " }",
> +                                         i, mem_type_64,
> +                                         r->type 
> &PCI_BASE_ADDRESS_MEM_PREFETCH,
> +                                         r->addr, r->size);
>              }
> +            qlist_append_obj(regions_list, obj);
>          }
>      }
> -    monitor_printf(mon, "      id \"%s\"\n", d->qdev.id ? d->qdev.id : "");
> -    if (class == 0x0604 && d->config[0x19] != 0) {
> -        pci_for_each_device(bus, d->config[0x19], pci_info_device);
> +
> +    return QOBJECT(regions_list);
> +}
> +
> +static QObject *pci_get_dev_dict(const PCIDevice *dev, int bus_num)
> +{
> +    QObject *obj;
> +
> +    obj = qobject_from_jsonf("{ 'bus': %d, 'slot': %d, 'function': %d,"      
>                                  "'class_info': %p, 'id': %p, 'regions': %p,"
> +                              " 'qdev_id': %s }",
> +                              bus_num,
> +                              PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn),
> +                              pci_get_dev_class(dev), pci_get_dev_id(dev),
> +                              pci_get_regions_list(dev),
> +                              dev->qdev.id ? dev->qdev.id : "");

Why repeat the bus number?  See below.

> +
> +    if (dev->config[PCI_INTERRUPT_PIN] != 0) {
> +        QDict *qdict = qobject_to_qdict(obj);
> +        qdict_put(qdict, "irq", 
> qint_from_int(dev->config[PCI_INTERRUPT_LINE]));
>      }
> +
> +    return obj;
>  }
>  
> -static void pci_for_each_device_under_bus(PCIBus *bus,
> -                                          void (*fn)(PCIBus *b, PCIDevice 
> *d))
> +static QObject *pci_get_devices_list(PCIBus *bus, int bus_num)
>  {
> -    PCIDevice *d;
>      int devfn;
> +    PCIDevice *dev;
> +    QList *dev_list;
>  
> -    for(devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
> -        d = bus->devices[devfn];
> -        if (d)
> -            fn(bus, d);
> +    dev_list = qlist_new();
> +
> +    for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
> +        dev = bus->devices[devfn];
> +        if (dev) {
> +            qlist_append_obj(dev_list, pci_get_dev_dict(dev, bus_num));
> +        }
>      }
> +
> +    return QOBJECT(dev_list);
>  }
>  
> -void pci_for_each_device(PCIBus *bus, int bus_num,
> -                         void (*fn)(PCIBus *b, PCIDevice *d))
> +static QObject *pci_get_bus_dict(PCIBus *bus, int bus_num)
>  {
>      bus = pci_find_bus(bus, bus_num);
> -
>      if (bus) {
> -        pci_for_each_device_under_bus(bus, fn);
> +        return qobject_from_jsonf("{ 'bus': %d, 'devices': %p }",
> +                                  bus_num, pci_get_devices_list(bus, 
> bus_num));
>      }
> +
> +    return NULL;
>  }
>  
> -void pci_info(Monitor *mon)
> +/**
> + * do_pci_info(): PCI buses and devices information
> + *
> + * The returned QObject is a QList of all buses. Each bus is
> + * represented by a QDict, which has a key with a QList of all
> + * PCI devices attached to it. Each device is represented by
> + * a QDict.
> + *
> + * The bus QDict contains the following:
> + *
> + * - "bus": bus number
> + * - "devices": a QList of QDicts, each QDict represents a PCI
> + *   device
> + *
> + * The PCI device QDict contains the following:
> + *
> + * - "bus": bus number

This is redundant, because a device QDict is always contained in a bus
QDict, which has the bus number.

> + * - "slot": slot number
> + * - "function": function number
> + * - "class_info": a QDict containing:
> + *      - "desc": device class description
> + *      - "class": device class number

Need to document how unknown classes are encoded (now: "desc" has value
"").

> + * - "id": a QDict containing:
> + *      - "device": device ID
> + *      - "vendor": vendor ID
> + * - "irq": device's IRQ if assigned (optional)
> + * - "qdev_id": qdev id string
> + * - "regions": a QList of QDicts, each QDict represents a
> + *   memory region of this device
> + *
> + * The region QDict can be an I/O region or a memory region,
> + * the I/O region contains the following:

"An I/O region QDict", because there can be more than one.

> + *
> + * - "type": "io"
> + * - "bar": BAR number
> + * - "address": memory address
> + * - "size": memory size
> + *
> + * The memory region QDict contains the following:

Likewise, "A memory region QDict".

> + *
> + * - "type": "memory"
> + * - "bar": BAR number
> + * - "address": memory address
> + * - "size": memory size
> + * - "mem_type_64": true or false
> + * - "prefetch": true or false
> + */
> +void do_pci_info(Monitor *mon, QObject **ret_data)
>  {
> +    QList *bus_list;
>      struct PCIHostBus *host;
> +
> +    bus_list = qlist_new();
> +
>      QLIST_FOREACH(host, &host_buses, next) {
> -        pci_for_each_device(host->bus, 0, pci_info_device);
> +        QObject *obj = pci_get_bus_dict(host->bus, 0);
> +        if (obj) {
> +            qlist_append_obj(bus_list, obj);
> +        }
>      }
> +
> +    *ret_data = QOBJECT(bus_list);
>  }
>  
>  static const char * const pci_nic_models[] = {
> diff --git a/hw/pci.h b/hw/pci.h
> index 9b5ae97..45edf75 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -2,6 +2,7 @@
>  #define QEMU_PCI_H
>  
>  #include "qemu-common.h"
> +#include "qobject.h"
>  
>  #include "qdev.h"
>  
> @@ -233,7 +234,8 @@ PCIBus *pci_get_bus_devfn(int *devfnp, const char 
> *devaddr);
>  int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
>                       unsigned *slotp);
>  
> -void pci_info(Monitor *mon);
> +void do_pci_info_print(Monitor *mon, const QObject *data);
> +void do_pci_info(Monitor *mon, QObject **ret_data);
>  PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
>                          pci_map_irq_fn map_irq, const char *name);
>  PCIDevice *pci_bridge_get_device(PCIBus *bus);
> diff --git a/monitor.c b/monitor.c
> index 2403a97..7536c1e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2438,7 +2438,8 @@ static const mon_cmd_t info_cmds[] = {
>          .args_type  = "",
>          .params     = "",
>          .help       = "show PCI info",
> -        .mhandler.info = pci_info,
> +        .user_print = do_pci_info_print,
> +        .mhandler.info_new = do_pci_info,
>      },
>  #if defined(TARGET_I386) || defined(TARGET_SH4)
>      {