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

[Michael S. Tsirkin]

On Thu, Oct 01, 2009 at 04:00:59PM +0900, Isaku Yamahata wrote:
> 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.

By the way, bus number might not technically be unique: a child bus
not reacheable by number from the root might have the same number
as another one reacheable: set secondary bus number to 1, subordinate 
bus number to 2, and you can have child bus with number 3 behind it
which is not reacheable.

So my idea was that we should have an array indexed by bus number for
reacheable child busses.  This array would be used only for fast address
decoding.  Each time you change some bus numbers, scan the tree and
rebuild the whole array from scratch.


> 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