qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library
@ 2010-07-12 10:36 Isaku Yamahata
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 1/5] pci: move out pci internal structures, PCIBus, PCIBridge, and pci_bus_info Isaku Yamahata
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Isaku Yamahata @ 2010-07-12 10:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: blauwirbel, yamahata, mst

changes v1 -> v2:
- introduce pci_internals.h to accomodate pci internal strcutures to
  share between pci.c and pci_bridge.c
- don't make PCIBridge::bus pointer as suggested by
  Michael S. Tsirkin <mst@redhat.com>
- rename PCIBridge::bus -> PCIBridge::sec_bus
- eliminate pci_reguster_secondary_bus()/pci_unregister_secondary_bus()
- document pci bridge library functions.
- introduced pci bridge library.

Clean up of pci host bus ans piix pci as discussed with v1
will be addressed after this patch set is accepted.

Patch description:
Now pci.c has grown. So split bridge related code into dedicated file
for further extension to pci bridge. Further clean up and pcie port emulator. 
This make patch conflict less possible in future.

Isaku Yamahata (5):
  pci: move out pci internal structures, PCIBus, PCIBridge, and
    pci_bus_info.
  pci/bridge: split out pci bridge code into pci_bridge.c from pci.c
  pci_bridge: rename PCIBridge::bus -> PCIBridge::sec_bus.
  pci_bridge: clean up: remove pci_{register,
    unregister}_secondary_bus()
  pci_bridge: introduce pci bridge library.

 Makefile.objs      |    2 +-
 hw/apb_pci.c       |   43 ++++++---
 hw/dec_pci.c       |   35 ++++++--
 hw/pci.c           |  207 +------------------------------------------
 hw/pci.h           |    5 +-
 hw/pci_bridge.c    |  249 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci_bridge.h    |   62 +++++++++++++
 hw/pci_internals.h |   42 +++++++++
 qemu-common.h      |    1 +
 9 files changed, 416 insertions(+), 230 deletions(-)
 create mode 100644 hw/pci_bridge.c
 create mode 100644 hw/pci_bridge.h
 create mode 100644 hw/pci_internals.h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 1/5] pci: move out pci internal structures, PCIBus, PCIBridge, and pci_bus_info.
  2010-07-12 10:36 [Qemu-devel] [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Isaku Yamahata
@ 2010-07-12 10:36 ` Isaku Yamahata
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 2/5] pci/bridge: split out pci bridge code into pci_bridge.c from pci.c Isaku Yamahata
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Isaku Yamahata @ 2010-07-12 10:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: blauwirbel, yamahata, mst

move out pci internal structures, PCIBus, PCIBridge and pci_bus_info into
private header file, pci_internals.h.
This is a preparation. Later pci bridge implementation will be
split out form pci.c into pci_bridge.c.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c           |   32 ++------------------------------
 hw/pci_internals.h |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 30 deletions(-)
 create mode 100644 hw/pci_internals.h

diff --git a/hw/pci.c b/hw/pci.c
index a3c2873..41227bf 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -23,6 +23,7 @@
  */
 #include "hw.h"
 #include "pci.h"
+#include "pci_internals.h"
 #include "monitor.h"
 #include "net.h"
 #include "sysemu.h"
@@ -36,31 +37,10 @@
 # define PCI_DPRINTF(format, ...)       do { } while (0)
 #endif
 
-struct PCIBus {
-    BusState qbus;
-    int devfn_min;
-    pci_set_irq_fn set_irq;
-    pci_map_irq_fn map_irq;
-    pci_hotplug_fn hotplug;
-    DeviceState *hotplug_qdev;
-    void *irq_opaque;
-    PCIDevice *devices[256];
-    PCIDevice *parent_dev;
-    target_phys_addr_t mem_base;
-
-    QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
-    QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */
-
-    /* The bus IRQ state is the logical OR of the connected devices.
-       Keep a count of the number of devices with raised IRQs.  */
-    int nirq;
-    int *irq_count;
-};
-
 static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *pcibus_get_dev_path(DeviceState *dev);
 
-static struct BusInfo pci_bus_info = {
+struct BusInfo pci_bus_info = {
     .name       = "PCI",
     .size       = sizeof(PCIBus),
     .print_dev  = pcibus_dev_print,
@@ -1524,14 +1504,6 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
     return res;
 }
 
-typedef struct {
-    PCIDevice dev;
-    PCIBus bus;
-    uint32_t vid;
-    uint32_t did;
-} PCIBridge;
-
-
 static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
 {
     pci_update_mappings(d);
diff --git a/hw/pci_internals.h b/hw/pci_internals.h
new file mode 100644
index 0000000..8a3026b
--- /dev/null
+++ b/hw/pci_internals.h
@@ -0,0 +1,40 @@
+#ifndef QEMU_PCI_INTERNALS_H
+#define QEMU_PCI_INTERNALS_H
+
+/*
+ * This header files is private to pci.c and pci_bridge.c
+ * So following structures are opaque to others and shouldn't be
+ * accessed.
+ */
+
+extern struct BusInfo pci_bus_info;
+
+struct PCIBus {
+    BusState qbus;
+    int devfn_min;
+    pci_set_irq_fn set_irq;
+    pci_map_irq_fn map_irq;
+    pci_hotplug_fn hotplug;
+    DeviceState *hotplug_qdev;
+    void *irq_opaque;
+    PCIDevice *devices[256];
+    PCIDevice *parent_dev;
+    target_phys_addr_t mem_base;
+
+    QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
+    QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */
+
+    /* The bus IRQ state is the logical OR of the connected devices.
+       Keep a count of the number of devices with raised IRQs.  */
+    int nirq;
+    int *irq_count;
+};
+
+typedef struct {
+    PCIDevice dev;
+    PCIBus bus;
+    uint32_t vid;
+    uint32_t did;
+} PCIBridge;
+
+#endif /* QEMU_PCI_INTERNALS_H */
-- 
1.7.1.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 2/5] pci/bridge: split out pci bridge code into pci_bridge.c from pci.c
  2010-07-12 10:36 [Qemu-devel] [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Isaku Yamahata
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 1/5] pci: move out pci internal structures, PCIBus, PCIBridge, and pci_bus_info Isaku Yamahata
@ 2010-07-12 10:36 ` Isaku Yamahata
  2010-07-12 12:16   ` [Qemu-devel] " Michael S. Tsirkin
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 3/5] pci_bridge: rename PCIBridge::bus -> PCIBridge::sec_bus Isaku Yamahata
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Isaku Yamahata @ 2010-07-12 10:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: blauwirbel, yamahata, mst

Move pci bridge related code into pci_bridge.c from pci.c
for further enhancement. pci.c is big enough now, so split it out.
No code change but exporting some accesser functions.

In fact, few pci bridge functions stays in pci.c.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 Makefile.objs   |    2 +-
 hw/apb_pci.c    |    1 +
 hw/dec_pci.c    |    1 +
 hw/pci.c        |  175 +----------------------------------------------
 hw/pci.h        |    5 +-
 hw/pci_bridge.c |  206 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci_bridge.h |   48 +++++++++++++
 7 files changed, 260 insertions(+), 178 deletions(-)
 create mode 100644 hw/pci_bridge.c
 create mode 100644 hw/pci_bridge.h

diff --git a/Makefile.objs b/Makefile.objs
index 67f1b21..594894b 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -139,7 +139,7 @@ user-obj-y += cutils.o cache-utils.o
 hw-obj-y =
 hw-obj-y += vl.o loader.o
 hw-obj-y += virtio.o virtio-console.o
-hw-obj-y += fw_cfg.o pci.o pci_host.o pcie_host.o
+hw-obj-y += fw_cfg.o pci.o pci_host.o pcie_host.o pci_bridge.o
 hw-obj-y += watchdog.o
 hw-obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
 hw-obj-$(CONFIG_ECC) += ecc.o
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 0ecac55..88ee4a9 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -29,6 +29,7 @@
 #include "sysbus.h"
 #include "pci.h"
 #include "pci_host.h"
+#include "pci_bridge.h"
 #include "rwhandler.h"
 #include "apb_pci.h"
 #include "sysemu.h"
diff --git a/hw/dec_pci.c b/hw/dec_pci.c
index ee49d5a..f7a9cdc 100644
--- a/hw/dec_pci.c
+++ b/hw/dec_pci.c
@@ -27,6 +27,7 @@
 #include "sysbus.h"
 #include "pci.h"
 #include "pci_host.h"
+#include "pci_bridge.h"
 
 /* debug DEC */
 //#define DEBUG_DEC
diff --git a/hw/pci.c b/hw/pci.c
index 41227bf..737fbd2 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -23,6 +23,7 @@
  */
 #include "hw.h"
 #include "pci.h"
+#include "pci_bridge.h"
 #include "pci_internals.h"
 #include "monitor.h"
 #include "net.h"
@@ -263,26 +264,6 @@ 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)
-{
-    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
-    bus->map_irq = map_irq;
-    bus->parent_dev = dev;
-
-    QLIST_INIT(&bus->child);
-    QLIST_INSERT_HEAD(&parent->child, bus, sibling);
-}
-
-static void pci_unregister_secondary_bus(PCIBus *bus)
-{
-    assert(QLIST_EMPTY(&bus->child));
-    QLIST_REMOVE(bus, sibling);
-}
-
 int pci_bus_num(PCIBus *s)
 {
     if (!s->parent_dev)
@@ -790,75 +771,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     }
 }
 
-static uint32_t pci_config_get_io_base(PCIDevice *d,
-                                       uint32_t base, uint32_t base_upper16)
-{
-    uint32_t val;
-
-    val = ((uint32_t)d->config[base] & PCI_IO_RANGE_MASK) << 8;
-    if (d->config[base] & PCI_IO_RANGE_TYPE_32) {
-        val |= (uint32_t)pci_get_word(d->config + base_upper16) << 16;
-    }
-    return val;
-}
-
-static pcibus_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
-{
-    return ((pcibus_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
-        << 16;
-}
-
-static pcibus_t pci_config_get_pref_base(PCIDevice *d,
-                                         uint32_t base, uint32_t upper)
-{
-    pcibus_t tmp;
-    pcibus_t val;
-
-    tmp = (pcibus_t)pci_get_word(d->config + base);
-    val = (tmp & PCI_PREF_RANGE_MASK) << 16;
-    if (tmp & PCI_PREF_RANGE_TYPE_64) {
-        val |= (pcibus_t)pci_get_long(d->config + upper) << 32;
-    }
-    return val;
-}
-
-static pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type)
-{
-    pcibus_t base;
-    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
-        base = pci_config_get_io_base(bridge,
-                                      PCI_IO_BASE, PCI_IO_BASE_UPPER16);
-    } else {
-        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
-            base = pci_config_get_pref_base(
-                bridge, PCI_PREF_MEMORY_BASE, PCI_PREF_BASE_UPPER32);
-        } else {
-            base = pci_config_get_memory_base(bridge, PCI_MEMORY_BASE);
-        }
-    }
-
-    return base;
-}
-
-static pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
-{
-    pcibus_t limit;
-    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
-        limit = pci_config_get_io_base(bridge,
-                                      PCI_IO_LIMIT, PCI_IO_LIMIT_UPPER16);
-        limit |= 0xfff;         /* PCI bridge spec 3.2.5.6. */
-    } else {
-        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
-            limit = pci_config_get_pref_base(
-                bridge, PCI_PREF_MEMORY_LIMIT, PCI_PREF_LIMIT_UPPER32);
-        } else {
-            limit = pci_config_get_memory_base(bridge, PCI_MEMORY_LIMIT);
-        }
-        limit |= 0xfffff;       /* PCI bridge spec 3.2.5.{1, 8}. */
-    }
-    return limit;
-}
-
 static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
                               uint8_t type)
 {
@@ -1509,7 +1421,7 @@ static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
     pci_update_mappings(d);
 }
 
-static void pci_bridge_update_mappings(PCIBus *b)
+void pci_bridge_update_mappings(PCIBus *b)
 {
     PCIBus *child;
 
@@ -1520,21 +1432,6 @@ static void pci_bridge_update_mappings(PCIBus *b)
     }
 }
 
-static void pci_bridge_write_config(PCIDevice *d,
-                             uint32_t address, uint32_t val, int len)
-{
-    pci_default_write_config(d, address, val, len);
-
-    if (/* io base/limit */
-        ranges_overlap(address, len, PCI_IO_BASE, 2) ||
-
-        /* memory base/limit, prefetchable base/limit and
-           io base/limit upper 16 */
-        ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
-        pci_bridge_update_mappings(d->bus);
-    }
-}
-
 PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 {
     PCIBus *sec;
@@ -1578,54 +1475,6 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
     return bus->devices[PCI_DEVFN(slot, function)];
 }
 
-static int pci_bridge_initfn(PCIDevice *dev)
-{
-    PCIBridge *s = DO_UPCAST(PCIBridge, dev, dev);
-
-    pci_config_set_vendor_id(s->dev.config, s->vid);
-    pci_config_set_device_id(s->dev.config, s->did);
-
-    pci_set_word(dev->config + PCI_STATUS,
-                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
-    pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_PCI);
-    dev->config[PCI_HEADER_TYPE] =
-        (dev->config[PCI_HEADER_TYPE] & PCI_HEADER_TYPE_MULTI_FUNCTION) |
-        PCI_HEADER_TYPE_BRIDGE;
-    pci_set_word(dev->config + PCI_SEC_STATUS,
-                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
-    return 0;
-}
-
-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);
-    return 0;
-}
-
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
-                        uint16_t vid, uint16_t did,
-                        pci_map_irq_fn map_irq, const char *name)
-{
-    PCIDevice *dev;
-    PCIBridge *s;
-
-    dev = pci_create_multifunction(bus, devfn, multifunction, "pci-bridge");
-    qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
-    qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
-    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;
-}
-
-PCIDevice *pci_bridge_get_device(PCIBus *bus)
-{
-    return bus->parent_dev;
-}
-
 static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
 {
     PCIDevice *pci_dev = (PCIDevice *)qdev;
@@ -1926,23 +1775,3 @@ static char *pcibus_get_dev_path(DeviceState *dev)
     return strdup(path);
 }
 
-static PCIDeviceInfo bridge_info = {
-    .qdev.name    = "pci-bridge",
-    .qdev.size    = sizeof(PCIBridge),
-    .init         = pci_bridge_initfn,
-    .exit         = pci_bridge_exitfn,
-    .config_write = pci_bridge_write_config,
-    .is_bridge    = 1,
-    .qdev.props   = (Property[]) {
-        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
-        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
-        DEFINE_PROP_END_OF_LIST(),
-    }
-};
-
-static void pci_register_devices(void)
-{
-    pci_qdev_register(&bridge_info);
-}
-
-device_init(pci_register_devices)
diff --git a/hw/pci.h b/hw/pci.h
index 1eab7e7..c551f96 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -233,10 +233,7 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
 
 void do_pci_info_print(Monitor *mon, const QObject *data);
 void do_pci_info(Monitor *mon, QObject **ret_data);
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
-                        uint16_t vid, uint16_t did,
-                        pci_map_irq_fn map_irq, const char *name);
-PCIDevice *pci_bridge_get_device(PCIBus *bus);
+void pci_bridge_update_mappings(PCIBus *b);
 
 static inline void
 pci_set_byte(uint8_t *config, uint8_t val)
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
new file mode 100644
index 0000000..3f69a44
--- /dev/null
+++ b/hw/pci_bridge.c
@@ -0,0 +1,206 @@
+/*
+ * QEMU PCI bus manager
+ *
+ * Copyright (c) 2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to dea
+
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM
+
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+/*
+ * split out from pci.c
+ * Copyright (c) 2010 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ */
+
+#include "pci_bridge.h"
+#include "pci_internals.h"
+
+PCIDevice *pci_bridge_get_device(PCIBus *bus)
+{
+    return bus->parent_dev;
+}
+
+static void pci_register_secondary_bus(PCIBus *parent,
+                                       PCIBus *bus,
+                                       PCIDevice *dev,
+                                       pci_map_irq_fn map_irq,
+                                       const char *name)
+{
+    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
+    bus->map_irq = map_irq;
+    bus->parent_dev = dev;
+
+    QLIST_INIT(&bus->child);
+    QLIST_INSERT_HEAD(&parent->child, bus, sibling);
+}
+
+static void pci_unregister_secondary_bus(PCIBus *bus)
+{
+    assert(QLIST_EMPTY(&bus->child));
+    QLIST_REMOVE(bus, sibling);
+}
+
+static uint32_t pci_config_get_io_base(PCIDevice *d,
+                                       uint32_t base, uint32_t base_upper16)
+{
+    uint32_t val;
+
+    val = ((uint32_t)d->config[base] & PCI_IO_RANGE_MASK) << 8;
+    if (d->config[base] & PCI_IO_RANGE_TYPE_32) {
+        val |= (uint32_t)pci_get_word(d->config + base_upper16) << 16;
+    }
+    return val;
+}
+
+static pcibus_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
+{
+    return ((pcibus_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
+        << 16;
+}
+
+static pcibus_t pci_config_get_pref_base(PCIDevice *d,
+                                         uint32_t base, uint32_t upper)
+{
+    pcibus_t tmp;
+    pcibus_t val;
+
+    tmp = (pcibus_t)pci_get_word(d->config + base);
+    val = (tmp & PCI_PREF_RANGE_MASK) << 16;
+    if (tmp & PCI_PREF_RANGE_TYPE_64) {
+        val |= (pcibus_t)pci_get_long(d->config + upper) << 32;
+    }
+    return val;
+}
+
+pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type)
+{
+    pcibus_t base;
+    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
+        base = pci_config_get_io_base(bridge,
+                                      PCI_IO_BASE, PCI_IO_BASE_UPPER16);
+    } else {
+        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
+            base = pci_config_get_pref_base(
+                bridge, PCI_PREF_MEMORY_BASE, PCI_PREF_BASE_UPPER32);
+        } else {
+            base = pci_config_get_memory_base(bridge, PCI_MEMORY_BASE);
+        }
+    }
+
+    return base;
+}
+
+pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
+{
+    pcibus_t limit;
+    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
+        limit = pci_config_get_io_base(bridge,
+                                      PCI_IO_LIMIT, PCI_IO_LIMIT_UPPER16);
+        limit |= 0xfff;         /* PCI bridge spec 3.2.5.6. */
+    } else {
+        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
+            limit = pci_config_get_pref_base(
+                bridge, PCI_PREF_MEMORY_LIMIT, PCI_PREF_LIMIT_UPPER32);
+        } else {
+            limit = pci_config_get_memory_base(bridge, PCI_MEMORY_LIMIT);
+        }
+        limit |= 0xfffff;       /* PCI bridge spec 3.2.5.{1, 8}. */
+    }
+    return limit;
+}
+
+static void pci_bridge_write_config(PCIDevice *d,
+                             uint32_t address, uint32_t val, int len)
+{
+    pci_default_write_config(d, address, val, len);
+
+    if (/* io base/limit */
+        ranges_overlap(address, len, PCI_IO_BASE, 2) ||
+
+        /* memory base/limit, prefetchable base/limit and
+           io base/limit upper 16 */
+        ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
+        pci_bridge_update_mappings(d->bus);
+    }
+}
+
+static int pci_bridge_initfn(PCIDevice *dev)
+{
+    PCIBridge *s = DO_UPCAST(PCIBridge, dev, dev);
+
+    pci_config_set_vendor_id(s->dev.config, s->vid);
+    pci_config_set_device_id(s->dev.config, s->did);
+
+    pci_set_word(dev->config + PCI_STATUS,
+                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
+    pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_PCI);
+    dev->config[PCI_HEADER_TYPE] =
+        (dev->config[PCI_HEADER_TYPE] & PCI_HEADER_TYPE_MULTI_FUNCTION) |
+        PCI_HEADER_TYPE_BRIDGE;
+    pci_set_word(dev->config + PCI_SEC_STATUS,
+                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
+    return 0;
+}
+
+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);
+    return 0;
+}
+
+PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
+                        uint16_t vid, uint16_t did,
+                        pci_map_irq_fn map_irq, const char *name)
+{
+    PCIDevice *dev;
+    PCIBridge *s;
+
+    dev = pci_create_multifunction(bus, devfn, multifunction, "pci-bridge");
+    qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
+    qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
+    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;
+}
+
+static PCIDeviceInfo bridge_info = {
+    .qdev.name    = "pci-bridge",
+    .qdev.size    = sizeof(PCIBridge),
+    .init         = pci_bridge_initfn,
+    .exit         = pci_bridge_exitfn,
+    .config_write = pci_bridge_write_config,
+    .is_bridge    = 1,
+    .qdev.props   = (Property[]) {
+        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
+        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static void pci_register_devices(void)
+{
+    pci_qdev_register(&bridge_info);
+}
+
+device_init(pci_register_devices)
diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
new file mode 100644
index 0000000..ddb2c82
--- /dev/null
+++ b/hw/pci_bridge.h
@@ -0,0 +1,48 @@
+/*
+ * QEMU PCI bridge
+ *
+ * Copyright (c) 2004 Fabrice Bellard
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * split out pci bus specific stuff from pci.[hc] to pci_bridge.[hc]
+ * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ *
+ */
+
+#ifndef QEMU_PCI_BRIDGE_H
+#define QEMU_PCI_BRIDGE_H
+
+#include "pci.h"
+
+PCIDevice *pci_bridge_get_device(PCIBus *bus);
+
+pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type);
+pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type);
+
+PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
+                        uint16_t vid, uint16_t did,
+                        pci_map_irq_fn map_irq, const char *name);
+
+#endif  /* QEMU_PCI_BRIDGE_H */
+/*
+ * Local variables:
+ *  c-indent-level: 4
+ *  c-basic-offset: 4
+ *  tab-width: 8
+ *  indent-tab-mode: nil
+ * End:
+ */
-- 
1.7.1.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 3/5] pci_bridge: rename PCIBridge::bus -> PCIBridge::sec_bus.
  2010-07-12 10:36 [Qemu-devel] [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Isaku Yamahata
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 1/5] pci: move out pci internal structures, PCIBus, PCIBridge, and pci_bus_info Isaku Yamahata
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 2/5] pci/bridge: split out pci bridge code into pci_bridge.c from pci.c Isaku Yamahata
@ 2010-07-12 10:36 ` Isaku Yamahata
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 4/5] pci_bridge: clean up: remove pci_{register, unregister}_secondary_bus() Isaku Yamahata
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Isaku Yamahata @ 2010-07-12 10:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: blauwirbel, yamahata, mst

