Re: [Qemu-devel] [PATCH] qdev: Reset hotplugged devices

[Markus Armbruster]

Anthony Liguori <address@hidden> 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 <address@hidden>
> 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 <address@hidden>
>
> 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;