From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46389 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OW7C3-00049f-Cn for qemu-devel@nongnu.org; Tue, 06 Jul 2010 08:24:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OW7C1-0005tS-3o for qemu-devel@nongnu.org; Tue, 06 Jul 2010 08:24:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12633) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OW7C0-0005t9-Re for qemu-devel@nongnu.org; Tue, 06 Jul 2010 08:24:05 -0400 Date: Tue, 6 Jul 2010 15:18:52 +0300 From: "Michael S. Tsirkin" Message-ID: <20100706121852.GA20208@redhat.com> References: <8ab6e42be20518d6491e0cd0d5985f7df6912c01.1278037560.git.yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8ab6e42be20518d6491e0cd0d5985f7df6912c01.1278037560.git.yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: blauwirbel@gmail.com, qemu-devel@nongnu.org On Fri, Jul 02, 2010 at 11:30:11AM +0900, Isaku Yamahata wrote: > allocate PCIBus dynamically for PCIBridge and bug fix of > pci_unregister_secondary_bus(). could you make the bugfix a separate patch please? > This is a preparation for splitting out pci_bridge functions. > Since PCIBus is private to pci.c, PCIBridge won't be able to > contain PCIBus in its structure. > > Signed-off-by: Isaku Yamahata I think this becomes too complex: as bridge configuration affects the bus operation, you might end up sticking a pointer to the device in the bus. A similar arrangement is in place in with piix_pci, and I would love to get rid of it, too. Let's just put PCIBus in a header? It could be a new header named pci_internals.h or something like this. > --- > hw/pci.c | 25 ++++++++++++++----------- > 1 files changed, 14 insertions(+), 11 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 08652e8..fdf02d0 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -286,23 +286,27 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, > return bus; > } > > -static void pci_register_secondary_bus(PCIBus *parent, > - PCIBus *bus, > - PCIDevice *dev, > - pci_map_irq_fn map_irq, > - const char *name) > +static PCIBus *pci_register_secondary_bus(PCIBus *parent, > + PCIDevice *dev, > + pci_map_irq_fn map_irq, > + const char *name) > { > - qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name); > + PCIBus *bus; > + bus = pci_bus_new(&dev->qdev, name, 0); > + > bus->map_irq = map_irq; > bus->parent_dev = dev; > > QLIST_INSERT_HEAD(&parent->child, bus, sibling); > + > + return bus; This does more than we need: pci_bus_new was created for host bus so it will also register in reset and vmstate lists. > } > > static void pci_unregister_secondary_bus(PCIBus *bus) > { > assert(QLIST_EMPTY(&bus->child)); > QLIST_REMOVE(bus, sibling); > + qbus_free(&bus->qbus); > } > > int pci_bus_num(PCIBus *s) > @@ -1527,7 +1531,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model, > > typedef struct { > PCIDevice dev; > - PCIBus bus; > + PCIBus *bus; > uint32_t vid; > uint32_t did; > } PCIBridge; > @@ -1628,8 +1632,7 @@ static int pci_bridge_initfn(PCIDevice *dev) > static int pci_bridge_exitfn(PCIDevice *pci_dev) > { > PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev); > - PCIBus *bus = &s->bus; > - pci_unregister_secondary_bus(bus); > + pci_unregister_secondary_bus(s->bus); > return 0; > } > > @@ -1646,8 +1649,8 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction, > qdev_init_nofail(&dev->qdev); > > s = DO_UPCAST(PCIBridge, dev, dev); > - pci_register_secondary_bus(bus, &s->bus, &s->dev, map_irq, name); > - return &s->bus; > + s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name); > + return s->bus; > } > > PCIDevice *pci_bridge_get_device(PCIBus *bus) > -- > 1.7.1.1