Patchwork [3/5] PCI: Convert pci_info() to QObject

login
register
mail settings
Submitter Luiz Capitulino
Date Jan. 8, 2010, 6:42 p.m.
Message ID <1262976136-31852-4-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/42530/
State New
Headers show

Comments

Luiz Capitulino - Jan. 8, 2010, 6:42 p.m.
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 <lcapitulino@redhat.com>
---
 hw/pci.c  |  308 +++++++++++++++++++++++++++++++++++++++++++++----------------
 hw/pci.h  |    4 +-
 monitor.c |    3 +-
 3 files changed, 234 insertions(+), 81 deletions(-)
Markus Armbruster - Jan. 18, 2010, 5:16 p.m.
Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>

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)
>      {
Luiz Capitulino - Jan. 19, 2010, 8:27 p.m.
On Mon, 18 Jan 2010 18:16:21 +0100
Markus Armbruster <armbru@redhat.com> wrote:

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

 Right, good catch.

> If we don't have a desc, put none into the dictionary.

 The right thing would be to have null there, but the standard
so far is just to drop the key (which is what I'm going to do here).

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

 I agree.

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

 Turns out I find this redundancy convenient for clients, because if
you pass the device dict down to a function you have the bus number
there if you need it (otherwise you'd need to pass the bus number
too).

 I'm writing a shell in Python and found this convenient, is it that
bad?

> > + * - "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
> "").

 I'll make this optional.

> > + * - "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.

 Ok.

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

 Ok.
Markus Armbruster - Jan. 20, 2010, 7:32 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 18 Jan 2010 18:16:21 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> > +/**
>> > + * 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.
>
>  Turns out I find this redundancy convenient for clients, because if
> you pass the device dict down to a function you have the bus number
> there if you need it (otherwise you'd need to pass the bus number
> too).
>
>  I'm writing a shell in Python and found this convenient, is it that
> bad?

It's not bad, it's redundant :)

A little convenient redundancy is okay.  I just don't want accidental
redundancy.

If you keep it, I'd suggest document that it's identical to the parent's
bus member.

[...]

Patch

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) {
-        monitor_printf(mon, "%s", desc->desc);
-    } else {
-        monitor_printf(mon, "Class %04x", class);
-    }
-    monitor_printf(mon, ": PCI device %04x:%04x\n",
-           pci_get_word(d->config + PCI_VENDOR_ID),
-           pci_get_word(d->config + PCI_DEVICE_ID));
-
-    if (d->config[PCI_INTERRUPT_PIN] != 0) {
-        monitor_printf(mon, "      IRQ %d.\n",
-                       d->config[PCI_INTERRUPT_LINE]);
-    }
-    if (class == 0x0604) {
-        uint64_t base;
-        uint64_t limit;
-
-        monitor_printf(mon, "      BUS %d.\n", d->config[0x19]);
-        monitor_printf(mon, "      secondary bus %d.\n",
-                       d->config[PCI_SECONDARY_BUS]);
-        monitor_printf(mon, "      subordinate bus %d.\n",
-                       d->config[PCI_SUBORDINATE_BUS]);
-
-        base = pci_bridge_get_base(d, PCI_BASE_ADDRESS_SPACE_IO);
-        limit = pci_bridge_get_limit(d, PCI_BASE_ADDRESS_SPACE_IO);
-        monitor_printf(mon, "      IO range [0x%04"PRIx64", 0x%04"PRIx64"]\n",
-                       base, limit);
-
-        base = pci_bridge_get_base(d, PCI_BASE_ADDRESS_SPACE_MEMORY);
-        limit= pci_bridge_get_limit(d, PCI_BASE_ADDRESS_SPACE_MEMORY);
-        monitor_printf(mon,
-                       "      memory range [0x%08"PRIx64", 0x%08"PRIx64"]\n",
-                       base, limit);
-
-        base = pci_bridge_get_base(d, PCI_BASE_ADDRESS_SPACE_MEMORY |
-                                   PCI_BASE_ADDRESS_MEM_PREFETCH);
-        limit = pci_bridge_get_limit(d, PCI_BASE_ADDRESS_SPACE_MEMORY |
-                                     PCI_BASE_ADDRESS_MEM_PREFETCH);
-        monitor_printf(mon, "      prefetchable memory range "
-                       "[0x%08"PRIx64", 0x%08"PRIx64"]\n", base, limit);
-    }
-    for(i = 0;i < PCI_NUM_REGIONS; i++) {
-        r = &d->io_regions[i];
+        str = desc->desc;
+    }
+
+    return qobject_from_jsonf("{ 'desc': %s, 'class': %d }", str, class);
+}
+
+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) {
-            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 : "");
+
+    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
+ * - "slot": slot number
+ * - "function": function number
+ * - "class_info": a QDict containing:
+ *      - "desc": device class description
+ *      - "class": device class number
+ * - "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:
+ *
+ * - "type": "io"
+ * - "bar": BAR number
+ * - "address": memory address
+ * - "size": memory size
+ *
+ * The memory region QDict contains the following:
+ *
+ * - "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)
     {