[Qemu-devel] [PATCH 0/7] qdev reset refactoring and pci bus reset

[Qemu-devel] [PATCH 0/7] qdev reset refactoring and pci bus reset

[Isaku Yamahata]

The goal of this patch series is to implement secondary bus reset
emulation in pci-to-pci bridge.
At first, this patch series refactors qdev reset,
and then cleans up pci bus reset. Lastly implements pci bridge control
secondary bus reset bit.

This patch series is for pci bus reset, which is ported
from the following repo.
git://repo.or.cz/qemu/aliguori.git qdev-refactor

Anthony Liguori (2):
  qbus: add functions to walk both devices and busses
  qdev: reset qdev along with qdev tree

Isaku Yamahata (5):
  qdev: introduce reset call back for qbus level
  qdev: introduce a helper function which triggers reset from a given
    device
  pci: make use of qdev reset frame work to pci bus reset.
  pci: teach pci devices that have reset callback how to reset common
    registers
  pci bridge: implement secondary bus reset

 hw/e1000.c      |    1 +
 hw/lsi53c895a.c |    2 +
 hw/pci.c        |   32 +++++++++++++++++--
 hw/pci.h        |    3 ++
 hw/pci_bridge.c |   12 +++++++-
 hw/pcnet.c      |    1 +
 hw/qdev.c       |   87 +++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/qdev.h       |   16 ++++++++++
 hw/rtl8139.c    |    2 +
 hw/virtio-pci.c |    1 +
 vl.c            |    1 +
 11 files changed, 144 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH 1/7] qbus: add functions to walk both devices and busses

[Isaku Yamahata]

From: Anthony Liguori <anthony@codemonkey.ws>

There are some cases where you want to walk the busses, in particular, when
searching for a bus either by name or DeviceInfo.
Paolo suggested that we model the return values on how GCC's walkers work which
allows an actor to skip child transversal, or terminate walking with a positive
value that's returned as the qbus_walk_children's result.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |    9 +++++++++
 2 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 35858cb..11d845a 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -449,6 +449,52 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
     return NULL;
 }
 
+int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
+                       qbus_walkerfn *busfn, void *opaque)
+{
+    DeviceState *dev;
+    int err;
+
+    if (busfn) {
+        err = busfn(bus, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
+    QLIST_FOREACH(dev, &bus->children, sibling) {
+        err = qdev_walk_children(dev, devfn, busfn, opaque);
+        if (err < 0) {
+            return err;
+        }
+    }
+
+    return 0;
+}
+
+int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
+                       qbus_walkerfn *busfn, void *opaque)
+{
+    BusState *bus;
+    int err;
+
+    if (devfn) {
+        err = devfn(dev, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
+    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+        err = qbus_walk_children(bus, devfn, busfn, opaque);
+        if (err < 0) {
+            return err;
+        }
+    }
+
+    return 0;
+}
+
 static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                      const BusInfo *info)
 {
diff --git a/hw/qdev.h b/hw/qdev.h
index 579328a..0cf50b1 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -173,9 +173,18 @@ BusState *qdev_get_parent_bus(DeviceState *dev);
 
 /*** BUS API. ***/
 
+/* Returns 0 to walk children, > 0 to terminate walk, < 0 to skip walk. */
+typedef int (qbus_walkerfn)(BusState *bus, void *opaque);
+typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
+
 void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
+/* Returns > 0 if either devfn or busfn terminate walk, 0 otherwise. */
+int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
+                       qbus_walkerfn *busfn, void *opaque);
+int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
+                       qbus_walkerfn *busfn, void *opaque);
 void qbus_free(BusState *bus);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
-- 
1.7.1.1

[Qemu-devel] [PATCH 2/7] qdev: reset qdev along with qdev tree

[Isaku Yamahata]

From: Anthony Liguori <anthony@codemonkey.ws>

This patch changes the reset handling so that qdev has no knowledge of the
global system reset.  Instead, a new bus/device level function is introduced
that allows all devices/buses on the bus/device to be reset using a depth
first transversal.

N.B. we have to expose the implicit system bus because we have various hacks
that result in an implicit system bus existing.  Instead, we ought to have an
explicitly created system bus that we can trigger reset from.  That's a topic
for a future patch though.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/qdev.c |   28 +++++++++++++++++++---------
 hw/qdev.h |    4 ++++
 vl.c      |    1 +
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 11d845a..92ccc8d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -257,13 +257,6 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     return qdev;
 }
 
-static void qdev_reset(void *opaque)
-{
-    DeviceState *dev = opaque;
-    if (dev->info->reset)
-        dev->info->reset(dev);
-}
-
 /* Initialize a device.  Device properties should be set before calling
    this function.  IRQs and MMIO regions should be connected/mapped after
    calling this function.
@@ -279,7 +272,6 @@ int qdev_init(DeviceState *dev)
         qdev_free(dev);
         return rc;
     }
-    qemu_register_reset(qdev_reset, dev);
     if (dev->info->vmsd) {
         vmstate_register_with_alias_id(dev, -1, dev->info->vmsd, dev,
                                        dev->instance_id_alias,
@@ -308,6 +300,25 @@ int qdev_unplug(DeviceState *dev)
     return dev->info->unplug(dev);
 }
 
+static int qdev_reset_one(DeviceState *dev, void *opaque)
+{
+    if (dev->info->reset) {
+        dev->info->reset(dev);
+    }
+
+    return 0;
+}
+
+BusState *sysbus_get_default(void)
+{
+    return main_system_bus;
+}
+
+void qbus_reset_all(BusState *bus)
+{
+    qbus_walk_children(bus, qdev_reset_one, NULL, NULL);
+}
+
 /* can be used as ->unplug() callback for the simple cases */
 int qdev_simple_unplug_cb(DeviceState *dev)
 {
@@ -351,7 +362,6 @@ void qdev_free(DeviceState *dev)
         if (dev->opts)
             qemu_opts_del(dev->opts);
     }
