[V6,19/32] pci: make pci configuration transaction more accurate.

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 <yamahata@valinux.co.jp>
---
 hw/pci-hotplug.c |    4 +-
 hw/pci.c         |  132 +++++++++++++++++++++++++++++++++++++++++-------------
 hw/pci.h         |    8 ++-
 3 files changed, 108 insertions(+), 36 deletions(-)

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 <yamahata@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
> 
>

On Tue, Nov 10, 2009 at 05:49:55PM +0200, Michael S. Tsirkin wrote:
> 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 <yamahata@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.

OK.

> > +{
> > +    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?

Do you mean something like the blow?
	for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
		if (bus->devices[i] is PCIBridge)
			recursive lookup;
	}

Unfortunately there is no reliable way to determine
whether a given PCIDevice is PCIBridge.
So I had to resort to introduce PCIBus::child member.
One possible way is to check pci config header type,
but if the header type were wrongly set due to a bug,
it would be very nasty which is very difficult to track down.
So I avoided it.

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