From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48170 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Omkz7-0001qW-BU for qemu-devel@nongnu.org; Sat, 21 Aug 2010 06:07:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Omkz5-0004io-KR for qemu-devel@nongnu.org; Sat, 21 Aug 2010 06:07:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5776) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Omkz5-0004id-D3 for qemu-devel@nongnu.org; Sat, 21 Aug 2010 06:07:31 -0400 From: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH] qdev: Reset hotplugged devices References: <20100803161914.15514.59304.stgit@localhost6.localdomain6> <1282308092.3860.0.camel@x201> <4C6EA5C9.8080700@codemonkey.ws> <4C6EC5AA.6050502@codemonkey.ws> Date: Sat, 21 Aug 2010 12:07:27 +0200 In-Reply-To: <4C6EC5AA.6050502@codemonkey.ws> (Anthony Liguori's message of "Fri, 20 Aug 2010 13:12:58 -0500") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Alex Williamson , glommer@redhat.com, qemu-devel@nongnu.org Anthony Liguori 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 > 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 > > 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;