-    qemu_unregister_reset(qdev_reset, dev);
     QLIST_REMOVE(dev, sibling);
     for (prop = dev->info->props; prop && prop->name; prop++) {
         if (prop->info->free) {
diff --git a/hw/qdev.h b/hw/qdev.h
index 0cf50b1..2eb1f34 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -185,10 +185,14 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
                        qbus_walkerfn *busfn, void *opaque);
 int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
                        qbus_walkerfn *busfn, void *opaque);
+void qbus_reset_all(BusState *bus);
 void qbus_free(BusState *bus);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
+/* This should go away once we get rid of the NULL bus hack */
+BusState *sysbus_get_default(void);
+
 /*** monitor commands ***/
 
 void do_info_qtree(Monitor *mon);
diff --git a/vl.c b/vl.c
index c58583d..135fdeb 100644
--- a/vl.c
+++ b/vl.c
@@ -2976,6 +2976,7 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
     qemu_system_reset();
     if (loadvm) {
         if (load_vmstate(loadvm) < 0) {
-- 
1.7.1.1

[Qemu-devel] [PATCH 3/7] qdev: introduce reset call back for qbus level

[Isaku Yamahata]

and make it called via qbus_reset_all().
The qbus reset callback will be used by pci bus reset.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   10 +++++++++-
 hw/qdev.h |    2 ++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 92ccc8d..b76da07 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -314,9 +314,17 @@ BusState *sysbus_get_default(void)
     return main_system_bus;
 }
 
+static int qbus_reset_one(BusState *bus, void *opaque)
+{
+    if (bus->info->reset) {
+        return bus->info->reset(bus);
+    }
+    return 0;
+}
+
 void qbus_reset_all(BusState *bus)
 {
-    qbus_walk_children(bus, qdev_reset_one, NULL, NULL);
+    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
 }
 
 /* can be used as ->unplug() callback for the simple cases */
diff --git a/hw/qdev.h b/hw/qdev.h
index 2eb1f34..ed10241 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -49,12 +49,14 @@ struct DeviceState {
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
 typedef char *(*bus_get_dev_path)(DeviceState *dev);
+typedef int (qbus_resetfn)(BusState *bus);
 
 struct BusInfo {
     const char *name;
     size_t size;
     bus_dev_printfn print_dev;
     bus_get_dev_path get_dev_path;
+    qbus_resetfn *reset;
     Property *props;
 };
 
-- 
1.7.1.1

[Qemu-devel] [PATCH 4/7] qdev: introduce a helper function which triggers reset from a given device

[Isaku Yamahata]

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |    5 +++++
 hw/qdev.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index b76da07..b65b63e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -322,6 +322,11 @@ static int qbus_reset_one(BusState *bus, void *opaque)
     return 0;
 }
 
+void qdev_reset_all(DeviceState *dev)
+{
+    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
+}
+
 void qbus_reset_all(BusState *bus)
 {
     qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
diff --git a/hw/qdev.h b/hw/qdev.h
index ed10241..02df3be 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -187,6 +187,7 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
                        qbus_walkerfn *busfn, void *opaque);
 int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
                        qbus_walkerfn *busfn, void *opaque);
+void qdev_reset_all(DeviceState *dev);
 void qbus_reset_all(BusState *bus);
 void qbus_free(BusState *bus);
 
-- 
1.7.1.1

[Qemu-devel] [PATCH 5/7] pci: make use of qdev reset frame work to pci bus reset.

[Isaku Yamahata]

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/pci.c |   38 ++++++++++++++++++++++++++++++++++----
 hw/pci.h |    3 +++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 962886e..b6f58de 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -43,12 +43,14 @@
 
 static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *pcibus_get_dev_path(DeviceState *dev);
+static int pcibus_reset(BusState *qbus);
 
 struct BusInfo pci_bus_info = {
     .name       = "PCI",
     .size       = sizeof(PCIBus),
     .print_dev  = pcibus_dev_print,
     .get_dev_path = pcibus_get_dev_path,
+    .reset      = pcibus_reset,
     .props      = (Property[]) {
         DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
         DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
@@ -133,7 +135,7 @@ static void pci_update_irq_status(PCIDevice *dev)
     }
 }
 
-static void pci_device_reset(PCIDevice *dev)
+void pci_device_reset_default(PCIDevice *dev)
 {
     int r;
 
@@ -161,9 +163,29 @@ static void pci_device_reset(PCIDevice *dev)
     pci_update_mappings(dev);
 }
 
-static void pci_bus_reset(void *opaque)
+static void pci_device_reset(PCIDevice *dev)
+{
+    if (!dev->qdev.info) {
+        /* not all pci devices haven't been qdev'fied yet
+           TODO: remove this when all pci devices are qdev'fied. */
+        pci_device_reset_default(dev);
+    } else {
+        /*
+         * TODO:
+         * each device should know what to do on RST#.
+         * move pci_device_reset_default() into each callback.
+         */
+        qdev_reset_all(&dev->qdev);
+        pci_device_reset_default(dev);
+    }
+}
+
+/*
+ * Trigger pci bus reset under a given bus.
+ * This functions emulates RST#.
+ */
+void pci_bus_reset(PCIBus *bus)
 {
-    PCIBus *bus = opaque;
     int i;
 
     for (i = 0; i < bus->nirq; i++) {
@@ -176,6 +198,15 @@ static void pci_bus_reset(void *opaque)
     }
 }
 
