From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40033 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OmTxr-0000Fx-GS for qemu-devel@nongnu.org; Fri, 20 Aug 2010 11:57:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OmTxo-0000Dg-LO for qemu-devel@nongnu.org; Fri, 20 Aug 2010 11:57:07 -0400 Received: from mail-gw0-f45.google.com ([74.125.83.45]:45311) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OmTxo-0000DP-Fp for qemu-devel@nongnu.org; Fri, 20 Aug 2010 11:57:04 -0400 Received: by gwb11 with SMTP id 11so1402007gwb.4 for ; Fri, 20 Aug 2010 08:57:03 -0700 (PDT) Message-ID: <4C6EA5C9.8080700@codemonkey.ws> Date: Fri, 20 Aug 2010 10:56:57 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] qdev: Reset hotplugged devices References: <20100803161914.15514.59304.stgit@localhost6.localdomain6> <1282308092.3860.0.camel@x201> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Alex Williamson , glommer@redhat.com, qemu-devel@nongnu.org On 08/20/2010 10:47 AM, Markus Armbruster wrote: > Alex Williamson writes: > > >> On Fri, 2010-08-20 at 11:00 +0200, Markus Armbruster wrote: >> >>> Alex Williamson writes: >>> >>> >>>> Several devices rely on their reset() function being called to >>>> initialize device state, e1000 and rtl8139 in particular. When >>>> the device is hot added, the reset doesn't occur, often leaving >>>> the device in an unusable state. Adding a call to reset() after >>>> init() for hotplugged devices puts the device in the expected >>>> state for the guest. >>>> >>>> Signed-off-by: Alex Williamson >>>> --- >>>> >>>> 0.13 candidate? >>>> >>>> hw/qdev.c | 3 +++ >>>> 1 files changed, 3 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/hw/qdev.c b/hw/qdev.c >>>> index e99c73f..b156272 100644 >>>> --- a/hw/qdev.c >>>> +++ b/hw/qdev.c >>>> @@ -278,6 +278,9 @@ int qdev_init(DeviceState *dev) >>>> qdev_free(dev); >>>> return rc; >>>> } >>>> + if (dev->hotplugged) { >>>> + qdev_reset(dev); >>>> + } >>>> qemu_register_reset(qdev_reset, dev); >>>> if (dev->info->vmsd) { >>>> vmstate_register_with_alias_id(dev, -1, dev->info->vmsd, dev, >>>> >>> qdev_reset() isn't necessary when !dev->hotplugged, because then >>> qemu_system_reset() will run shortly, which will call qdev_reset(). >>> Correct? >>> >> Yes, exactly. >> > Hmm. "qdev_init() automatically calls qdev_reset() if hotplug" feels > needlessly complicated. I'd prefer qdev_init() to call it always or > never. > > If "always", we reset twice for cold-plug. Is that bad? > > If "never", we need to reset somewhere else for hot-plug. What about > do_device_add()? > 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. Regards, Anthony Liguori