Re: [Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from().

[Isaku Yamahata]

On Thu, Oct 01, 2009 at 08:28:26AM +0200, Michael S. Tsirkin wrote:
> On Thu, Oct 01, 2009 at 12:29:34PM +0900, Isaku Yamahata wrote:
> > On Wed, Sep 30, 2009 at 01:45:06PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Jul 15, 2009 at 08:15:06PM +0900, Isaku Yamahata wrote:
> > > > factor out while(bus) bus->next loop logic into pci_find_bus_from()
> > > > which will be used later.
> > > > 
> > > > Signed-off-by: Isaku Yamahata <address@hidden>
> > > > ---
> > > >  hw/pci.c |   26 ++++++++++++++------------
> > > >  1 files changed, 14 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > index 9639a32..f06e1da 100644
> > > > --- a/hw/pci.c
> > > > +++ b/hw/pci.c
> > > > @@ -574,6 +574,16 @@ void pci_default_write_config(PCIDevice *d, 
> > > > uint32_t addr, uint32_t val, int l)
> > > >          pci_update_mappings(d);
> > > >  }
> > > >  
> > > > +static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> > > 
> > > Why what is "from"? pci_find_parent_bus a better name?
> > 
> > Its intention is to go down from a given parent bus
> > to find child bus of a given bus number, bus_num.
> > The current implementation arranges PCIBus in a single linked list,
> > and a single linked list doesn't represent pci bus topology well.
> > So it may find a pci bus which isn't child of a given bus.
> 
> Sounds very messy. Let's try to define sane semantics, otherwise maybe
> we don't want to factor this out in a function after all.
> 
> > So far it hasn't been a issue because only a single bus PCI.0 is supported.
> > 
> > Given that several people including me want multiple PCI bus,
> > your question arises that the linked list should be changed/enhanced to
> > some kind of tree structure to represent PCI bus topology accurately.
> > Does it sound a good idea?
> 
> I won't be surprised if we need a tree to represent PCI bus topology.
> But we already have it, don't we: each bus lists devices behind it, some
> of these could be buses.
> 
> OTOH a list of all buses is currently used to look up device by bus
> number.  Since there are only 256 possible buses, how about an array indexing
> buses by bus number? Will be simpler and cleaner than what we have,
> and the api would be pci_find_bus(int bus_num) with no confusing from?

Static array sound good. however I'm not sure bus number indexing
is appropriate because bus number can be changed.
Anyway I'll switch it from list to array.

thanks,

> > > > +{
> > > > +    PCIBus *s = from;
> > > > +
> > > > +    while (s && s->bus_num != bus_num)
> > > 
> > > No space after while.
> > > 
> > > > +        s = s->next;
> > > > +
> > > > +    return s;
> > > > +}
> > > > +
> > > >  void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > > >  {
> > > >      PCIBus *s = opaque;
> > > > @@ -585,8 +595,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 && s->bus_num != bus_num)
> > > > -        s = s->next;
> > > > +    s = pci_find_bus_from(s, bus_num);
> > > >      if (!s)
> > > >          return;
> > > >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > > > @@ -607,8 +616,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, 
> > > > int len)
> > > >      uint32_t val;
> > > >  
> > > >      bus_num = (addr >> 16) & 0xff;
> > > > -    while (s && s->bus_num != bus_num)
> > > > -        s= s->next;
> > > > +    s = pci_find_bus_from(s, bus_num);
> > > >      if (!s)
> > > >          goto fail;
> > > >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > > > @@ -792,8 +800,7 @@ void pci_for_each_device(int bus_num, void 
> > > > (*fn)(PCIDevice *d))
> > > >      PCIDevice *d;
> > > >      int devfn;
> > > >  
> > > > -    while (bus && bus->bus_num != bus_num)
> > > > -        bus = bus->next;
> > > > +    bus = pci_find_bus_from(bus, bus_num);
> > > >      if (bus) {
> > > >          for(devfn = 0; devfn < 256; devfn++) {
> > > >              d = bus->devices[devfn];
> > > > @@ -891,12 +898,7 @@ static void pci_bridge_write_config(PCIDevice *d,
> > > >  
> > > >  PCIBus *pci_find_bus(int bus_num)
> > > >  {
> > > > -    PCIBus *bus = first_bus;
> > > > -
> > > > -    while (bus && bus->bus_num != bus_num)
> > > > -        bus = bus->next;
> > > > -
> > > > -    return bus;
> > > > +    return pci_find_bus_from(first_bus, bus_num);
> > > >  }
> > > >  
> > > >  PCIDevice *pci_find_device(int bus_num, int slot, int function)
> > > 
> > 
> 

-- 
yamahata