+static int pcibus_reset(BusState *qbus)
+{
+    pci_bus_reset(DO_UPCAST(PCIBus, qbus, qbus));
+
+    /* topology traverse is done by pci_bus_reset().
+       Tell qbus/qdev walker not to traverse the tree */
+    return 1;
+}
+
 static void pci_host_bus_register(int domain, PCIBus *bus)
 {
     struct PCIHostBus *host;
@@ -230,7 +261,6 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
     pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported */
 
     vmstate_register(NULL, -1, &vmstate_pcibus, bus);
-    qemu_register_reset(pci_bus_reset, bus);
 }
 
 PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min)
diff --git a/hw/pci.h b/hw/pci.h
index 7100804..280a2f8 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -225,6 +225,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                          void *irq_opaque, int devfn_min, int nirq);
 
+void pci_bus_reset(PCIBus *bus);
+void pci_device_reset_default(PCIDevice *dev);
+
 void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base);
 
 PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
-- 
1.7.1.1

[Qemu-devel] [PATCH 6/7] pci: teach pci devices that have reset callback how to reset common registers

[Isaku Yamahata]

Each pci devices should know the behavior on reset.
So make each reset functions call pci default reset function.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/e1000.c      |    1 +
 hw/lsi53c895a.c |    2 ++
 hw/pci.c        |    8 +-------
 hw/pcnet.c      |    1 +
 hw/rtl8139.c    |    2 ++
 hw/virtio-pci.c |    1 +
 6 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 532efdc..747b740 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1091,6 +1091,7 @@ static void e1000_reset(void *opaque)
     memmove(d->mac_reg, mac_reg_init, sizeof mac_reg_init);
     d->rxbuf_min_shift = 1;
     memset(&d->tx, 0, sizeof d->tx);
+    pci_device_reset_default(&d->dev);
 }
 
 static NetClientInfo net_e1000_info = {
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index f97335e..d490c63 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -358,6 +358,8 @@ static void lsi_soft_reset(LSIState *s)
         qemu_free(s->current);
         s->current = NULL;
     }
+
+    pci_device_reset_default(&s->dev);
 }
 
 static int lsi_dma_40bit(LSIState *s)
diff --git a/hw/pci.c b/hw/pci.c
index b6f58de..51c8307 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -165,18 +165,12 @@ void pci_device_reset_default(PCIDevice *dev)
 
 static void pci_device_reset(PCIDevice *dev)
 {
-    if (!dev->qdev.info) {
+    if (!dev->qdev.info || !dev->qdev.info->reset) {
         /* not all pci devices haven't been qdev'fied yet
            TODO: remove this when all pci devices are qdev'fied. */
         pci_device_reset_default(dev);
     } else {
-        /*
-         * TODO:
-         * each device should know what to do on RST#.
-         * move pci_device_reset_default() into each callback.
-         */
         qdev_reset_all(&dev->qdev);
-        pci_device_reset_default(dev);
     }
 }
 
diff --git a/hw/pcnet.c b/hw/pcnet.c
index b52935a..e73e682 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -2023,6 +2023,7 @@ static void pci_reset(DeviceState *dev)
     PCIPCNetState *d = DO_UPCAST(PCIPCNetState, pci_dev.qdev, dev);
 
     pcnet_h_reset(&d->state);
+    pci_device_reset_default(&d->pci_dev);
 }
 
 static PCIDeviceInfo pcnet_info = {
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index d92981d..1f35e5d 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -1260,6 +1260,8 @@ static void rtl8139_reset(DeviceState *d)
 
     /* reset tally counters */
     RTL8139TallyCounters_clear(&s->tally_counters);
+
+    pci_device_reset_default(&s->dev);
 }
 
 static void RTL8139TallyCounters_clear(RTL8139TallyCounters* counters)
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 729917d..f4d39f6 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -186,6 +186,7 @@ static void virtio_pci_reset(DeviceState *d)
     virtio_reset(proxy->vdev);
     msix_reset(&proxy->pci_dev);
     proxy->bugs = 0;
+    pci_device_reset_default(&proxy->pci_dev);
 }
 
 static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
-- 
1.7.1.1

[Qemu-devel] [PATCH 7/7] pci bridge: implement secondary bus reset

[Isaku Yamahata]

Emulates secondary bus reset when secondary bus reset bit
is written from 0 to 1.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/pci_bridge.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index 58cc2e4..618a81e 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -139,6 +139,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
 void pci_bridge_write_config(PCIDevice *d,
                              uint32_t address, uint32_t val, int len)
 {
+    PCIBridge *s = container_of(d, PCIBridge, dev);
+    uint16_t bridge_control = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
+    uint16_t bridge_control_new;
+
     pci_default_write_config(d, address, val, len);
 
     if (/* io base/limit */
@@ -147,9 +151,15 @@ 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)) {
-        PCIBridge *s = container_of(d, PCIBridge, dev);
         pci_bridge_update_mappings(&s->sec_bus);
     }
+
+    bridge_control_new = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
+    if (!(bridge_control & PCI_BRIDGE_CTL_BUS_RESET) &&
+        (bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET)) {
+        /* 0 -> 1 */
+        pci_bus_reset(&s->sec_bus);
+    }
 }
 
 void pci_bridge_disable_base_limit(PCIDevice *dev)
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH 1/7] qbus: add functions to walk both devices and busses

[Paolo Bonzini]

On 11/17/2010 05:50 AM, Isaku Yamahata wrote:
> +/* Returns 0 to walk children,>  0 to terminate walk,<  0 to skip walk. */

Shouldn't this be the other way round according to the code (< 0 to 
terminate, > 0 to skip children)?

> +/* Returns > 0 if either devfn or busfn terminate walk, 0 otherwise. */

This should be "returns != 0".  It can return a value > 0 if the 
toplevel callback asks to skip the children, < 0 if any callback 
(including the toplevel) asks to terminate the walk, 0 otherwise.