To avoid confusion of primary bus with secondary bus,
rename PCIBridge::bus to PCIBridge::sec_bus.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci_bridge.c    |    7 +++----
 hw/pci_internals.h |    2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index 3f69a44..a5c4ece 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -162,8 +162,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->sec_bus);
     return 0;
 }
 
@@ -180,8 +179,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;
+    pci_register_secondary_bus(bus, &s->sec_bus, &s->dev, map_irq, name);
+    return &s->sec_bus;
 }
 
 static PCIDeviceInfo bridge_info = {
diff --git a/hw/pci_internals.h b/hw/pci_internals.h
index 8a3026b..fa844ab 100644
--- a/hw/pci_internals.h
+++ b/hw/pci_internals.h
@@ -32,7 +32,7 @@ struct PCIBus {
 
 typedef struct {
     PCIDevice dev;
-    PCIBus bus;
+    PCIBus sec_bus;
     uint32_t vid;
     uint32_t did;
 } PCIBridge;
-- 
1.7.1.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 4/5] pci_bridge: clean up: remove pci_{register, unregister}_secondary_bus()
  2010-07-12 10:36 [Qemu-devel] [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Isaku Yamahata
                   ` (2 preceding siblings ...)
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 3/5] pci_bridge: rename PCIBridge::bus -> PCIBridge::sec_bus Isaku Yamahata
@ 2010-07-12 10:36 ` Isaku Yamahata
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 5/5] pci_bridge: introduce pci bridge library Isaku Yamahata
  2010-07-12 12:11 ` [Qemu-devel] Re: [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Michael S. Tsirkin
  5 siblings, 0 replies; 12+ messages in thread
From: Isaku Yamahata @ 2010-07-12 10:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: blauwirbel, yamahata, mst

Remove pci_{register, unregister}_secondary_bus() by open code.
They are old stype API and aren't used any more by others. So eliminate it.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci_bridge.c |   32 ++++++++++----------------------
 1 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index a5c4ece..16f930a 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -37,26 +37,6 @@ PCIDevice *pci_bridge_get_device(PCIBus *bus)
     return bus->parent_dev;
 }
 
-static void pci_register_secondary_bus(PCIBus *parent,
-                                       PCIBus *bus,
-                                       PCIDevice *dev,
-                                       pci_map_irq_fn map_irq,
-                                       const char *name)
-{
-    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
-    bus->map_irq = map_irq;
-    bus->parent_dev = dev;
-
-    QLIST_INIT(&bus->child);
-    QLIST_INSERT_HEAD(&parent->child, bus, sibling);
-}
-
-static void pci_unregister_secondary_bus(PCIBus *bus)
-{
-    assert(QLIST_EMPTY(&bus->child));
-    QLIST_REMOVE(bus, sibling);
-}
-
 static uint32_t pci_config_get_io_base(PCIDevice *d,
                                        uint32_t base, uint32_t base_upper16)
 {
@@ -162,7 +142,8 @@ static int pci_bridge_initfn(PCIDevice *dev)
 static int pci_bridge_exitfn(PCIDevice *pci_dev)
 {
     PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
-    pci_unregister_secondary_bus(&s->sec_bus);
+    assert(QLIST_EMPTY(&s->sec_bus.child));
+    QLIST_REMOVE(&s->sec_bus, sibling);
     return 0;
 }
 
@@ -172,6 +153,7 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
 {
     PCIDevice *dev;
     PCIBridge *s;
+    PCIBus *sec_bus;
 
     dev = pci_create_multifunction(bus, devfn, multifunction, "pci-bridge");
     qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
@@ -179,7 +161,13 @@ 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->sec_bus, &s->dev, map_irq, name);
+    sec_bus = &s->sec_bus;
+    qbus_create_inplace(&sec_bus->qbus, &pci_bus_info, &dev->qdev, name);
+    sec_bus->parent_dev = dev;
+    sec_bus->map_irq = map_irq;
+
+    QLIST_INIT(&sec_bus->child);
+    QLIST_INSERT_HEAD(&bus->child, sec_bus, sibling);
     return &s->sec_bus;
 }
 
-- 
1.7.1.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 5/5] pci_bridge: introduce pci bridge library.
  2010-07-12 10:36 [Qemu-devel] [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Isaku Yamahata
                   ` (3 preceding siblings ...)
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 4/5] pci_bridge: clean up: remove pci_{register, unregister}_secondary_bus() Isaku Yamahata
@ 2010-07-12 10:36 ` Isaku Yamahata
  2010-07-12 12:10   ` [Qemu-devel] " Michael S. Tsirkin
  2010-07-12 12:11 ` [Qemu-devel] Re: [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Michael S. Tsirkin
  5 siblings, 1 reply; 12+ messages in thread
From: Isaku Yamahata @ 2010-07-12 10:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: blauwirbel, yamahata, mst

introduce pci bridge library.
convert apb bridge and dec p2p bridge to use new pci bridge library.
save/restore is supported as a side effect.
This is also preparation for pci express root/upstream/downstream port.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/apb_pci.c       |   42 ++++++++++-----
 hw/dec_pci.c       |   34 +++++++++---
 hw/pci_bridge.c    |  154 +++++++++++++++++++++++++++++++++++-----------------
 hw/pci_bridge.h    |   24 +++++++--
 hw/pci_internals.h |   10 ++--
 qemu-common.h      |    1 +
 6 files changed, 185 insertions(+), 80 deletions(-)

diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 88ee4a9..d446550 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -31,6 +31,7 @@
 #include "pci_host.h"
 #include "pci_bridge.h"
 #include "rwhandler.h"
+#include "pci_bridge.h"
 #include "apb_pci.h"
 #include "sysemu.h"
 
@@ -294,9 +295,17 @@ static void pci_apb_set_irq(void *opaque, int irq_num, int level)
     }
 }
 
-static void apb_pci_bridge_init(PCIBus *b)
+static int apb_pci_bridge_initfn(PCIDevice *dev)
 {
-    PCIDevice *dev = pci_bridge_get_device(b);
+    int rc;
+
+    rc = pci_bridge_initfn(dev);
+    if (rc < 0) {
+        return rc;
+    }
+
+    pci_config_set_vendor_id(dev->config, PCI_VENDOR_ID_SUN);
+    pci_config_set_device_id(dev->config, PCI_DEVICE_ID_SUN_SIMBA);
 
     /*
      * command register:
@@ -313,6 +322,7 @@ static void apb_pci_bridge_init(PCIBus *b)
                  PCI_STATUS_FAST_BACK | PCI_STATUS_66MHZ |
                  PCI_STATUS_DEVSEL_MEDIUM);
     pci_set_byte(dev->config + PCI_REVISION_ID, 0x11);
+    return 0;
 }
 
 PCIBus *pci_apb_init(target_phys_addr_t special_base,
@@ -323,6 +333,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
     SysBusDevice *s;
     APBState *d;
     unsigned int i;
+    PCIBridge *br;
 
     /* Ultrasparc PBM main bus */
     dev = qdev_create(NULL, "pbm");
@@ -348,17 +359,15 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
     pci_create_simple(d->bus, 0, "pbm");
 
     /* APB secondary busses */
-    *bus2 = pci_bridge_init(d->bus, PCI_DEVFN(1, 0), true,
-                            PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_SIMBA,
-                            pci_apb_map_irq,
-                            "Advanced PCI Bus secondary bridge 1");
-    apb_pci_bridge_init(*bus2);
-
-    *bus3 = pci_bridge_init(d->bus, PCI_DEVFN(1, 1), true,
-                            PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_SIMBA,
-                            pci_apb_map_irq,
-                            "Advanced PCI Bus secondary bridge 2");
-    apb_pci_bridge_init(*bus3);
+    br = pci_bridge_create_simple(d->bus, PCI_DEVFN(1, 0), true,
+                                  pci_apb_map_irq, "pbm-bridge",
+                                  "Advanced PCI Bus secondary bridge 1");
+    *bus2 = pci_bridge_get_sec_bus(br);
+
+    br = pci_bridge_create_simple(d->bus, PCI_DEVFN(1, 1), true,
+                                  pci_apb_map_irq, "pbm-bridge",
+                                  "Advanced PCI Bus secondary bridge 2");
+    *bus3 = pci_bridge_get_sec_bus(br);
 
     return d->bus;
 }
@@ -441,10 +450,17 @@ static SysBusDeviceInfo pbm_host_info = {
     .qdev.reset = pci_pbm_reset,
     .init = pci_pbm_init_device,
 };
+
+static PCIDeviceInfo pbm_pci_bridge_info = {
+    .qdev.name = "pbm-bridge",
+    .init = apb_pci_bridge_initfn,
+};
+
 static void pbm_register_devices(void)
 {
     sysbus_register_withprop(&pbm_host_info);
     pci_qdev_register(&pbm_pci_host_info);
+    pci_bridge_qdev_register(&pbm_pci_bridge_info);
 }
 
 device_init(pbm_register_devices)
diff --git a/hw/dec_pci.c b/hw/dec_pci.c
index f7a9cdc..ce84faa 100644
--- a/hw/dec_pci.c
+++ b/hw/dec_pci.c
@@ -49,18 +49,33 @@ static int dec_map_irq(PCIDevice *pci_dev, int irq_num)
     return irq_num;
 }
 
-PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn)
+static int dec_21154_initfn(PCIDevice *dev)
 {
-    DeviceState *dev;
-    PCIBus *ret;
+    int rc;
+
+    rc = pci_bridge_initfn(dev);
+    if (rc < 0) {
+        return rc;
+    }
+
+    pci_config_set_vendor_id(dev->config, PCI_VENDOR_ID_DEC);
+    pci_config_set_device_id(dev->config, PCI_DEVICE_ID_DEC_21154);
+    return 0;
+}
 
-    dev = qdev_create(NULL, "dec-21154");
-    qdev_init_nofail(dev);
-    ret = pci_bridge_init(parent_bus, devfn, false,
-                          PCI_VENDOR_ID_DEC, PCI_DEVICE_ID_DEC_21154,
-                          dec_map_irq, "DEC 21154 PCI-PCI bridge");
+static PCIDeviceInfo dec_21154_pci_bridge_info = {
+    .qdev.name = "dec-21154-p2p-bridge",
+    .qdev.desc = "DEC 21154 PCI-PCI bridge",
+    .init = dec_21154_initfn,
+};
 
-    return ret;
+PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn)
+{
+    PCIBridge *br;
+    br = pci_bridge_create_simple(parent_bus, devfn, false, dec_map_irq,
+                                  "dec-21154-p2p-bridge",
+                                  "DEC 21154 PCI-PCI bridge");
+    return pci_bridge_get_sec_bus(br);
 }
 
 static int pci_dec_21154_init_device(SysBusDevice *dev)
@@ -99,6 +114,7 @@ static void dec_register_devices(void)
     sysbus_register_dev("dec-21154", sizeof(DECState),
                         pci_dec_21154_init_device);
     pci_qdev_register(&dec_21154_pci_host_info);
+    pci_bridge_qdev_register(&dec_21154_pci_bridge_info);
 }
 
 device_init(dec_register_devices)
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index 16f930a..f369dc3 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -32,12 +32,19 @@
 #include "pci_bridge.h"
 #include "pci_internals.h"
 
+/* Accessor function to get parent bridge device from pci bus. */
 PCIDevice *pci_bridge_get_device(PCIBus *bus)
 {
     return bus->parent_dev;
 }
 
-static uint32_t pci_config_get_io_base(PCIDevice *d,
+/* Accessor function to get secondary bus from pci-to-pci bridge device */
+PCIBus *pci_bridge_get_sec_bus(PCIBridge *br)
+{
+    return &br->sec_bus;
+}
+
+static uint32_t pci_config_get_io_base(const PCIDevice *d,
                                        uint32_t base, uint32_t base_upper16)
 {
     uint32_t val;
@@ -49,13 +56,13 @@ static uint32_t pci_config_get_io_base(PCIDevice *d,
     return val;
 }
 
-static pcibus_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
+static pcibus_t pci_config_get_memory_base(const PCIDevice *d, uint32_t base)
 {
     return ((pcibus_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
         << 16;
 }
 
-static pcibus_t pci_config_get_pref_base(PCIDevice *d,
+static pcibus_t pci_config_get_pref_base(const PCIDevice *d,
                                          uint32_t base, uint32_t upper)
 {
     pcibus_t tmp;
@@ -69,7 +76,8 @@ static pcibus_t pci_config_get_pref_base(PCIDevice *d,
     return val;
 }
 
-pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type)
+/* accessor function to get bridge filtering base address */
+pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type)
 {
     pcibus_t base;
     if (type & PCI_BASE_ADDRESS_SPACE_IO) {
@@ -87,7 +95,8 @@ pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type)
     return base;
 }
 
-pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
+/* accessor funciton to get bridge filtering limit */
+pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
 {
     pcibus_t limit;
     if (type & PCI_BASE_ADDRESS_SPACE_IO) {
@@ -106,9 +115,11 @@ pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
     return limit;
 }
 
-static void pci_bridge_write_config(PCIDevice *d,
+/* default write_config function for PCI-to-PCI bridge */
+void pci_bridge_write_config(PCIDevice *d,
                              uint32_t address, uint32_t val, int len)
 {
+    PCIBridge *br = DO_UPCAST(PCIBridge, dev, d);
     pci_default_write_config(d, address, val, len);
 
     if (/* io base/limit */
@@ -117,16 +128,45 @@ static void pci_bridge_write_config(PCIDevice *d,
         /* memory base/limit, prefetchable base/limit and
            io base/limit upper 16 */
         ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
-        pci_bridge_update_mappings(d->bus);
+        pci_bridge_update_mappings(&br->sec_bus);
     }
 }
 
-static int pci_bridge_initfn(PCIDevice *dev)
+/* reset bridge specific configuration registers */
+void pci_bridge_reset_reg(PCIDevice *dev)
 {
-    PCIBridge *s = DO_UPCAST(PCIBridge, dev, dev);
+    uint8_t *conf = dev->config;
+
+    conf[PCI_PRIMARY_BUS] = 0;
+    conf[PCI_SECONDARY_BUS] = 0;
+    conf[PCI_SUBORDINATE_BUS] = 0;
+    conf[PCI_SEC_LATENCY_TIMER] = 0;
+
+    conf[PCI_IO_BASE] = 0;
+    conf[PCI_IO_LIMIT] = 0;
+    pci_set_word(conf + PCI_MEMORY_BASE, 0);
+    pci_set_word(conf + PCI_MEMORY_LIMIT, 0);
+    pci_set_word(conf + PCI_PREF_MEMORY_BASE, 0);
+    pci_set_word(conf + PCI_PREF_MEMORY_LIMIT, 0);
+    pci_set_word(conf + PCI_PREF_BASE_UPPER32, 0);
+    pci_set_word(conf + PCI_PREF_LIMIT_UPPER32, 0);
+
+    pci_set_word(conf + PCI_BRIDGE_CONTROL, 0);
+}
 
-    pci_config_set_vendor_id(s->dev.config, s->vid);
-    pci_config_set_device_id(s->dev.config, s->did);
+/* default reset function for PCI-to-PCI bridge */
+void pci_bridge_reset(DeviceState *qdev)
+{
+    PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
+    pci_bridge_reset_reg(dev);
+}
+
+/* default qdev initialization function for PCI-to-PCI bridge */
+int pci_bridge_initfn(PCIDevice *dev)
+{
+    PCIBus *parent = dev->bus;
+    PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
+    PCIBus *sec_bus = &br->sec_bus;
 
     pci_set_word(dev->config + PCI_STATUS,
                  PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
@@ -136,58 +176,74 @@ static int pci_bridge_initfn(PCIDevice *dev)
         PCI_HEADER_TYPE_BRIDGE;
     pci_set_word(dev->config + PCI_SEC_STATUS,
                  PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
+
+    qbus_create_inplace(&sec_bus->qbus, &pci_bus_info, &dev->qdev,
+                        br->bus_name);
+    sec_bus->parent_dev = dev;
+    sec_bus->map_irq = br->map_irq;
+
+    QLIST_INIT(&sec_bus->child);
+    QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
     return 0;
 }
 
-static int pci_bridge_exitfn(PCIDevice *pci_dev)
+/* default qdev clean up function for PCI-to-PCI bridge */
+int pci_bridge_exitfn(PCIDevice *pci_dev)
 {
     PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
     assert(QLIST_EMPTY(&s->sec_bus.child));
     QLIST_REMOVE(&s->sec_bus, sibling);
+    /* qbus_free() is called automatically by qdev_free() */
     return 0;
 }
 
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
-                        uint16_t vid, uint16_t did,
-                        pci_map_irq_fn map_irq, const char *name)
+/*
+ * convenience function of pci_qdev_register for PCI-to-PCI bridge
+ * This function fills not-initialized members with default value.
+ */
+void pci_bridge_qdev_register(PCIDeviceInfo *info)
 {
-    PCIDevice *dev;
-    PCIBridge *s;
-    PCIBus *sec_bus;
-
-    dev = pci_create_multifunction(bus, devfn, multifunction, "pci-bridge");
-    qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
-    qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
-    qdev_init_nofail(&dev->qdev);
-
-    s = DO_UPCAST(PCIBridge, dev, dev);
-    sec_bus = &s->sec_bus;
-    qbus_create_inplace(&sec_bus->qbus, &pci_bus_info, &dev->qdev, name);
-    sec_bus->parent_dev = dev;
-    sec_bus->map_irq = map_irq;
-
-    QLIST_INIT(&sec_bus->child);
-    QLIST_INSERT_HEAD(&bus->child, sec_bus, sibling);
-    return &s->sec_bus;
-}
+    if (!info->qdev.size) {
+        info->qdev.size = sizeof(PCIBridge);
+    }
+    if (!info->qdev.vmsd) {
+        info->qdev.vmsd = &vmstate_pci_device;
+    }
 
-static PCIDeviceInfo bridge_info = {
-    .qdev.name    = "pci-bridge",
-    .qdev.size    = sizeof(PCIBridge),
-    .init         = pci_bridge_initfn,
-    .exit         = pci_bridge_exitfn,
-    .config_write = pci_bridge_write_config,
-    .is_bridge    = 1,
-    .qdev.props   = (Property[]) {
-        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
-        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
-        DEFINE_PROP_END_OF_LIST(),
+    assert(info->init); /* vid/did must be set at least */
+    if (!info->exit) {
+        info->exit = pci_bridge_exitfn;
     }
-};
+    if (!info->config_write) {
+        info->config_write = pci_bridge_write_config;
+    }
+    if (!info->qdev.reset) {
+        info->qdev.reset = pci_bridge_reset;
+    }
+
+    info->is_bridge = 1;
+    pci_qdev_register(info);
+}
 
-static void pci_register_devices(void)
+/* transitional function to create pci-to-pci bridge */
+PCIBridge *pci_bridge_create(PCIBus *bus, int devfn, bool multifunction,
+                             pci_map_irq_fn map_irq,
+                             const char *name, const char *bus_name)
 {
-    pci_qdev_register(&bridge_info);
+    PCIDevice *dev = pci_create_multifunction(bus, devfn, multifunction, name);
+    PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
+    br->map_irq = map_irq;
+    br->bus_name = bus_name;
+    return br;
 }
 
-device_init(pci_register_devices)
+/* convenience function = pci_bridge_create() + qdev_init_nofail() */
+PCIBridge *pci_bridge_create_simple(PCIBus *bus, int devfn, bool multifunction,
+                                    pci_map_irq_fn map_irq,
+                                    const char *name, const char *bus_name)
+{
+    PCIBridge *br = pci_bridge_create(bus, devfn, multifunction, map_irq,
+                                      name, bus_name);
+    qdev_init_nofail(&br->dev.qdev);
+    return br;
+}
diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
index ddb2c82..4697c7a 100644
--- a/hw/pci_bridge.h
+++ b/hw/pci_bridge.h
@@ -29,13 +29,27 @@
 #include "pci.h"
 
 PCIDevice *pci_bridge_get_device(PCIBus *bus);
+PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
 
-pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type);
-pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type);
+pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
+pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
 
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
-                        uint16_t vid, uint16_t did,
-                        pci_map_irq_fn map_irq, const char *name);
+void pci_bridge_write_config(PCIDevice *d,
+                             uint32_t address, uint32_t val, int len);
+void pci_bridge_reset_reg(PCIDevice *dev);
+void pci_bridge_reset(DeviceState *qdev);
+
+int pci_bridge_initfn(PCIDevice *pci_dev);
+int pci_bridge_exitfn(PCIDevice *pci_dev);
+
+void pci_bridge_qdev_register(PCIDeviceInfo *info);
+
+PCIBridge *pci_bridge_create(PCIBus *bus, int devfn, bool multifunction,
+                             pci_map_irq_fn map_irq,
+                             const char *name, const char *bus_name);
+PCIBridge *pci_bridge_create_simple(PCIBus *bus, int devfn, bool multifunction,
+                                    pci_map_irq_fn map_irq,
+                                    const char *name, const char *bus_name);
 
 #endif  /* QEMU_PCI_BRIDGE_H */
 /*
diff --git a/hw/pci_internals.h b/hw/pci_internals.h
index fa844ab..6502f83 100644
--- a/hw/pci_internals.h
+++ b/hw/pci_internals.h
@@ -30,11 +30,13 @@ struct PCIBus {
     int *irq_count;
 };
 
-typedef struct {
+struct PCIBridge {
     PCIDevice dev;
+
+    /* private member */
     PCIBus sec_bus;
-    uint32_t vid;
-    uint32_t did;
-} PCIBridge;
+    pci_map_irq_fn map_irq;
+    const char *bus_name;
+};
 
 #endif /* QEMU_PCI_INTERNALS_H */
diff --git a/qemu-common.h b/qemu-common.h
index 3fb2f0b..d735235 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -219,6 +219,7 @@ typedef struct PCIHostState PCIHostState;
 typedef struct PCIExpressHost PCIExpressHost;
 typedef struct PCIBus PCIBus;
 typedef struct PCIDevice PCIDevice;
+typedef struct PCIBridge PCIBridge;
 typedef struct SerialState SerialState;
 typedef struct IRQState *qemu_irq;
 typedef struct PCMCIACardState PCMCIACardState;
-- 
1.7.1.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] Re: [PATCH v2 5/5] pci_bridge: introduce pci bridge library.
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 5/5] pci_bridge: introduce pci bridge library Isaku Yamahata
@ 2010-07-12 12:10   ` Michael S. Tsirkin
  2010-07-12 13:28     ` Isaku Yamahata
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2010-07-12 12:10 UTC (permalink / raw)
  To: Isaku Yamahata; +Cc: blauwirbel, qemu-devel

On Mon, Jul 12, 2010 at 07:36:44PM +0900, Isaku Yamahata wrote:
> diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
> index ddb2c82..4697c7a 100644
> --- a/hw/pci_bridge.h
> +++ b/hw/pci_bridge.h
> @@ -29,13 +29,27 @@
>  #include "pci.h"
>  
>  PCIDevice *pci_bridge_get_device(PCIBus *bus);
> +PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
>  
> -pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type);
> -pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type);
> +pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
> +pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
>  
> -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> -                        uint16_t vid, uint16_t did,
> -                        pci_map_irq_fn map_irq, const char *name);
> +void pci_bridge_write_config(PCIDevice *d,
> +                             uint32_t address, uint32_t val, int len);
> +void pci_bridge_reset_reg(PCIDevice *dev);
> +void pci_bridge_reset(DeviceState *qdev);
> +
> +int pci_bridge_initfn(PCIDevice *pci_dev);
> +int pci_bridge_exitfn(PCIDevice *pci_dev);
> +
> +void pci_bridge_qdev_register(PCIDeviceInfo *info);
> +
> +PCIBridge *pci_bridge_create(PCIBus *bus, int devfn, bool multifunction,
> +                             pci_map_irq_fn map_irq,
> +                             const char *name, const char *bus_name);
> +PCIBridge *pci_bridge_create_simple(PCIBus *bus, int devfn, bool multifunction,
> +                                    pci_map_irq_fn map_irq,
> +                                    const char *name, const char *bus_name);

The APIs leave much to be desired.  _simple and regular are same?  What does _register do?

We really should just use qdev: Can't we use pci_qdev_register_many and
pci_create to create the bridge?  Long term, all pci_create variants
should go and get replaced with qdev_create.

>  
>  #endif  /* QEMU_PCI_BRIDGE_H */
>  /*
> diff --git a/hw/pci_internals.h b/hw/pci_internals.h
> index fa844ab..6502f83 100644
> --- a/hw/pci_internals.h
> +++ b/hw/pci_internals.h
> @@ -30,11 +30,13 @@ struct PCIBus {
>      int *irq_count;
>  };
>  
> -typedef struct {
> +struct PCIBridge {
>      PCIDevice dev;
> +
> +    /* private member */
>      PCIBus sec_bus;
> -    uint32_t vid;
> -    uint32_t did;
> -} PCIBridge;
> +    pci_map_irq_fn map_irq;
> +    const char *bus_name;
> +};
>  
>  #endif /* QEMU_PCI_INTERNALS_H */
> diff --git a/qemu-common.h b/qemu-common.h
> index 3fb2f0b..d735235 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -219,6 +219,7 @@ typedef struct PCIHostState PCIHostState;
>  typedef struct PCIExpressHost PCIExpressHost;
>  typedef struct PCIBus PCIBus;
>  typedef struct PCIDevice PCIDevice;
> +typedef struct PCIBridge PCIBridge;
>  typedef struct SerialState SerialState;
>  typedef struct IRQState *qemu_irq;
>  typedef struct PCMCIACardState PCMCIACardState;
> -- 
> 1.7.1.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] Re: [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library
  2010-07-12 10:36 [Qemu-devel] [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Isaku Yamahata
                   ` (4 preceding siblings ...)
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 5/5] pci_bridge: introduce pci bridge library Isaku Yamahata
@ 2010-07-12 12:11 ` Michael S. Tsirkin
  5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2010-07-12 12:11 UTC (permalink / raw)
  To: Isaku Yamahata; +Cc: blauwirbel, qemu-devel

On Mon, Jul 12, 2010 at 07:36:39PM +0900, Isaku Yamahata wrote:
> changes v1 -> v2:
> - introduce pci_internals.h to accomodate pci internal strcutures to
>   share between pci.c and pci_bridge.c
> - don't make PCIBridge::bus pointer as suggested by
>   Michael S. Tsirkin <mst@redhat.com>
> - rename PCIBridge::bus -> PCIBridge::sec_bus
> - eliminate pci_reguster_secondary_bus()/pci_unregister_secondary_bus()
> - document pci bridge library functions.
> - introduced pci bridge library.

I've applied patches 1-4 to make it easier to build upon.
Pushed in pci branch in my tree.
Sent comments on patch 5.

> Clean up of pci host bus ans piix pci as discussed with v1
> will be addressed after this patch set is accepted.
> 
> Patch description:
> Now pci.c has grown. So split bridge related code into dedicated file
> for further extension to pci bridge. Further clean up and pcie port emulator. 
> This make patch conflict less possible in future.
> 
> Isaku Yamahata (5):
>   pci: move out pci internal structures, PCIBus, PCIBridge, and
>     pci_bus_info.
>   pci/bridge: split out pci bridge code into pci_bridge.c from pci.c
>   pci_bridge: rename PCIBridge::bus -> PCIBridge::sec_bus.
>   pci_bridge: clean up: remove pci_{register,
>     unregister}_secondary_bus()
>   pci_bridge: introduce pci bridge library.
> 
>  Makefile.objs      |    2 +-
>  hw/apb_pci.c       |   43 ++++++---
>  hw/dec_pci.c       |   35 ++++++--
>  hw/pci.c           |  207 +------------------------------------------
>  hw/pci.h           |    5 +-
>  hw/pci_bridge.c    |  249 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci_bridge.h    |   62 +++++++++++++
>  hw/pci_internals.h |   42 +++++++++
>  qemu-common.h      |    1 +
>  9 files changed, 416 insertions(+), 230 deletions(-)
>  create mode 100644 hw/pci_bridge.c
>  create mode 100644 hw/pci_bridge.h
>  create mode 100644 hw/pci_internals.h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] Re: [PATCH v2 2/5] pci/bridge: split out pci bridge code into pci_bridge.c from pci.c
  2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 2/5] pci/bridge: split out pci bridge code into pci_bridge.c from pci.c Isaku Yamahata
@ 2010-07-12 12:16   ` Michael S. Tsirkin
  2010-07-12 13:22     ` Isaku Yamahata
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2010-07-12 12:16 UTC (permalink / raw)
  To: Isaku Yamahata; +Cc: blauwirbel, qemu-devel

On Mon, Jul 12, 2010 at 07:36:41PM +0900, Isaku Yamahata wrote:
> Move pci bridge related code into pci_bridge.c from pci.c
> for further enhancement. pci.c is big enough now, so split it out.
> No code change but exporting some accesser functions.
> 
> In fact, few pci bridge functions stays in pci.c.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Got conflicts starting with this patch.
Could you please rebase to my pci tree and repost patches 2-4?

> ---
>  Makefile.objs   |    2 +-
>  hw/apb_pci.c    |    1 +
>  hw/dec_pci.c    |    1 +
>  hw/pci.c        |  175 +----------------------------------------------
>  hw/pci.h        |    5 +-
>  hw/pci_bridge.c |  206 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci_bridge.h |   48 +++++++++++++
>  7 files changed, 260 insertions(+), 178 deletions(-)
>  create mode 100644 hw/pci_bridge.c
>  create mode 100644 hw/pci_bridge.h
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 67f1b21..594894b 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -139,7 +139,7 @@ user-obj-y += cutils.o cache-utils.o
>  hw-obj-y =
>  hw-obj-y += vl.o loader.o
>  hw-obj-y += virtio.o virtio-console.o
> -hw-obj-y += fw_cfg.o pci.o pci_host.o pcie_host.o
> +hw-obj-y += fw_cfg.o pci.o pci_host.o pcie_host.o pci_bridge.o
>  hw-obj-y += watchdog.o
>  hw-obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
>  hw-obj-$(CONFIG_ECC) += ecc.o
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 0ecac55..88ee4a9 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -29,6 +29,7 @@
>  #include "sysbus.h"
>  #include "pci.h"
>  #include "pci_host.h"
> +#include "pci_bridge.h"
>  #include "rwhandler.h"
>  #include "apb_pci.h"
>  #include "sysemu.h"
> diff --git a/hw/dec_pci.c b/hw/dec_pci.c
> index ee49d5a..f7a9cdc 100644
> --- a/hw/dec_pci.c
> +++ b/hw/dec_pci.c
> @@ -27,6 +27,7 @@
>  #include "sysbus.h"
>  #include "pci.h"
>  #include "pci_host.h"
> +#include "pci_bridge.h"
>  
>  /* debug DEC */
>  //#define DEBUG_DEC
> diff --git a/hw/pci.c b/hw/pci.c
> index 41227bf..737fbd2 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -23,6 +23,7 @@
>   */
>  #include "hw.h"
>  #include "pci.h"
> +#include "pci_bridge.h"
>  #include "pci_internals.h"
>  #include "monitor.h"
>  #include "net.h"
> @@ -263,26 +264,6 @@ 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)
> -{
> -    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
> -    bus->map_irq = map_irq;
> -    bus->parent_dev = dev;
> -
> -    QLIST_INIT(&bus->child);
> -    QLIST_INSERT_HEAD(&parent->child, bus, sibling);
> -}
> -
> -static void pci_unregister_secondary_bus(PCIBus *bus)
> -{
> -    assert(QLIST_EMPTY(&bus->child));
> -    QLIST_REMOVE(bus, sibling);
> -}
> -
>  int pci_bus_num(PCIBus *s)
>  {
>      if (!s->parent_dev)
> @@ -790,75 +771,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      }
>  }
>  
> -static uint32_t pci_config_get_io_base(PCIDevice *d,
> -                                       uint32_t base, uint32_t base_upper16)
> -{
> -    uint32_t val;
> -
> -    val = ((uint32_t)d->config[base] & PCI_IO_RANGE_MASK) << 8;
> -    if (d->config[base] & PCI_IO_RANGE_TYPE_32) {
> -        val |= (uint32_t)pci_get_word(d->config + base_upper16) << 16;
> -    }
> -    return val;
> -}
> -
> -static pcibus_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
> -{
> -    return ((pcibus_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
> -        << 16;
> -}
> -
> -static pcibus_t pci_config_get_pref_base(PCIDevice *d,
> -                                         uint32_t base, uint32_t upper)
> -{
> -    pcibus_t tmp;
> -    pcibus_t val;
> -
> -    tmp = (pcibus_t)pci_get_word(d->config + base);
> -    val = (tmp & PCI_PREF_RANGE_MASK) << 16;
> -    if (tmp & PCI_PREF_RANGE_TYPE_64) {
> -        val |= (pcibus_t)pci_get_long(d->config + upper) << 32;
> -    }
> -    return val;
> -}
> -
> -static pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type)
> -{
> -    pcibus_t base;
> -    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> -        base = pci_config_get_io_base(bridge,
> -                                      PCI_IO_BASE, PCI_IO_BASE_UPPER16);
> -    } else {
> -        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
> -            base = pci_config_get_pref_base(
> -                bridge, PCI_PREF_MEMORY_BASE, PCI_PREF_BASE_UPPER32);
> -        } else {
> -            base = pci_config_get_memory_base(bridge, PCI_MEMORY_BASE);
> -        }
> -    }
> -
> -    return base;
> -}
> -
> -static pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
> -{
> -    pcibus_t limit;
> -    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> -        limit = pci_config_get_io_base(bridge,
> -                                      PCI_IO_LIMIT, PCI_IO_LIMIT_UPPER16);
> -        limit |= 0xfff;         /* PCI bridge spec 3.2.5.6. */
> -    } else {
> -        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
> -            limit = pci_config_get_pref_base(
> -                bridge, PCI_PREF_MEMORY_LIMIT, PCI_PREF_LIMIT_UPPER32);
> -        } else {
> -            limit = pci_config_get_memory_base(bridge, PCI_MEMORY_LIMIT);
> -        }
> -        limit |= 0xfffff;       /* PCI bridge spec 3.2.5.{1, 8}. */
> -    }
> -    return limit;
> -}
> -
>  static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>                                uint8_t type)
>  {
> @@ -1509,7 +1421,7 @@ static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
>      pci_update_mappings(d);
>  }
>  
> -static void pci_bridge_update_mappings(PCIBus *b)
> +void pci_bridge_update_mappings(PCIBus *b)
>  {
>      PCIBus *child;
>  
> @@ -1520,21 +1432,6 @@ static void pci_bridge_update_mappings(PCIBus *b)
>      }
>  }
>  
> -static void pci_bridge_write_config(PCIDevice *d,
> -                             uint32_t address, uint32_t val, int len)
> -{
> -    pci_default_write_config(d, address, val, len);
> -
> -    if (/* io base/limit */
> -        ranges_overlap(address, len, PCI_IO_BASE, 2) ||
> -
> -        /* memory base/limit, prefetchable base/limit and
> -           io base/limit upper 16 */
> -        ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
> -        pci_bridge_update_mappings(d->bus);
> -    }
> -}
> -
>  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>  {
>      PCIBus *sec;
> @@ -1578,54 +1475,6 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
>      return bus->devices[PCI_DEVFN(slot, function)];
>  }
>  
> -static int pci_bridge_initfn(PCIDevice *dev)
> -{
> -    PCIBridge *s = DO_UPCAST(PCIBridge, dev, dev);
> -
> -    pci_config_set_vendor_id(s->dev.config, s->vid);
> -    pci_config_set_device_id(s->dev.config, s->did);
> -
> -    pci_set_word(dev->config + PCI_STATUS,
> -                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> -    pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_PCI);
> -    dev->config[PCI_HEADER_TYPE] =
> -        (dev->config[PCI_HEADER_TYPE] & PCI_HEADER_TYPE_MULTI_FUNCTION) |
> -        PCI_HEADER_TYPE_BRIDGE;
> -    pci_set_word(dev->config + PCI_SEC_STATUS,
> -                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> -    return 0;
> -}
> -
> -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);
> -    return 0;
> -}
> -
> -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> -                        uint16_t vid, uint16_t did,
> -                        pci_map_irq_fn map_irq, const char *name)
> -{
> -    PCIDevice *dev;
> -    PCIBridge *s;
> -
> -    dev = pci_create_multifunction(bus, devfn, multifunction, "pci-bridge");
> -    qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
> -    qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
> -    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;
> -}
> -
> -PCIDevice *pci_bridge_get_device(PCIBus *bus)
> -{
> -    return bus->parent_dev;
> -}
> -
>  static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
>  {
>      PCIDevice *pci_dev = (PCIDevice *)qdev;
> @@ -1926,23 +1775,3 @@ static char *pcibus_get_dev_path(DeviceState *dev)
>      return strdup(path);
>  }
>  
> -static PCIDeviceInfo bridge_info = {
> -    .qdev.name    = "pci-bridge",
> -    .qdev.size    = sizeof(PCIBridge),
> -    .init         = pci_bridge_initfn,
> -    .exit         = pci_bridge_exitfn,
> -    .config_write = pci_bridge_write_config,
> -    .is_bridge    = 1,
> -    .qdev.props   = (Property[]) {
> -        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
> -        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
> -        DEFINE_PROP_END_OF_LIST(),
> -    }
> -};
> -
> -static void pci_register_devices(void)
> -{
> -    pci_qdev_register(&bridge_info);
> -}
> -
> -device_init(pci_register_devices)
> diff --git a/hw/pci.h b/hw/pci.h
> index 1eab7e7..c551f96 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -233,10 +233,7 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
>  
>  void do_pci_info_print(Monitor *mon, const QObject *data);
>  void do_pci_info(Monitor *mon, QObject **ret_data);
> -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> -                        uint16_t vid, uint16_t did,
> -                        pci_map_irq_fn map_irq, const char *name);
> -PCIDevice *pci_bridge_get_device(PCIBus *bus);
> +void pci_bridge_update_mappings(PCIBus *b);
>  
>  static inline void
>  pci_set_byte(uint8_t *config, uint8_t val)
> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> new file mode 100644
> index 0000000..3f69a44
> --- /dev/null
> +++ b/hw/pci_bridge.c
> @@ -0,0 +1,206 @@
> +/*
> + * QEMU PCI bus manager
> + *
> + * Copyright (c) 2004 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to dea
> +
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM
> +
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +/*
> + * split out from pci.c
> + * Copyright (c) 2010 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + */
> +
> +#include "pci_bridge.h"
> +#include "pci_internals.h"
> +
> +PCIDevice *pci_bridge_get_device(PCIBus *bus)
> +{
> +    return bus->parent_dev;
> +}
> +
> +static void pci_register_secondary_bus(PCIBus *parent,
> +                                       PCIBus *bus,
> +                                       PCIDevice *dev,
> +                                       pci_map_irq_fn map_irq,
> +                                       const char *name)
> +{
> +    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
> +    bus->map_irq = map_irq;
> +    bus->parent_dev = dev;
> +
> +    QLIST_INIT(&bus->child);
> +    QLIST_INSERT_HEAD(&parent->child, bus, sibling);
> +}
> +
> +static void pci_unregister_secondary_bus(PCIBus *bus)
> +{
> +    assert(QLIST_EMPTY(&bus->child));
> +    QLIST_REMOVE(bus, sibling);
> +}
> +
> +static uint32_t pci_config_get_io_base(PCIDevice *d,
> +                                       uint32_t base, uint32_t base_upper16)
> +{
> +    uint32_t val;
> +
> +    val = ((uint32_t)d->config[base] & PCI_IO_RANGE_MASK) << 8;
> +    if (d->config[base] & PCI_IO_RANGE_TYPE_32) {
> +        val |= (uint32_t)pci_get_word(d->config + base_upper16) << 16;
> +    }
> +    return val;
> +}
> +
> +static pcibus_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
> +{
> +    return ((pcibus_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
> +        << 16;
> +}
> +
> +static pcibus_t pci_config_get_pref_base(PCIDevice *d,
> +                                         uint32_t base, uint32_t upper)
> +{
> +    pcibus_t tmp;
> +    pcibus_t val;
> +
> +    tmp = (pcibus_t)pci_get_word(d->config + base);
> +    val = (tmp & PCI_PREF_RANGE_MASK) << 16;
> +    if (tmp & PCI_PREF_RANGE_TYPE_64) {
> +        val |= (pcibus_t)pci_get_long(d->config + upper) << 32;
> +    }
> +    return val;
> +}
> +
> +pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type)
> +{
> +    pcibus_t base;
> +    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> +        base = pci_config_get_io_base(bridge,
> +                                      PCI_IO_BASE, PCI_IO_BASE_UPPER16);
> +    } else {
> +        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
> +            base = pci_config_get_pref_base(
> +                bridge, PCI_PREF_MEMORY_BASE, PCI_PREF_BASE_UPPER32);
> +        } else {
> +            base = pci_config_get_memory_base(bridge, PCI_MEMORY_BASE);
> +        }
> +    }
> +
> +    return base;
> +}
> +
> +pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
> +{
> +    pcibus_t limit;
> +    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> +        limit = pci_config_get_io_base(bridge,
> +                                      PCI_IO_LIMIT, PCI_IO_LIMIT_UPPER16);
> +        limit |= 0xfff;         /* PCI bridge spec 3.2.5.6. */
> +    } else {
> +        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
> +            limit = pci_config_get_pref_base(
> +                bridge, PCI_PREF_MEMORY_LIMIT, PCI_PREF_LIMIT_UPPER32);
> +        } else {
> +            limit = pci_config_get_memory_base(bridge, PCI_MEMORY_LIMIT);
> +        }
> +        limit |= 0xfffff;       /* PCI bridge spec 3.2.5.{1, 8}. */
> +    }
> +    return limit;
> +}
> +
> +static void pci_bridge_write_config(PCIDevice *d,
> +                             uint32_t address, uint32_t val, int len)
> +{
> +    pci_default_write_config(d, address, val, len);
> +
> +    if (/* io base/limit */
> +        ranges_overlap(address, len, PCI_IO_BASE, 2) ||
> +
> +        /* memory base/limit, prefetchable base/limit and
> +           io base/limit upper 16 */
> +        ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
> +        pci_bridge_update_mappings(d->bus);
> +    }
> +}
> +
> +static int pci_bridge_initfn(PCIDevice *dev)
> +{
> +    PCIBridge *s = DO_UPCAST(PCIBridge, dev, dev);
> +
> +    pci_config_set_vendor_id(s->dev.config, s->vid);
> +    pci_config_set_device_id(s->dev.config, s->did);
> +
> +    pci_set_word(dev->config + PCI_STATUS,
> +                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> +    pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_PCI);
> +    dev->config[PCI_HEADER_TYPE] =
> +        (dev->config[PCI_HEADER_TYPE] & PCI_HEADER_TYPE_MULTI_FUNCTION) |
> +        PCI_HEADER_TYPE_BRIDGE;
> +    pci_set_word(dev->config + PCI_SEC_STATUS,
> +                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> +    return 0;
> +}
> +
> +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);
> +    return 0;
> +}
> +
> +PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> +                        uint16_t vid, uint16_t did,
> +                        pci_map_irq_fn map_irq, const char *name)
> +{
> +    PCIDevice *dev;
> +    PCIBridge *s;
> +
> +    dev = pci_create_multifunction(bus, devfn, multifunction, "pci-bridge");
> +    qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
> +    qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
> +    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;
> +}
> +
> +static PCIDeviceInfo bridge_info = {
> +    .qdev.name    = "pci-bridge",
> +    .qdev.size    = sizeof(PCIBridge),
> +    .init         = pci_bridge_initfn,
> +    .exit         = pci_bridge_exitfn,
> +    .config_write = pci_bridge_write_config,
> +    .is_bridge    = 1,
> +    .qdev.props   = (Property[]) {
> +        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
> +        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +static void pci_register_devices(void)
> +{
> +    pci_qdev_register(&bridge_info);
> +}
> +
> +device_init(pci_register_devices)
> diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
> new file mode 100644
> index 0000000..ddb2c82
> --- /dev/null
> +++ b/hw/pci_bridge.h
> @@ -0,0 +1,48 @@
> +/*
> + * QEMU PCI bridge
> + *
> + * Copyright (c) 2004 Fabrice Bellard
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + * split out pci bus specific stuff from pci.[hc] to pci_bridge.[hc]
> + * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + *
> + */
> +
> +#ifndef QEMU_PCI_BRIDGE_H
> +#define QEMU_PCI_BRIDGE_H
> +
> +#include "pci.h"
> +
> +PCIDevice *pci_bridge_get_device(PCIBus *bus);
> +
> +pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type);
> +pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type);
> +
> +PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> +                        uint16_t vid, uint16_t did,
> +                        pci_map_irq_fn map_irq, const char *name);
> +
> +#endif  /* QEMU_PCI_BRIDGE_H */
> +/*
> + * Local variables:
> + *  c-indent-level: 4
> + *  c-basic-offset: 4
> + *  tab-width: 8
> + *  indent-tab-mode: nil
> + * End:
> + */
> -- 
> 1.7.1.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] Re: [PATCH v2 2/5] pci/bridge: split out pci bridge code into pci_bridge.c from pci.c
  2010-07-12 12:16   ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-07-12 13:22     ` Isaku Yamahata
  0 siblings, 0 replies; 12+ messages in thread
