Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Michael S. Tsirkin]

On Thu, Dec 16, 2010 at 08:31:21PM +0100, Stefan Weil wrote:
> Am 19.11.2010 10:55, schrieb Isaku Yamahata:
> >From: Anthony Liguori <address@hidden>
> >
> >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 <address@hidden>
> >Signed-off-by: Isaku Yamahata <address@hidden>
> >---
> >hw/qdev.c | 28 +++++++++++++++++++---------
> >hw/qdev.h | 4 ++++
> >vl.c | 1 +
> >3 files changed, 24 insertions(+), 9 deletions(-)
> 
> 
> This patch (which is included in qemu master now) introduces
> two regressions at least for MIPS system emulation:
> 
> The emulated MIPS machines mipssim and malta no longer work
> because qemu tries a null pointer read access (caused by
> sysbus_get_default returning NULL). Just run any of these
> machines (big or little endian, 32 or 64 bit) with any mips
> kernel or bios to get this crash.
> 
> This first regression can be fixed with a patch which I have
> sent to qemu-devel.
> 
> The second regression also occurs with MIPS malta.
> Networking no longer works with the default pcnet nic.
> 
> This is caused because the reset function for pcnet is no
> longer called during system boot. The result in an invalid
> mac address (all zero) and a non-working nic.
> 
> For this second regression I still have no simple solution.
> Of course mips_malta.c should be converted to qdev which
> would fix both problems (but only for malta system emulation).
> 
> Maybe other systems / devices don't get their reset functions
> called during system boot, too.
> 
> Stefan Weil

Ugh, I keep forgetting about the non-qdev systems.
Maybe it's a good way to finally make everyone to convert? If a system
maintainer can't be bothered to convert to qdev we can declare the
system unsupported :)

I think that as long as the system bus is qdev all pci devices are fine.


> >
> >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 550fd9b..e5ed333 100644
> >--- a/hw/qdev.h
> >+++ b/hw/qdev.h
> >@@ -187,10 +187,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) {