Paolo

[Qemu-devel] Re: [PATCH 5/7] pci: make use of qdev reset frame work to pci bus reset.

[Michael S. Tsirkin]

On Wed, Nov 17, 2010 at 01:50:25PM +0900, Isaku Yamahata wrote:
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/pci.c |   38 ++++++++++++++++++++++++++++++++++----
>  hw/pci.h |    3 +++
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 962886e..b6f58de 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -43,12 +43,14 @@
>  
>  static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>  static char *pcibus_get_dev_path(DeviceState *dev);
> +static int pcibus_reset(BusState *qbus);
>  
>  struct BusInfo pci_bus_info = {
>      .name       = "PCI",
>      .size       = sizeof(PCIBus),
>      .print_dev  = pcibus_dev_print,
>      .get_dev_path = pcibus_get_dev_path,
> +    .reset      = pcibus_reset,
>      .props      = (Property[]) {
>          DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>          DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> @@ -133,7 +135,7 @@ static void pci_update_irq_status(PCIDevice *dev)
>      }
>  }
>  
> -static void pci_device_reset(PCIDevice *dev)
> +void pci_device_reset_default(PCIDevice *dev)
>  {
>      int r;
>  
> @@ -161,9 +163,29 @@ static void pci_device_reset(PCIDevice *dev)
>      pci_update_mappings(dev);
>  }
>  
> -static void pci_bus_reset(void *opaque)
> +static void pci_device_reset(PCIDevice *dev)
> +{
> +    if (!dev->qdev.info) {
> +        /* not all pci devices haven't been qdev'fied yet

Double negation :)

> +           TODO: remove this when all pci devices are qdev'fied. */
> +        pci_device_reset_default(dev);
> +    } else {
> +        /*
> +         * TODO:
> +         * each device should know what to do on RST#.
> +         * move pci_device_reset_default() into each callback.
> +         */

Is this doing anything besides give devices another way to shoot
themselves in the foot?  Handling this all in one place seems easier,
assuming everyone just calls pci_device_reset_default in the end.  Or do
you expect some devices to avoid calling pci_device_reset_default?

> +        qdev_reset_all(&dev->qdev);
> +        pci_device_reset_default(dev);
> +    }
> +}
> +
> +/*
> + * Trigger pci bus reset under a given bus.
> + * This functions emulates RST#.
> + */
> +void pci_bus_reset(PCIBus *bus)
>  {
> -    PCIBus *bus = opaque;
>      int i;
>  
>      for (i = 0; i < bus->nirq; i++) {
> @@ -176,6 +198,15 @@ static void pci_bus_reset(void *opaque)
>      }
>  }
>  
> +static int pcibus_reset(BusState *qbus)
> +{
> +    pci_bus_reset(DO_UPCAST(PCIBus, qbus, qbus));
> +
> +    /* topology traverse is done by pci_bus_reset().
> +       Tell qbus/qdev walker not to traverse the tree */
> +    return 1;
> +}
> +
>  static void pci_host_bus_register(int domain, PCIBus *bus)
>  {
>      struct PCIHostBus *host;
> @@ -230,7 +261,6 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
>      pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported */
>  
>      vmstate_register(NULL, -1, &vmstate_pcibus, bus);
> -    qemu_register_reset(pci_bus_reset, bus);
>  }
>  
>  PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min)
> diff --git a/hw/pci.h b/hw/pci.h
> index 7100804..280a2f8 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -225,6 +225,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>                           void *irq_opaque, int devfn_min, int nirq);
>  
> +void pci_bus_reset(PCIBus *bus);
> +void pci_device_reset_default(PCIDevice *dev);
> +
>  void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base);
>  
>  PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH 7/7] pci bridge: implement secondary bus reset

[Michael S. Tsirkin]

On Wed, Nov 17, 2010 at 01:50:27PM +0900, Isaku Yamahata wrote:
> Emulates secondary bus reset when secondary bus reset bit
> is written from 0 to 1.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/pci_bridge.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> index 58cc2e4..618a81e 100644
> --- a/hw/pci_bridge.c
> +++ b/hw/pci_bridge.c
> @@ -139,6 +139,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
>  void pci_bridge_write_config(PCIDevice *d,
>                               uint32_t address, uint32_t val, int len)
>  {
> +    PCIBridge *s = container_of(d, PCIBridge, dev);
> +    uint16_t bridge_control = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> +    uint16_t bridge_control_new;
> +
>      pci_default_write_config(d, address, val, len);
>  
>      if (/* io base/limit */
> @@ -147,9 +151,15 @@ 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)) {
> -        PCIBridge *s = container_of(d, PCIBridge, dev);
>          pci_bridge_update_mappings(&s->sec_bus);
>      }
> +
> +    bridge_control_new = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> +    if (!(bridge_control & PCI_BRIDGE_CTL_BUS_RESET) &&
> +        (bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET)) {
> +        /* 0 -> 1 */
> +        pci_bus_reset(&s->sec_bus);
> +    }
>  }
>  
>  void pci_bridge_disable_base_limit(PCIDevice *dev)

Presumably this bit will have to be made writeable?

> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH 7/7] pci bridge: implement secondary bus reset

[Isaku Yamahata]

