From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MtFBN-0008Lg-LV for qemu-devel@nongnu.org; Thu, 01 Oct 2009 02:30:29 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MtFBI-0008Ia-EV for qemu-devel@nongnu.org; Thu, 01 Oct 2009 02:30:28 -0400 Received: from [199.232.76.173] (port=39502 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MtFBI-0008IP-8X for qemu-devel@nongnu.org; Thu, 01 Oct 2009 02:30:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59427) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MtFBH-0007tG-MY for qemu-devel@nongnu.org; Thu, 01 Oct 2009 02:30:24 -0400 Date: Thu, 1 Oct 2009 08:28:26 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from(). Message-ID: <20091001062826.GD5142@redhat.com> References: <1247656509-32227-1-git-send-email-yamahata@valinux.co.jp> <1247656509-32227-7-git-send-email-yamahata@valinux.co.jp> <20090930114506.GI18802@redhat.com> <20091001032934.GM24813%yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091001032934.GM24813%yamahata@valinux.co.jp> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org 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 > > > --- > > > 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? > > > > +{ > > > + 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