From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NWvDK-0008Ft-2E for qemu-devel@nongnu.org; Mon, 18 Jan 2010 12:16:30 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NWvDF-0008Ev-3J for qemu-devel@nongnu.org; Mon, 18 Jan 2010 12:16:29 -0500 Received: from [199.232.76.173] (port=57690 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NWvDE-0008Es-Vo for qemu-devel@nongnu.org; Mon, 18 Jan 2010 12:16:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41504) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NWvDE-0004S4-Dd for qemu-devel@nongnu.org; Mon, 18 Jan 2010 12:16:24 -0500 From: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH 3/5] PCI: Convert pci_info() to QObject References: <1262976136-31852-1-git-send-email-lcapitulino@redhat.com> <1262976136-31852-4-git-send-email-lcapitulino@redhat.com> Date: Mon, 18 Jan 2010 18:16:21 +0100 In-Reply-To: <1262976136-31852-4-git-send-email-lcapitulino@redhat.com> (Luiz Capitulino's message of "Fri, 8 Jan 2010 16:42:14 -0200") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, mst@redhat.com Luiz Capitulino 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 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) > {