On Thu, Nov 18, 2010 at 09:05:30AM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 17, 2010 at 01:50:27PM +0900, Isaku Yamahata wrote:
> > Emulates secondary bus reset when secondary bus reset bit
> > is written from 0 to 1.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > ---
> >  hw/pci_bridge.c |   12 +++++++++++-
> >  1 files changed, 11 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> > index 58cc2e4..618a81e 100644
> > --- a/hw/pci_bridge.c
> > +++ b/hw/pci_bridge.c
> > @@ -139,6 +139,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
> >  void pci_bridge_write_config(PCIDevice *d,
> >                               uint32_t address, uint32_t val, int len)
> >  {
> > +    PCIBridge *s = container_of(d, PCIBridge, dev);
> > +    uint16_t bridge_control = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > +    uint16_t bridge_control_new;
> > +
> >      pci_default_write_config(d, address, val, len);
> >  
> >      if (/* io base/limit */
> > @@ -147,9 +151,15 @@ 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)) {
> > -        PCIBridge *s = container_of(d, PCIBridge, dev);
> >          pci_bridge_update_mappings(&s->sec_bus);
> >      }
> > +
> > +    bridge_control_new = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > +    if (!(bridge_control & PCI_BRIDGE_CTL_BUS_RESET) &&
> > +        (bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET)) {
> > +        /* 0 -> 1 */
> > +        pci_bus_reset(&s->sec_bus);
> > +    }
> >  }
> >  
> >  void pci_bridge_disable_base_limit(PCIDevice *dev)
> 
> Presumably this bit will have to be made writeable?

Yes, it's already writable.
static void pci_init_wmask_bridge(PCIDevice *d)
...
   pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);

-- 
yamahata

[Qemu-devel] Re: [PATCH 5/7] pci: make use of qdev reset frame work to pci bus reset.

[Isaku Yamahata]

On Thu, Nov 18, 2010 at 09:02:35AM +0200, Michael S. Tsirkin wrote:
> > +        /*
> > +         * TODO:
> > +         * each device should know what to do on RST#.
> > +         * move pci_device_reset_default() into each callback.
> > +         */
> 
> Is this doing anything besides give devices another way to shoot
> themselves in the foot?  Handling this all in one place seems easier,
> assuming everyone just calls pci_device_reset_default in the end.  Or do
> you expect some devices to avoid calling pci_device_reset_default?

I think only single function per a device should know all about reset
behavior and if a device overrides reset behavior, it should take care
of itself fully.
But it seems you don't think so. I can drop the following patch(6/7)
and eliminate this TODO comment.
-- 
yamahata

[Qemu-devel] Re: [PATCH 7/7] pci bridge: implement secondary bus reset

[Michael S. Tsirkin]

On Thu, Nov 18, 2010 at 04:29:10PM +0900, Isaku Yamahata wrote:
> On Thu, Nov 18, 2010 at 09:05:30AM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 17, 2010 at 01:50:27PM +0900, Isaku Yamahata wrote:
> > > Emulates secondary bus reset when secondary bus reset bit
> > > is written from 0 to 1.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > ---
> > >  hw/pci_bridge.c |   12 +++++++++++-
> > >  1 files changed, 11 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> > > index 58cc2e4..618a81e 100644
> > > --- a/hw/pci_bridge.c
> > > +++ b/hw/pci_bridge.c
> > > @@ -139,6 +139,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
> > >  void pci_bridge_write_config(PCIDevice *d,
> > >                               uint32_t address, uint32_t val, int len)
> > >  {
> > > +    PCIBridge *s = container_of(d, PCIBridge, dev);
> > > +    uint16_t bridge_control = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > > +    uint16_t bridge_control_new;
> > > +
> > >      pci_default_write_config(d, address, val, len);
> > >  
> > >      if (/* io base/limit */
> > > @@ -147,9 +151,15 @@ 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)) {
> > > -        PCIBridge *s = container_of(d, PCIBridge, dev);
> > >          pci_bridge_update_mappings(&s->sec_bus);
> > >      }
> > > +
> > > +    bridge_control_new = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > > +    if (!(bridge_control & PCI_BRIDGE_CTL_BUS_RESET) &&
> > > +        (bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET)) {
> > > +        /* 0 -> 1 */
> > > +        pci_bus_reset(&s->sec_bus);
> > > +    }
> > >  }
> > >  
> > >  void pci_bridge_disable_base_limit(PCIDevice *dev)
> > 
> > Presumably this bit will have to be made writeable?
> 
> Yes, it's already writable.
> static void pci_init_wmask_bridge(PCIDevice *d)
> ...
>    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);

Ouch, that's wrong, isn't it?
Bits 15:12 are reserved, readonly, 0.

I think we need the following (untested).
Comments?

pci: fix bridge control bit wmask

Bits 12 to 15 in bridge control register are reserver and must be
read-only zero, curent mask is 0xffff which makes them writeable. Fix
this up by using symbolic bit names for writeable bits instead of a
hardcoded constant.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

diff --git a/hw/pci.c b/hw/pci.c
index 00ec8ea..7d6d5ad 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -588,7 +588,17 @@ static void pci_init_wmask_bridge(PCIDevice *d)
     /* PCI_PREF_BASE_UPPER32 and PCI_PREF_LIMIT_UPPER32 */
     memset(d->wmask + PCI_PREF_BASE_UPPER32, 0xff, 8);
 
-    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
+/* TODO: add this define to pci_regs.h in linux and then in qemu. */
+#define  PCI_BRIDGE_CTL_VGA_16BIT 0x10	/* VGA 16-bit decode */
+    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL,
+                 PCI_BRIDGE_CTL_PARITY |
+                 PCI_BRIDGE_CTL_SERR |
+                 PCI_BRIDGE_CTL_ISA |
+                 PCI_BRIDGE_CTL_VGA |
+                 PCI_BRIDGE_CTL_VGA_16BIT |
+                 PCI_BRIDGE_CTL_MASTER_ABORT |
+                 PCI_BRIDGE_CTL_BUS_RESET |
+                 PCI_BRIDGE_CTL_FAST_BACK);
 }
 
 static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)