From: Isaku Yamahata @ 2010-07-12 13:22 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: blauwirbel, qemu-devel

On Mon, Jul 12, 2010 at 03:16:02PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 12, 2010 at 07:36:41PM +0900, Isaku Yamahata wrote:
> > Move pci bridge related code into pci_bridge.c from pci.c
> > for further enhancement. pci.c is big enough now, so split it out.
> > No code change but exporting some accesser functions.
> > 
> > In fact, few pci bridge functions stays in pci.c.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> Got conflicts starting with this patch.
> Could you please rebase to my pci tree and repost patches 2-4?

Okay, will do tomorrow.
-- 
yamahata

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] Re: [PATCH v2 5/5] pci_bridge: introduce pci bridge library.
  2010-07-12 12:10   ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-07-12 13:28     ` Isaku Yamahata
  2010-07-12 14:47       ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Isaku Yamahata @ 2010-07-12 13:28 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: blauwirbel, qemu-devel

On Mon, Jul 12, 2010 at 03:10:00PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 12, 2010 at 07:36:44PM +0900, Isaku Yamahata wrote:
> > diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
> > index ddb2c82..4697c7a 100644
> > --- a/hw/pci_bridge.h
> > +++ b/hw/pci_bridge.h
> > @@ -29,13 +29,27 @@
> >  #include "pci.h"
> >  
> >  PCIDevice *pci_bridge_get_device(PCIBus *bus);
> > +PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
> >  
> > -pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type);
> > -pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type);
> > +pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
> > +pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
> >  
> > -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> > -                        uint16_t vid, uint16_t did,
> > -                        pci_map_irq_fn map_irq, const char *name);
> > +void pci_bridge_write_config(PCIDevice *d,
> > +                             uint32_t address, uint32_t val, int len);
> > +void pci_bridge_reset_reg(PCIDevice *dev);
> > +void pci_bridge_reset(DeviceState *qdev);
> > +
> > +int pci_bridge_initfn(PCIDevice *pci_dev);
> > +int pci_bridge_exitfn(PCIDevice *pci_dev);
> > +
> > +void pci_bridge_qdev_register(PCIDeviceInfo *info);
> > +
> > +PCIBridge *pci_bridge_create(PCIBus *bus, int devfn, bool multifunction,
> > +                             pci_map_irq_fn map_irq,
> > +                             const char *name, const char *bus_name);
> > +PCIBridge *pci_bridge_create_simple(PCIBus *bus, int devfn, bool multifunction,
> > +                                    pci_map_irq_fn map_irq,
> > +                                    const char *name, const char *bus_name);
> 
> The APIs leave much to be desired.  _simple and regular are same?  What does _register do?
> 
> We really should just use qdev: Can't we use pci_qdev_register_many and
> pci_create to create the bridge?  Long term, all pci_create variants
> should go and get replaced with qdev_create.

If struct PCIBridge is exported, those three can be eliminated.
I think it is okay to export struct PCIBridge, but it would not
be a good idea to export PCIBus which is embedded in PCIBridge::sec_bus.

So how should we go?
- export both PCIBus and PCIBridge.

- make PCIBridge::sec_bus pointer, and export PCIBridge.
  And kill register, create, create_simple.
  v1 patch. Although you rejected it, I suppose you didn't see
  this issue.

- introduce wrapper functions to convert types.
  This patch.

- better alternatives?

-- 
yamahata

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] Re: [PATCH v2 5/5] pci_bridge: introduce pci bridge library.
  2010-07-12 13:28     ` Isaku Yamahata
