On Fri, Oct 30, 2009 at 09:21:13PM +0900, Isaku Yamahata wrote: > This patch sorts out/enhances pci code to track pci bus topology > more accurately. > - Track host bus bridge with pci domain number. Although the > current qemu implementation supports only pci domian 0 yet. > - Track pci bridge parent-child relationship. > When looking down from pci host bus for pci sub bus, be aware of > secondary bus/subordinate bus. > Thus pci configuration transaction is more accurately emulated. > > This patch adds new member to PCIBus to track pci bus topology. > Since qdev already tracks down bus relationship, those new member > wouldn't be necessary. > However it would be addressed later because not all the pci device > isn't converted to qdev yet. > > Signed-off-by: Isaku Yamahata <yamah...@valinux.co.jp>
I agree with what you are doing here, overall. Some comments: > --- > hw/pci-hotplug.c | 4 +- > hw/pci.c | 132 > +++++++++++++++++++++++++++++++++++++++++------------- > hw/pci.h | 8 ++- > 3 files changed, 108 insertions(+), 36 deletions(-) > > diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c > index 15a2dfb..48a641b 100644 > --- a/hw/pci-hotplug.c > +++ b/hw/pci-hotplug.c > @@ -113,7 +113,7 @@ void drive_hot_add(Monitor *mon, const QDict *qdict) > if (pci_read_devaddr(mon, pci_addr, &dom, &pci_bus, &slot)) { > goto err; > } > - dev = pci_find_device(pci_bus, slot, 0); > + dev = pci_find_device(pci_find_host_bus(0), pci_bus, slot, 0); > if (!dev) { > monitor_printf(mon, "no pci device with address %s\n", pci_addr); > goto err; > @@ -257,7 +257,7 @@ void pci_device_hot_remove(Monitor *mon, const char > *pci_addr) > return; > } > > - d = pci_find_device(bus, slot, 0); > + d = pci_find_device(pci_find_host_bus(0), bus, slot, 0); > if (!d) { > monitor_printf(mon, "slot %d empty\n", slot); > return; > diff --git a/hw/pci.c b/hw/pci.c > index a75d981..3e5780a 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -44,7 +44,10 @@ struct PCIBus { > void *irq_opaque; > PCIDevice *devices[256]; > PCIDevice *parent_dev; > - PCIBus *next; > + > + QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */ > + QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */ > + > /* The bus IRQ state is the logical OR of the connected devices. > Keep a count of the number of devices with raised IRQs. */ > int nirq; > @@ -69,7 +72,13 @@ static void pci_set_irq(void *opaque, int irq_num, int > level); > target_phys_addr_t pci_mem_base; > static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET; > static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU; > -static PCIBus *first_bus; > + > +struct PCIHostBus { > + int domain; > + struct PCIBus *bus; > + QLIST_ENTRY(PCIHostBus) next; > +}; > +static QLIST_HEAD(, PCIHostBus) host_buses; > > static const VMStateDescription vmstate_pcibus = { > .name = "PCIBUS", > @@ -127,6 +136,28 @@ static void pci_bus_reset(void *opaque) > } > } > > +static void pci_host_bus_register(int domain, PCIBus *bus) > +{ > + struct PCIHostBus *host; > + host = qemu_mallocz(sizeof(*host)); > + host->domain = domain; > + host->bus = bus; > + QLIST_INSERT_HEAD(&host_buses, host, next); > +} > + > +PCIBus *pci_find_host_bus(int domain) > +{ > + struct PCIHostBus *host; > + > + QLIST_FOREACH(host, &host_buses, next) { > + if (host->domain == domain) { > + return host->bus; > + } > + } > + > + return NULL; > +} > + > void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent, > const char *name, int devfn_min) > { > @@ -134,8 +165,11 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState > *parent, > > qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name); > bus->devfn_min = devfn_min; > - bus->next = first_bus; > - first_bus = bus; > + > + /* host bridge */ > + QLIST_INIT(&bus->child); > + pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported > */ > + > vmstate_register(nbus++, &vmstate_pcibus, bus); > qemu_register_reset(pci_bus_reset, bus); > } > @@ -177,7 +211,8 @@ PCIBus *pci_register_bus(DeviceState *parent, const char > *name, > return bus; > } > > -static void pci_register_secondary_bus(PCIBus *bus, > +static void pci_register_secondary_bus(PCIBus *parent, > + PCIBus *bus, > PCIDevice *dev, > pci_map_irq_fn map_irq, > const char *name) > @@ -185,8 +220,15 @@ static void pci_register_secondary_bus(PCIBus *bus, > qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name); > bus->map_irq = map_irq; > bus->parent_dev = dev; > - bus->next = dev->bus->next; > - dev->bus->next = bus; > + > + QLIST_INIT(&bus->child); > + QLIST_INSERT_HEAD(&parent->child, bus, sibling); > +} > + > +static void pci_unregister_secondary_bus(PCIBus *bus) > +{ > + assert(QLIST_EMPTY(&bus->child)); > + QLIST_REMOVE(bus, sibling); > } > > int pci_bus_num(PCIBus *s) > @@ -196,6 +238,13 @@ int pci_bus_num(PCIBus *s) > return s->parent_dev->config[PCI_SECONDARY_BUS]; > } > > +static uint8_t pci_sub_bus(PCIBus *s) This seems to be only used in one place, and it's not obvious what this does. Please open-code. > +{ > + if (!s->parent_dev) > + return 255; /* pci host bridge */ Please use a symbolic constant. Is this one UINT8_MAX or ARRAY_SIZE() or some array? > + return s->parent_dev->config[PCI_SUBORDINATE_BUS]; > +} > + > static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) > { > PCIDevice *s = container_of(pv, PCIDevice, config); > @@ -301,7 +350,7 @@ static int pci_parse_devaddr(const char *addr, int *domp, > int *busp, unsigned *s > return -1; > > /* Note: QEMU doesn't implement domains other than 0 */ > - if (dom != 0 || pci_find_bus(bus) == NULL) > + if (!pci_find_bus(pci_find_host_bus(dom), bus)) > return -1; > > *domp = dom; > @@ -331,7 +380,7 @@ PCIBus *pci_get_bus_devfn(int *devfnp, const char > *devaddr) > > if (!devaddr) { > *devfnp = -1; > - return pci_find_bus(0); > + return pci_find_bus(pci_find_host_bus(0), 0); > } > > if (pci_parse_devaddr(devaddr, &dom, &bus, &slot) < 0) { > @@ -339,7 +388,7 @@ PCIBus *pci_get_bus_devfn(int *devfnp, const char > *devaddr) > } > > *devfnp = slot << 3; > - return pci_find_bus(bus); > + return pci_find_bus(pci_find_host_bus(0), bus); > } > > static void pci_init_cmask(PCIDevice *dev) > @@ -625,8 +674,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t > val, int len) > addr, val, len); > #endif > bus_num = (addr >> 16) & 0xff; > - while (s && pci_bus_num(s) != bus_num) > - s = s->next; > + s = pci_find_bus(s, bus_num); > if (!s) > return; > pci_dev = s->devices[(addr >> 8) & 0xff]; > @@ -646,8 +694,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int > len) > uint32_t val; > > bus_num = (addr >> 16) & 0xff; > - while (s && pci_bus_num(s) != bus_num) > - s= s->next; > + s = pci_find_bus(s, bus_num); > if (!s) > goto fail; > pci_dev = s->devices[(addr >> 8) & 0xff]; > @@ -753,7 +800,7 @@ static const pci_class_desc pci_class_descriptions[] = > { 0, NULL} > }; > > -static void pci_info_device(PCIDevice *d) > +static void pci_info_device(PCIBus *bus, PCIDevice *d) > { > Monitor *mon = cur_mon; > int i, class; > @@ -808,30 +855,32 @@ static void pci_info_device(PCIDevice *d) > } > monitor_printf(mon, " id \"%s\"\n", d->qdev.id ? d->qdev.id : ""); > if (class == 0x0604 && d->config[0x19] != 0) { > - pci_for_each_device(d->config[0x19], pci_info_device); > + pci_for_each_device(bus, d->config[0x19], pci_info_device); > } > } > > -void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d)) > +void pci_for_each_device(PCIBus *bus, int bus_num, > + void (*fn)(PCIBus *b, PCIDevice *d)) > { > - PCIBus *bus = first_bus; > PCIDevice *d; > int devfn; > > - while (bus && pci_bus_num(bus) != bus_num) > - bus = bus->next; > + bus = pci_find_bus(bus, bus_num); > if (bus) { > for(devfn = 0; devfn < 256; devfn++) { > d = bus->devices[devfn]; > if (d) > - fn(d); > + fn(bus, d); > } > } > } > > void pci_info(Monitor *mon) > { > - pci_for_each_device(0, pci_info_device); > + struct PCIHostBus *host; > + QLIST_FOREACH(host, &host_buses, next) { > + pci_for_each_device(host->bus, 0, pci_info_device); > + } > } > > static const char * const pci_nic_models[] = { > @@ -918,19 +967,30 @@ static void pci_bridge_write_config(PCIDevice *d, > pci_default_write_config(d, address, val, len); > } > > -PCIBus *pci_find_bus(int bus_num) > +PCIBus *pci_find_bus(PCIBus *bus, int bus_num) > { > - PCIBus *bus = first_bus; > + PCIBus *sec; > > - while (bus && pci_bus_num(bus) != bus_num) > - bus = bus->next; > + if (!bus) > + return NULL; > > - return bus; > + if (pci_bus_num(bus) == bus_num) { > + return bus; > + } > + > + /* try child bus */ > + QLIST_FOREACH(sec, &bus->child, sibling) { > + if (pci_bus_num(sec) <= bus_num && bus_num <= pci_sub_bus(sec)) { I find this confusing. Can we just look at each bridge and decide whether to propage down from it, instead of lookup up at the parent bridge? > + return pci_find_bus(sec, bus_num); > + } > + } > + > + return NULL; > } > > -PCIDevice *pci_find_device(int bus_num, int slot, int function) > +PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function) > { > - PCIBus *bus = pci_find_bus(bus_num); > + bus = pci_find_bus(bus, bus_num); > > if (!bus) > return NULL; > @@ -973,6 +1033,14 @@ static int pci_bridge_initfn(PCIDevice *dev) > return 0; > } > > +static int pci_bridge_exitfn(PCIDevice *pci_dev) > +{ > + PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev); > + PCIBus *bus = &s->bus; > + pci_unregister_secondary_bus(bus); > + return 0; > +} > + > PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, > pci_map_irq_fn map_irq, const char *name) > { > @@ -985,7 +1053,7 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t > vid, uint16_t did, > qdev_init_nofail(&dev->qdev); > > s = DO_UPCAST(PCIBridge, dev, dev); > - pci_register_secondary_bus(&s->bus, &s->dev, map_irq, name); > + pci_register_secondary_bus(bus, &s->bus, &s->dev, map_irq, name); > return &s->bus; > } > > @@ -1148,7 +1216,8 @@ static void pcibus_dev_print(Monitor *mon, DeviceState > *dev, int indent) > monitor_printf(mon, "%*sclass %s, addr %02x:%02x.%x, " > "pci id %04x:%04x (sub %04x:%04x)\n", > indent, "", ctxt, > - pci_bus_num(d->bus), PCI_SLOT(d->devfn), > PCI_FUNC(d->devfn), > + d->config[PCI_SECONDARY_BUS], > + PCI_SLOT(d->devfn), PCI_FUNC(d->devfn), > pci_get_word(d->config + PCI_VENDOR_ID), > pci_get_word(d->config + PCI_DEVICE_ID), > pci_get_word(d->config + PCI_SUBSYSTEM_VENDOR_ID), > @@ -1169,6 +1238,7 @@ static PCIDeviceInfo bridge_info = { > .qdev.name = "pci-bridge", > .qdev.size = sizeof(PCIBridge), > .init = pci_bridge_initfn, > + .exit = pci_bridge_exitfn, > .config_write = pci_bridge_write_config, > .qdev.props = (Property[]) { > DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0), > diff --git a/hw/pci.h b/hw/pci.h > index e83faf5..b16f8f8 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -118,6 +118,7 @@ typedef struct PCIIORegion { > #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */ > #define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ > #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ > +#define PCI_SUBORDINATE_BUS 0x1a /* Highest bus number behind the bridge > */ > #define PCI_SEC_STATUS 0x1e /* Secondary status register, > only bit 14 used */ > #define PCI_SUBSYSTEM_VENDOR_ID 0x2c /* 16 bits */ > #define PCI_SUBSYSTEM_ID 0x2e /* 16 bits */ > @@ -267,9 +268,10 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char > *default_model, > void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len); > uint32_t pci_data_read(void *opaque, uint32_t addr, int len); > int pci_bus_num(PCIBus *s); > -void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d)); > -PCIBus *pci_find_bus(int bus_num); > -PCIDevice *pci_find_device(int bus_num, int slot, int function); > +void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, > PCIDevice *d)); > +PCIBus *pci_find_host_bus(int domain); pci_find_root_bus would be more descriptive IMO. > +PCIBus *pci_find_bus(PCIBus *bus, int bus_num); > +PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function); > PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr); > > int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp, > -- > 1.6.0.2 > >