[Qemu-devel] Re: [PATCH 5/7] pci: make use of qdev reset frame work to pci bus reset.

[Michael S. Tsirkin]

On Thu, Nov 18, 2010 at 05:22:50PM +0900, Isaku Yamahata wrote:
> On Thu, Nov 18, 2010 at 09:02:35AM +0200, Michael S. Tsirkin wrote:
> > > +        /*
> > > +         * TODO:
> > > +         * each device should know what to do on RST#.
> > > +         * move pci_device_reset_default() into each callback.
> > > +         */
> > 
> > Is this doing anything besides give devices another way to shoot
> > themselves in the foot?  Handling this all in one place seems easier,
> > assuming everyone just calls pci_device_reset_default in the end.  Or do
> > you expect some devices to avoid calling pci_device_reset_default?
> 
> I think only single function per a device should know all about reset
> behavior and if a device overrides reset behavior, it should take care
> of itself fully.

Yes. However devices don't seem to override pci reset behavior
- instead they want a callback to reset the devicestate fields.

> But it seems you don't think so.  I can drop the following patch(6/7)
> and eliminate this TODO comment.

Yes, if everyone just calls default reset, let's invoke it from common
core. If we see some devices not call common reset, that's when we
better move it.

-- 
MST

[Qemu-devel] Re: [PATCH 7/7] pci bridge: implement secondary bus reset

[Isaku Yamahata]

On Thu, Nov 18, 2010 at 10:46:25AM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 04:29:10PM +0900, Isaku Yamahata wrote:
> > On Thu, Nov 18, 2010 at 09:05:30AM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Nov 17, 2010 at 01:50:27PM +0900, Isaku Yamahata wrote:
> > > > Emulates secondary bus reset when secondary bus reset bit
> > > > is written from 0 to 1.
> > > > 
> > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > > ---
> > > >  hw/pci_bridge.c |   12 +++++++++++-
> > > >  1 files changed, 11 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> > > > index 58cc2e4..618a81e 100644
> > > > --- a/hw/pci_bridge.c
> > > > +++ b/hw/pci_bridge.c
> > > > @@ -139,6 +139,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
> > > >  void pci_bridge_write_config(PCIDevice *d,
> > > >                               uint32_t address, uint32_t val, int len)
> > > >  {
> > > > +    PCIBridge *s = container_of(d, PCIBridge, dev);
> > > > +    uint16_t bridge_control = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > > > +    uint16_t bridge_control_new;
> > > > +
> > > >      pci_default_write_config(d, address, val, len);
> > > >  
> > > >      if (/* io base/limit */
> > > > @@ -147,9 +151,15 @@ 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)) {
> > > > -        PCIBridge *s = container_of(d, PCIBridge, dev);
> > > >          pci_bridge_update_mappings(&s->sec_bus);
> > > >      }
> > > > +
> > > > +    bridge_control_new = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > > > +    if (!(bridge_control & PCI_BRIDGE_CTL_BUS_RESET) &&
> > > > +        (bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET)) {
> > > > +        /* 0 -> 1 */
> > > > +        pci_bus_reset(&s->sec_bus);
> > > > +    }
> > > >  }
> > > >  
> > > >  void pci_bridge_disable_base_limit(PCIDevice *dev)
> > > 
> > > Presumably this bit will have to be made writeable?
> > 
> > Yes, it's already writable.
> > static void pci_init_wmask_bridge(PCIDevice *d)
> > ...
> >    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> 
> Ouch, that's wrong, isn't it?
> Bits 15:12 are reserved, readonly, 0.
> 
> I think we need the following (untested).
> Comments?

Basically it looks good if you left bits 8-11 RO intentional.
qemu doesn't emulate pci bus cycles, so it won't matter.

Also, please include following hunk.

commit 8c76e0427234290a640499c1305cabd998d6f777
Author: Isaku Yamahata <yamahata@valinux.co.jp>
Date:   Fri Nov 19 17:08:57 2010 +0900

    pcie/port: initialize bridge control register properly
    
    pci generic layer initialized bridge control register
    according to pci spec. pcie deviates slightly from it,
    so initializes it properly.
    
    Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff --git a/hw/pcie_port.c b/hw/pcie_port.c
index 117de61..340dcdb 100644
--- a/hw/pcie_port.c
+++ b/hw/pcie_port.c
@@ -27,6 +27,14 @@ void pcie_port_init_reg(PCIDevice *d)
     pci_set_word(d->config + PCI_STATUS, 0);
     pci_set_word(d->config + PCI_SEC_STATUS, 0);
 
+    /* Unlike conventional pci bridge, some bits are hardwared to 0. */
+    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL,
+                 PCI_BRIDGE_CTL_PARITY |
+                 PCI_BRIDGE_CTL_ISA |
+                 PCI_BRIDGE_CTL_VGA |
+                 PCI_BRIDGE_CTL_SERR |
+                 PCI_BRIDGE_CTL_BUS_RESET);
+
     /* 7.5.3.5 Prefetchable Memory Base Limit
      * The Prefetchable Memory Base and Prefetchable Memory Limit registers
      * must indicate that 64-bit addresses are supported, as defined in


> 
> pci: fix bridge control bit wmask
> 
> Bits 12 to 15 in bridge control register are reserver and must be
> read-only zero, curent mask is 0xffff which makes them writeable. Fix
> this up by using symbolic bit names for writeable bits instead of a
> hardcoded constant.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> --
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 00ec8ea..7d6d5ad 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -588,7 +588,17 @@ static void pci_init_wmask_bridge(PCIDevice *d)
>      /* PCI_PREF_BASE_UPPER32 and PCI_PREF_LIMIT_UPPER32 */
>      memset(d->wmask + PCI_PREF_BASE_UPPER32, 0xff, 8);
>  
> -    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> +/* TODO: add this define to pci_regs.h in linux and then in qemu. */
> +#define  PCI_BRIDGE_CTL_VGA_16BIT 0x10	/* VGA 16-bit decode */
> +    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL,
> +                 PCI_BRIDGE_CTL_PARITY |
> +                 PCI_BRIDGE_CTL_SERR |
> +                 PCI_BRIDGE_CTL_ISA |
> +                 PCI_BRIDGE_CTL_VGA |
> +                 PCI_BRIDGE_CTL_VGA_16BIT |
> +                 PCI_BRIDGE_CTL_MASTER_ABORT |
> +                 PCI_BRIDGE_CTL_BUS_RESET |
> +                 PCI_BRIDGE_CTL_FAST_BACK);
>  }
>  
>  static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 7/7] pci bridge: implement secondary bus reset

