From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=55683 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OmUEb-00075g-4Q for qemu-devel@nongnu.org; Fri, 20 Aug 2010 12:14:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OmUEZ-0002wJ-Nw for qemu-devel@nongnu.org; Fri, 20 Aug 2010 12:14:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20443) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OmUEZ-0002w5-Gr for qemu-devel@nongnu.org; Fri, 20 Aug 2010 12:14:23 -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> Date: Fri, 20 Aug 2010 18:14:19 +0200 In-Reply-To: <4C6EA5C9.8080700@codemonkey.ws> (Anthony Liguori's message of "Fri, 20 Aug 2010 10:56:57 -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 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. 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?