From patchwork Fri Aug 20 18:12:58 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anthony Liguori X-Patchwork-Id: 62300 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 5FB23B70DD for ; Sat, 21 Aug 2010 04:13:54 +1000 (EST) Received: from localhost ([127.0.0.1]:34932 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OmW6A-0002ZQ-9Q for incoming@patchwork.ozlabs.org; Fri, 20 Aug 2010 14:13:50 -0400 Received: from [140.186.70.92] (port=37135 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OmW5V-0002Z7-68 for qemu-devel@nongnu.org; Fri, 20 Aug 2010 14:13:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OmW5T-0004VY-BR for qemu-devel@nongnu.org; Fri, 20 Aug 2010 14:13:09 -0400 Received: from mail-iw0-f173.google.com ([209.85.214.173]:41154) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OmW5T-0004VF-3W for qemu-devel@nongnu.org; Fri, 20 Aug 2010 14:13:07 -0400 Received: by iwn8 with SMTP id 8so2565739iwn.4 for ; Fri, 20 Aug 2010 11:13:05 -0700 (PDT) Received: by 10.231.32.75 with SMTP id b11mr1641696ibd.162.1282327984637; Fri, 20 Aug 2010 11:13:04 -0700 (PDT) Received: from [192.168.0.105] (cpe-70-123-132-139.austin.res.rr.com [70.123.132.139]) by mx.google.com with ESMTPS id e8sm2816350ibb.2.2010.08.20.11.13.03 (version=TLSv1/SSLv3 cipher=RC4-MD5); Fri, 20 Aug 2010 11:13:03 -0700 (PDT) Message-ID: <4C6EC5AA.6050502@codemonkey.ws> Date: Fri, 20 Aug 2010 13:12:58 -0500 From: Anthony Liguori User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.11) Gecko/20100713 Lightning/1.0b1 Thunderbird/3.0.6 MIME-Version: 1.0 To: 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> In-Reply-To: X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) Cc: Alex Williamson , glommer@redhat.com, qemu-devel@nongnu.org X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 Acked-by: Alex Williamson 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 @@ -256,13 +256,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. @@ -278,13 +271,15 @@ 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, dev->alias_required_for_version); } dev->state = DEV_STATE_INITIALIZED; + if (dev->info->reset) { + dev->info->reset(dev); + } return 0; } @@ -307,6 +302,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 1; +} + +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) { @@ -350,7 +364,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) { @@ -448,6 +461,27 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name) return NULL; } +int qbus_walk_children(BusState *bus, qdev_walkerfn *walker, void *opaque) +{ + DeviceState *dev; + + QLIST_FOREACH(dev, &bus->children, sibling) { + BusState *child; + + if (!walker(dev, opaque)) { + return 0; + } + + QLIST_FOREACH(child, &dev->child_bus, sibling) { + if (!qbus_walk_children(child, walker, opaque)) { + return 0; + } + } + } + + return 1; +} + static BusState *qbus_find_recursive(BusState *bus, const char *name, const BusInfo *info) { diff --git a/hw/qdev.h b/hw/qdev.h index 678f8b7..1e5f983 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -174,13 +174,21 @@ BusState *qdev_get_parent_bus(DeviceState *dev); /*** BUS API. ***/ +/* Returns false to terminate walk; true to continue */ +typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque); + void qbus_create_inplace(BusState *bus, BusInfo *info, DeviceState *parent, const char *name); BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name); +int qbus_walk_children(BusState *bus, qdev_walkerfn *walker, 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 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()); + if (loadvm) { if (load_vmstate(loadvm) < 0) { autostart = 0; -- 1.7.0.4