[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Michael S. Tsirkin]

On Wed, Nov 24, 2010 at 04:15:14PM +0900, Isaku Yamahata wrote:
> On Wed, Nov 24, 2010 at 07:27:58AM +0200, Michael S. Tsirkin wrote:
> > Right. So let's add an inline helper to avoid code duplication here?
> > 
> > pci: fix bus walk under secondary bus reset
> > 
> > Take into account secondary bus reset bit for
> > bus walk: devices behind a reset bus should not
> > respond to configuration cycles.
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > 
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index d02f980..0c15b13 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1540,6 +1540,16 @@ void pci_bridge_update_mappings(PCIBus *b)
> >      }
> >  }
> >  
> > +/* Whether a given bus number is in range of the secondary
> > + * bus of the given bridge device. */
> > +static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
> > +{
> > +    return !(pci_get_word(dev->config + PCI_BRIDGE_CONTROL) &
> > +             PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's 
> > reset. */ &&
> > +        dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > +        bus_num <= dev->config[PCI_SUBORDINATE_BUS];
> > +}
> > +
> >  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> >  {
> >      PCIBus *sec;
> > @@ -1552,20 +1562,21 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> >          return bus;
> >      }
> >  
> > +    /* Consider all bus numbers in range for the host pci bridge. */
> > +    if (bus->parent_dev &&
> > +        !pci_secondary_bus_in_range(bus->parent_dev, bus_num)) {
> > +        return NULL;
> > +    }
> > +
> >      /* try child bus */
> > -    if (!bus->parent_dev /* host pci bridge */ ||
> > -        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > -         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
> > -        for (; bus; bus = sec) {
> > -            QLIST_FOREACH(sec, &bus->child, sibling) {
> > -                assert(sec->parent_dev);
> > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) 
> > {
> > -                    return sec;
> > -                }
> > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > -                    bus_num <= 
> > sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
> > -                    break;
> > -                }
> > +    for (; bus; bus = sec) {
> > +        QLIST_FOREACH(sec, &bus->child, sibling) {
> > +            assert(sec->parent_dev);
> > +            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> > +                return sec;
> > +            }
> > +            if (pci_secondary_bus_in_range(sec->parent_dev, bus_num)) {
> 
> This condition should be "if (!pci_...)"

Why? We are looking for a device on the given bus that claims the given
bus_num.  If we find one, break out of the inner loop and walk down to
the child.

> 
> > +                break;
> >              }
> >          }
> >      }
> > 
> > > -- 
> > > yamahata
> > 
> 
> -- 
> yamahata