From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39710 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PTNZ1-0003C7-GT for qemu-devel@nongnu.org; Thu, 16 Dec 2010 18:48:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PTNYy-0003MA-O7 for qemu-devel@nongnu.org; Thu, 16 Dec 2010 18:48:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:16016) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PTNYy-0003M3-E3 for qemu-devel@nongnu.org; Thu, 16 Dec 2010 18:48:44 -0500 Date: Fri, 17 Dec 2010 01:48:04 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree Message-ID: <20101216234804.GG3878@redhat.com> References: <32f3c5f99628967649cf6987dbf9c4c2c4baa568.1290160397.git.yamahata@valinux.co.jp> <4D0A6909.2090404@mail.berlios.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4D0A6909.2090404@mail.berlios.de> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: Isaku Yamahata , Anthony Liguori , qemu-devel@nongnu.org, Aurelien Jarno On Thu, Dec 16, 2010 at 08:31:21PM +0100, Stefan Weil wrote: > Am 19.11.2010 10:55, schrieb Isaku Yamahata: > >From: Anthony Liguori > > > >This patch changes the reset handling so that qdev has no knowledge of the > >global system reset. Instead, a new bus/device level function is > >introduced > >that allows all devices/buses on the bus/device to be reset using a depth > >first transversal. > > > >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 > >Signed-off-by: Isaku Yamahata > >--- > >hw/qdev.c | 28 +++++++++++++++++++--------- > >hw/qdev.h | 4 ++++ > >vl.c | 1 + > >3 files changed, 24 insertions(+), 9 deletions(-) > > > This patch (which is included in qemu master now) introduces > two regressions at least for MIPS system emulation: > > The emulated MIPS machines mipssim and malta no longer work > because qemu tries a null pointer read access (caused by > sysbus_get_default returning NULL). Just run any of these > machines (big or little endian, 32 or 64 bit) with any mips > kernel or bios to get this crash. > > This first regression can be fixed with a patch which I have > sent to qemu-devel. > > The second regression also occurs with MIPS malta. > Networking no longer works with the default pcnet nic. > > This is caused because the reset function for pcnet is no > longer called during system boot. The result in an invalid > mac address (all zero) and a non-working nic. > > For this second regression I still have no simple solution. > Of course mips_malta.c should be converted to qdev which > would fix both problems (but only for malta system emulation). > > Maybe other systems / devices don't get their reset functions > called during system boot, too. > > Stefan Weil Ugh, I keep forgetting about the non-qdev systems. Maybe it's a good way to finally make everyone to convert? If a system maintainer can't be bothered to convert to qdev we can declare the system unsupported :) I think that as long as the system bus is qdev all pci devices are fine. > > > >diff --git a/hw/qdev.c b/hw/qdev.c > >index 11d845a..92ccc8d 100644 > >--- a/hw/qdev.c > >+++ b/hw/qdev.c > >@@ -257,13 +257,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. > >@@ -279,7 +272,6 @@ 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, > >@@ -308,6 +300,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 0; > >+} > >+ > >+BusState *sysbus_get_default(void) > >+{ > >+ return main_system_bus; > >+} > >+ > >+void qbus_reset_all(BusState *bus) > >+{ > >+ qbus_walk_children(bus, qdev_reset_one, NULL, NULL); > >+} > >+ > >/* can be used as ->unplug() callback for the simple cases */ > >int qdev_simple_unplug_cb(DeviceState *dev) > >{ > >@@ -351,7 +362,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) { > >diff --git a/hw/qdev.h b/hw/qdev.h > >index 550fd9b..e5ed333 100644 > >--- a/hw/qdev.h > >+++ b/hw/qdev.h > >@@ -187,10 +187,14 @@ int qbus_walk_children(BusState *bus, > >qdev_walkerfn *devfn, > >qbus_walkerfn *busfn, void *opaque); > >int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn, > >qbus_walkerfn *busfn, 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 c58583d..135fdeb 100644 > >--- a/vl.c > >+++ b/vl.c > >@@ -2976,6 +2976,7 @@ int main(int argc, char **argv, char **envp) > >exit(1); > >} > > > >+ qemu_register_reset((void *)qbus_reset_all, sysbus_get_default()); > >qemu_system_reset(); > >if (loadvm) { > >if (load_vmstate(loadvm) < 0) {