From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MtFtu-00049G-M9 for qemu-devel@nongnu.org; Thu, 01 Oct 2009 03:16:30 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MtFtp-00047l-DN for qemu-devel@nongnu.org; Thu, 01 Oct 2009 03:16:29 -0400 Received: from [199.232.76.173] (port=53863 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MtFtp-00047h-4o for qemu-devel@nongnu.org; Thu, 01 Oct 2009 03:16:25 -0400 Received: from mx20.gnu.org ([199.232.41.8]:27196) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MtFto-00065l-DM for qemu-devel@nongnu.org; Thu, 01 Oct 2009 03:16:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MtFtn-0005ju-HR for qemu-devel@nongnu.org; Thu, 01 Oct 2009 03:16:23 -0400 Date: Thu, 1 Oct 2009 09:14: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: <20091001071426.GA5718@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> <20091001062826.GD5142@redhat.com> <20091001070059.GW24813%yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091001070059.GW24813%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 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 > > > > > --- > > > > > 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