@ 2010-07-12 14:47       ` Michael S. Tsirkin
  0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2010-07-12 14:47 UTC (permalink / raw)
  To: Isaku Yamahata; +Cc: blauwirbel, qemu-devel

On Mon, Jul 12, 2010 at 10:28:24PM +0900, Isaku Yamahata wrote:
> On Mon, Jul 12, 2010 at 03:10:00PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jul 12, 2010 at 07:36:44PM +0900, Isaku Yamahata wrote:
> > > diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
> > > index ddb2c82..4697c7a 100644
> > > --- a/hw/pci_bridge.h
> > > +++ b/hw/pci_bridge.h
> > > @@ -29,13 +29,27 @@
> > >  #include "pci.h"
> > >  
> > >  PCIDevice *pci_bridge_get_device(PCIBus *bus);
> > > +PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
> > >  
> > > -pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type);
> > > -pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type);
> > > +pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
> > > +pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
> > >  
> > > -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> > > -                        uint16_t vid, uint16_t did,
> > > -                        pci_map_irq_fn map_irq, const char *name);
> > > +void pci_bridge_write_config(PCIDevice *d,
> > > +                             uint32_t address, uint32_t val, int len);
> > > +void pci_bridge_reset_reg(PCIDevice *dev);
> > > +void pci_bridge_reset(DeviceState *qdev);
> > > +
> > > +int pci_bridge_initfn(PCIDevice *pci_dev);
> > > +int pci_bridge_exitfn(PCIDevice *pci_dev);
> > > +
> > > +void pci_bridge_qdev_register(PCIDeviceInfo *info);
> > > +
> > > +PCIBridge *pci_bridge_create(PCIBus *bus, int devfn, bool multifunction,
> > > +                             pci_map_irq_fn map_irq,
> > > +                             const char *name, const char *bus_name);
> > > +PCIBridge *pci_bridge_create_simple(PCIBus *bus, int devfn, bool multifunction,
> > > +                                    pci_map_irq_fn map_irq,
> > > +                                    const char *name, const char *bus_name);
> > 
> > The APIs leave much to be desired.  _simple and regular are same?  What does _register do?
> > 
> > We really should just use qdev: Can't we use pci_qdev_register_many and
> > pci_create to create the bridge?  Long term, all pci_create variants
> > should go and get replaced with qdev_create.
> 
> If struct PCIBridge is exported, those three can be eliminated.
> I think it is okay to export struct PCIBridge, but it would not
> be a good idea to export PCIBus which is embedded in PCIBridge::sec_bus.
> 
> So how should we go?
> - export both PCIBus and PCIBridge.
> 
> - make PCIBridge::sec_bus pointer, and export PCIBridge.
>   And kill register, create, create_simple.
>   v1 patch. Although you rejected it, I suppose you didn't see
>   this issue.
> 
> - introduce wrapper functions to convert types.
>   This patch.
> 
> - better alternatives?

Let's export and see where this leads us.

> -- 
> yamahata

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2010-07-12 14:52 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-07-12 10:36 [Qemu-devel] [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Isaku Yamahata
2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 1/5] pci: move out pci internal structures, PCIBus, PCIBridge, and pci_bus_info Isaku Yamahata
2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 2/5] pci/bridge: split out pci bridge code into pci_bridge.c from pci.c Isaku Yamahata
2010-07-12 12:16   ` [Qemu-devel] " Michael S. Tsirkin
2010-07-12 13:22     ` Isaku Yamahata
2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 3/5] pci_bridge: rename PCIBridge::bus -> PCIBridge::sec_bus Isaku Yamahata
2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 4/5] pci_bridge: clean up: remove pci_{register, unregister}_secondary_bus() Isaku Yamahata
2010-07-12 10:36 ` [Qemu-devel] [PATCH v2 5/5] pci_bridge: introduce pci bridge library Isaku Yamahata
2010-07-12 12:10   ` [Qemu-devel] " Michael S. Tsirkin
2010-07-12 13:28     ` Isaku Yamahata
2010-07-12 14:47       ` Michael S. Tsirkin
2010-07-12 12:11 ` [Qemu-devel] Re: [PATCH v2 0/5] pci: split out bridge code into pci_bridge and make it library Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).