Anthony Liguori <anth...@codemonkey.ws> writes: > On 08/20/2010 11:14 AM, Markus Armbruster wrote: >>> The real problem is how we do reset. We shouldn't register a reset >>> handler for every qdev device but rather register a single reset >>> handler that walks the device tree and calls reset on every reachable >>> device. >>> >>> Then we can always call reset in init() and there's no need to have a >>> dev->hotplugged check. The qdev device tree reset handler should not >>> be registered until *after* we call qemu_system_reset() after creating >>> the device model which will ensure that we don't do a double reset. >>> >> Fine with me. >> >> But we need to merge something short term (pre 0.13) to fix hot plug of >> e1000 et al. Use Alex's patch as such a stop-gap? >> > > No, we're accumulating crud in base qdev at an alarming rate. It's > important to fix these things now before it gets prohibitively hard to > take care of. > > Can you and Alex review/try the following patch? It seems to work for > me although I'm not sure how to trigger the original bug. > > Regards, > > Anthony Liguori
Looks good to me, except I dislike one little thing: >>From df719f1cc6ae2cd430e1cc47896a13d25af81e67 Mon Sep 17 00:00:00 2001 > From: Anthony Liguori <aligu...@us.ibm.com> > Date: Fri, 20 Aug 2010 13:06:22 -0500 > Subject: [PATCH] qdev: fix reset with hotplug > > Devices expect to be reset after being initialized. Today, we achieve this by > registering a reset handler in each qdev device. We then rely on this reset > handler getting called after device init but before CPU execution runs. > > Since hot plug results in a device being initialized outside of the normal > system reset, things go badly today. > > This patch changes the reset handling so that qdev has no knowledge of the > global system reset. Instead, qdev devices are reset after initialization and > then a new bus level function is introduced that allows all devices on the bus > to be reset using a depth first transversal. > > We still need to do a system_reset before CPU init to preserve behavior of > non-qdev devices so we make sure to register the qdev-based reset handler > after > that reset. > > 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 <aligu...@us.ibm.com> > > diff --git a/hw/qdev.c b/hw/qdev.c > index e99c73f..dfd91d7 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c [...] > +BusState *sysbus_get_default(void) > +{ > + return main_system_bus; > +} > + > +void qbus_reset_all(BusState *bus) > +{ > + qbus_walk_children(bus, qdev_reset_one, NULL); > +} > + > /* can be used as ->unplug() callback for the simple cases */ > int qdev_simple_unplug_cb(DeviceState *dev) > { [...] > diff --git a/vl.c b/vl.c > index b3e3676..5de1688 100644 > --- a/vl.c > +++ b/vl.c > @@ -2968,6 +2968,9 @@ int main(int argc, char **argv, char **envp) > } > > qemu_system_reset(); > + > + qemu_register_reset((void *)qbus_reset_all, sysbus_get_default()); > + This is inconsistent with qdev_create(). qdev_create() uses null. I agree with the N.B. in your commit message: the root of the tree should be explicit. Implicit is too much magic. But you create a second kind of magic. I don't object to how that works, only to having two different kinds. I'd suggest you either make your qemu_reset_all() work like existing qdev_create(), i.e. null means root. Or change qdev_create() to work like your qemu_reset_all(), i.e. use sysbus_get_default() instead of null. > if (loadvm) { > if (load_vmstate(loadvm) < 0) { > autostart = 0;