[Michael S. Tsirkin]

On Fri, Nov 19, 2010 at 05:15:19PM +0900, Isaku Yamahata wrote:
> On Thu, Nov 18, 2010 at 10:46:25AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 18, 2010 at 04:29:10PM +0900, Isaku Yamahata wrote:
> > > On Thu, Nov 18, 2010 at 09:05:30AM +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Nov 17, 2010 at 01:50:27PM +0900, Isaku Yamahata wrote:
> > > > > Emulates secondary bus reset when secondary bus reset bit
> > > > > is written from 0 to 1.
> > > > > 
> > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > > > ---
> > > > >  hw/pci_bridge.c |   12 +++++++++++-
> > > > >  1 files changed, 11 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> > > > > index 58cc2e4..618a81e 100644
> > > > > --- a/hw/pci_bridge.c
> > > > > +++ b/hw/pci_bridge.c
> > > > > @@ -139,6 +139,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
> > > > >  void pci_bridge_write_config(PCIDevice *d,
> > > > >                               uint32_t address, uint32_t val, int len)
> > > > >  {
> > > > > +    PCIBridge *s = container_of(d, PCIBridge, dev);
> > > > > +    uint16_t bridge_control = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > > > > +    uint16_t bridge_control_new;
> > > > > +
> > > > >      pci_default_write_config(d, address, val, len);
> > > > >  
> > > > >      if (/* io base/limit */
> > > > > @@ -147,9 +151,15 @@ 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)) {
> > > > > -        PCIBridge *s = container_of(d, PCIBridge, dev);
> > > > >          pci_bridge_update_mappings(&s->sec_bus);
> > > > >      }
> > > > > +
> > > > > +    bridge_control_new = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > > > > +    if (!(bridge_control & PCI_BRIDGE_CTL_BUS_RESET) &&
> > > > > +        (bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET)) {
> > > > > +        /* 0 -> 1 */
> > > > > +        pci_bus_reset(&s->sec_bus);
> > > > > +    }
> > > > >  }
> > > > >  
> > > > >  void pci_bridge_disable_base_limit(PCIDevice *dev)
> > > > 
> > > > Presumably this bit will have to be made writeable?
> > > 
> > > Yes, it's already writable.
> > > static void pci_init_wmask_bridge(PCIDevice *d)
> > > ...
> > >    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > 
> > Ouch, that's wrong, isn't it?
> > Bits 15:12 are reserved, readonly, 0.
> > 
> > I think we need the following (untested).
> > Comments?
> 
> Basically it looks good if you left bits 8-11 RO intentional.
> qemu doesn't emulate pci bus cycles, so it won't matter.

Hmm, no, not intentional. I'll fix it up.

> Also, please include following hunk.
> 
> commit 8c76e0427234290a640499c1305cabd998d6f777
> Author: Isaku Yamahata <yamahata@valinux.co.jp>
> Date:   Fri Nov 19 17:08:57 2010 +0900
> 
>     pcie/port: initialize bridge control register properly
>     
>     pci generic layer initialized bridge control register
>     according to pci spec. pcie deviates slightly from it,
>     so initializes it properly.
>     
>     Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Applied, thanks.

> diff --git a/hw/pcie_port.c b/hw/pcie_port.c
> index 117de61..340dcdb 100644
> --- a/hw/pcie_port.c
> +++ b/hw/pcie_port.c
> @@ -27,6 +27,14 @@ void pcie_port_init_reg(PCIDevice *d)
>      pci_set_word(d->config + PCI_STATUS, 0);
>      pci_set_word(d->config + PCI_SEC_STATUS, 0);
>  
> +    /* Unlike conventional pci bridge, some bits are hardwared to 0. */
> +    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL,
> +                 PCI_BRIDGE_CTL_PARITY |
> +                 PCI_BRIDGE_CTL_ISA |
> +                 PCI_BRIDGE_CTL_VGA |
> +                 PCI_BRIDGE_CTL_SERR |
> +                 PCI_BRIDGE_CTL_BUS_RESET);
> +
>      /* 7.5.3.5 Prefetchable Memory Base Limit
>       * The Prefetchable Memory Base and Prefetchable Memory Limit registers
>       * must indicate that 64-bit addresses are supported, as defined in
> 
> 
> > 
> > pci: fix bridge control bit wmask
> > 
> > Bits 12 to 15 in bridge control register are reserver and must be
> > read-only zero, curent mask is 0xffff which makes them writeable. Fix
> > this up by using symbolic bit names for writeable bits instead of a
> > hardcoded constant.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > --
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 00ec8ea..7d6d5ad 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -588,7 +588,17 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> >      /* PCI_PREF_BASE_UPPER32 and PCI_PREF_LIMIT_UPPER32 */
> >      memset(d->wmask + PCI_PREF_BASE_UPPER32, 0xff, 8);
> >  
> > -    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > +/* TODO: add this define to pci_regs.h in linux and then in qemu. */
> > +#define  PCI_BRIDGE_CTL_VGA_16BIT 0x10	/* VGA 16-bit decode */
> > +    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL,
> > +                 PCI_BRIDGE_CTL_PARITY |
> > +                 PCI_BRIDGE_CTL_SERR |
> > +                 PCI_BRIDGE_CTL_ISA |
> > +                 PCI_BRIDGE_CTL_VGA |
> > +                 PCI_BRIDGE_CTL_VGA_16BIT |
> > +                 PCI_BRIDGE_CTL_MASTER_ABORT |
> > +                 PCI_BRIDGE_CTL_BUS_RESET |
> > +                 PCI_BRIDGE_CTL_FAST_BACK);
> >  }
> >  
> >  static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> > 
> 
> -- 
> yamahata

