From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=45020 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OQfhp-0001CL-C1 for qemu-devel@nongnu.org; Mon, 21 Jun 2010 08:02:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OQfhn-00008f-Jv for qemu-devel@nongnu.org; Mon, 21 Jun 2010 08:02:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51068) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OQfhn-00008a-8L for qemu-devel@nongnu.org; Mon, 21 Jun 2010 08:02:23 -0400 Date: Mon, 21 Jun 2010 14:57:09 +0300 From: "Michael S. Tsirkin" Message-ID: <20100621115709.GA10511@redhat.com> References: <748531d265b3d915ddac6316fdcd5bb7f9475ab4.1277100005.git.yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <748531d265b3d915ddac6316fdcd5bb7f9475ab4.1277100005.git.yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH v4 1/6] pci: use PCI_DEVFN() where appropriate. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: blauwirbel@gmail.com, yu.liu@freescale.com, qemu-devel@nongnu.org, aurelien@aurel32.net, paul@codesourcery.com On Mon, Jun 21, 2010 at 03:03:56PM +0900, Isaku Yamahata wrote: > Use PCI_DEVFN() and PCI_FUNC_MAX where appropriate. > This patch make it clear that func = 0 and > added assert() to ensure it. > This patch guarantees that auto assigned function is always > single function device at function = 0. > > Cc: Aurelien Jarno > Cc: Yu Liu > Cc: Paul Brook > Signed-off-by: Isaku Yamahata How was this tested? I suggest splitting the asserts to a separate patch, then you can compare stripped object files before and after the change, should be identical. > --- > hw/gt64xxx.c | 2 +- > hw/pci.c | 4 +++- > hw/pci.h | 1 + > hw/ppce500_pci.c | 3 ++- > hw/unin_pci.c | 12 ++++++------ > hw/versatile_pci.c | 2 +- > 6 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c > index 7691e1d..8e2cf14 100644 > --- a/hw/gt64xxx.c > +++ b/hw/gt64xxx.c > @@ -1115,7 +1115,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic) > > s->pci->bus = pci_register_bus(NULL, "pci", > pci_gt64120_set_irq, pci_gt64120_map_irq, > - pic, 144, 4); > + pic, PCI_DEVFN(18, 0), 4); > s->ISD_handle = cpu_register_io_memory(gt64120_read, gt64120_write, s); > d = pci_register_device(s->pci->bus, "GT64120 PCI Bus", sizeof(PCIDevice), > 0, NULL, NULL); > diff --git a/hw/pci.c b/hw/pci.c > index ef17eb4..28ba9cf 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -224,6 +224,7 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent, > const char *name, int devfn_min) > { > qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name); > + assert(PCI_FUNC(devfn_min) == 0); > bus->devfn_min = devfn_min; > QLIST_INIT(&bus->child); > vmstate_register(-1, &vmstate_pcibus, bus); > @@ -600,8 +601,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > uint8_t header_type) > { > if (devfn < 0) { > + assert(PCI_FUNC(bus->devfn_min) == 0); I think that the assert when bus is created is enough. > for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices); > - devfn += 8) { > + devfn += PCI_FUNC_MAX) { > if (!bus->devices[devfn]) > goto found; > } > diff --git a/hw/pci.h b/hw/pci.h > index 6a2bc6a..077387d 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -14,6 +14,7 @@ > #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07)) > #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f) > #define PCI_FUNC(devfn) ((devfn) & 0x07) > +#define PCI_FUNC_MAX 8 > > /* Class, Vendor and Device IDs from Linux's pci_ids.h */ > #include "pci_ids.h" > diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c > index 336d284..f949fe3 100644 > --- a/hw/ppce500_pci.c > +++ b/hw/ppce500_pci.c > @@ -279,7 +279,8 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers) > controller->pci_state.bus = pci_register_bus(NULL, "pci", > mpc85xx_pci_set_irq, > mpc85xx_pci_map_irq, > - pci_irqs, 0x88, 4); > + pci_irqs, PCI_DEVFN(0x11, 0), > + 4); > d = pci_register_device(controller->pci_state.bus, > "host bridge", sizeof(PCIDevice), > 0, NULL, NULL); > diff --git a/hw/unin_pci.c b/hw/unin_pci.c > index f0a773d..0ecf40f 100644 > --- a/hw/unin_pci.c > +++ b/hw/unin_pci.c > @@ -228,10 +228,10 @@ PCIBus *pci_pmac_init(qemu_irq *pic) > d = FROM_SYSBUS(UNINState, s); > d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci", > pci_unin_set_irq, pci_unin_map_irq, > - pic, 11 << 3, 4); > + pic, PCI_DEVFN(11, 0), 4); > > #if 0 > - pci_create_simple(d->host_state.bus, 11 << 3, "uni-north"); > + pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north"); > #endif > > sysbus_mmio_map(s, 0, 0xf2800000); > @@ -240,11 +240,11 @@ PCIBus *pci_pmac_init(qemu_irq *pic) > /* DEC 21154 bridge */ > #if 0 > /* XXX: not activated as PPC BIOS doesn't handle multiple buses properly */ > - pci_create_simple(d->host_state.bus, 12 << 3, "dec-21154"); > + pci_create_simple(d->host_state.bus, PCI_DEVFN(12, 0), "dec-21154"); > #endif > > /* Uninorth AGP bus */ > - pci_create_simple(d->host_state.bus, 11 << 3, "uni-north-agp"); > + pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north-agp"); > dev = qdev_create(NULL, "uni-north-agp"); > qdev_init_nofail(dev); > s = sysbus_from_qdev(dev); > @@ -254,7 +254,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic) > /* Uninorth internal bus */ > #if 0 > /* XXX: not needed for now */ > - pci_create_simple(d->host_state.bus, 14 << 3, "uni-north-pci"); > + pci_create_simple(d->host_state.bus, PCI_DEVFN(14, 0), "uni-north-pci"); > dev = qdev_create(NULL, "uni-north-pci"); > qdev_init_nofail(dev); > s = sysbus_from_qdev(dev); > @@ -280,7 +280,7 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic) > > d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci", > pci_unin_set_irq, pci_unin_map_irq, > - pic, 11 << 3, 4); > + pic, PCI_DEVFN(11, 0), 4); > > sysbus_mmio_map(s, 0, 0xf0800000); > sysbus_mmio_map(s, 1, 0xf0c00000); > diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c > index 199bc19..a76bdfa 100644 > --- a/hw/versatile_pci.c > +++ b/hw/versatile_pci.c > @@ -127,7 +127,7 @@ static int pci_vpb_init(SysBusDevice *dev) > } > bus = pci_register_bus(&dev->qdev, "pci", > pci_vpb_set_irq, pci_vpb_map_irq, s->irq, > - 11 << 3, 4); > + PCI_DEVFN(11, 0), 4); > > /* ??? Register memory space. */ > > -- > 1.6.6.1