[Qemu-devel] Re: [PATCH 7/7] pci bridge: implement secondary bus reset

[Michael S. Tsirkin]

On Fri, Nov 19, 2010 at 05:15:19PM +0900, Isaku Yamahata wrote:
> On Thu, Nov 18, 2010 at 10:46:25AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 18, 2010 at 04:29:10PM +0900, Isaku Yamahata wrote:
> > > On Thu, Nov 18, 2010 at 09:05:30AM +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Nov 17, 2010 at 01:50:27PM +0900, Isaku Yamahata wrote:
> > > > > Emulates secondary bus reset when secondary bus reset bit
> > > > > is written from 0 to 1.
> > > > > 
> > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > > > ---
> > > > >  hw/pci_bridge.c |   12 +++++++++++-
> > > > >  1 files changed, 11 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> > > > > index 58cc2e4..618a81e 100644
> > > > > --- a/hw/pci_bridge.c
> > > > > +++ b/hw/pci_bridge.c
> > > > > @@ -139,6 +139,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
> > > > >  void pci_bridge_write_config(PCIDevice *d,
> > > > >                               uint32_t address, uint32_t val, int len)
> > > > >  {
> > > > > +    PCIBridge *s = container_of(d, PCIBridge, dev);
> > > > > +    uint16_t bridge_control = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > > > > +    uint16_t bridge_control_new;
> > > > > +
> > > > >      pci_default_write_config(d, address, val, len);
> > > > >  
> > > > >      if (/* io base/limit */
> > > > > @@ -147,9 +151,15 @@ 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)) {
> > > > > -        PCIBridge *s = container_of(d, PCIBridge, dev);
> > > > >          pci_bridge_update_mappings(&s->sec_bus);
> > > > >      }
> > > > > +
> > > > > +    bridge_control_new = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > > > > +    if (!(bridge_control & PCI_BRIDGE_CTL_BUS_RESET) &&
> > > > > +        (bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET)) {
> > > > > +        /* 0 -> 1 */
> > > > > +        pci_bus_reset(&s->sec_bus);
> > > > > +    }
> > > > >  }
> > > > >  
> > > > >  void pci_bridge_disable_base_limit(PCIDevice *dev)
> > > > 
> > > > Presumably this bit will have to be made writeable?
> > > 
> > > Yes, it's already writable.
> > > static void pci_init_wmask_bridge(PCIDevice *d)
> > > ...
> > >    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > 
> > Ouch, that's wrong, isn't it?
> > Bits 15:12 are reserved, readonly, 0.
> > 
> > I think we need the following (untested).
> > Comments?
> 
> Basically it looks good if you left bits 8-11 RO intentional.
> qemu doesn't emulate pci bus cycles, so it won't matter.
>

So this on top?

diff --git a/hw/pci.c b/hw/pci.c
index 7d6d5ad..75da4f7 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -589,7 +589,11 @@ static void pci_init_wmask_bridge(PCIDevice *d)
     memset(d->wmask + PCI_PREF_BASE_UPPER32, 0xff, 8);
 
 /* TODO: add this define to pci_regs.h in linux and then in qemu. */
-#define  PCI_BRIDGE_CTL_VGA_16BIT 0x10	/* VGA 16-bit decode */
+#define  PCI_BRIDGE_CTL_VGA_16BIT	0x10	/* VGA 16-bit decode */
+#define  PCI_BRIDGE_CTL_DISCARD		0x100	/* Primary discard timer */
+#define  PCI_BRIDGE_CTL_SEC_DISCARD	0x200	/* Secondary discard timer */
+#define  PCI_BRIDGE_CTL_DISCARD_STATUS	0x400	/* Discard timer status */
+#define  PCI_BRIDGE_CTL_DISCARD_SERR	0x800	/* Discard timer SERR# enable */
     pci_set_word(d->wmask + PCI_BRIDGE_CONTROL,
                  PCI_BRIDGE_CTL_PARITY |
                  PCI_BRIDGE_CTL_SERR |
@@ -598,7 +602,15 @@ static void pci_init_wmask_bridge(PCIDevice *d)
                  PCI_BRIDGE_CTL_VGA_16BIT |
                  PCI_BRIDGE_CTL_MASTER_ABORT |
                  PCI_BRIDGE_CTL_BUS_RESET |
-                 PCI_BRIDGE_CTL_FAST_BACK);
+                 PCI_BRIDGE_CTL_FAST_BACK |
+                 PCI_BRIDGE_CTL_DISCARD |
+                 PCI_BRIDGE_CTL_SEC_DISCARD |
+                 PCI_BRIDGE_CTL_DISCARD_STATUS |
+                 PCI_BRIDGE_CTL_DISCARD_SERR);
+    /* Below does not do anything as we never set this bit, put here for
+     * completeness. */
+    pci_set_word(d->w1mask + PCI_BRIDGE_CONTROL,
+                 PCI_BRIDGE_CTL_DISCARD_STATUS);
 }
 
 static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)