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

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

[Alex Williamson]

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 <alex.williamson@redhat.com>
---

 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,

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

[Glauber Costa]

On Tue, Aug 03, 2010 at 10:19:47AM -0600, Alex Williamson wrote:
> 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 <alex.williamson@redhat.com>
I like this one better.

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

[Markus Armbruster]

Alex Williamson <alex.williamson@redhat.com> 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 <alex.williamson@redhat.com>
> ---
>
>  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?

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

[Alex Williamson]

On Fri, 2010-08-20 at 11:00 +0200, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> 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 <alex.williamson@redhat.com>
> > ---
> >
> >  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.

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

[Markus Armbruster]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Fri, 2010-08-20 at 11:00 +0200, Markus Armbruster wrote:
>> Alex Williamson <alex.williamson@redhat.com> 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 <alex.williamson@redhat.com>
>> > ---
>> >
>> >  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()?

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

[Anthony Liguori]

On 08/20/2010 10:47 AM, Markus Armbruster wrote:
> Alex Williamson<alex.williamson@redhat.com>  writes:
>
>    
>> On Fri, 2010-08-20 at 11:00 +0200, Markus Armbruster wrote:
>>      
>>> Alex Williamson<alex.williamson@redhat.com>  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<alex.williamson@redhat.com>
>>>> ---
>>>>
>>>>   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

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

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 08/20/2010 10:47 AM, Markus Armbruster wrote:
>> Alex Williamson<alex.williamson@redhat.com>  writes:
>>
>>    
>>> On Fri, 2010-08-20 at 11:00 +0200, Markus Armbruster wrote:
>>>      
>>>> Alex Williamson<alex.williamson@redhat.com>  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<alex.williamson@redhat.com>
>>>>> ---
>>>>>
>>>>>   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?

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

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

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


[-- Attachment #2: 0001-qdev-fix-reset-with-hotplug.patch --]
[-- Type: text/x-patch, Size: 5097 bytes --]

>From df719f1cc6ae2cd430e1cc47896a13d25af81e67 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
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 <aliguori@us.ibm.com>

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

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

[Alex Williamson]

On Fri, 2010-08-20 at 13:12 -0500, Anthony Liguori wrote:
> 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.

Yep, that works.  The test is simply to hot add an e1000, much of the
register state is setup in the reset function so the guest won't be able
to make use of the device unless reset is called somewhere along the
way.

Acked-by: Alex Williamson <alex.williamson@redhat.com>

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

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> 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 <aliguori@us.ibm.com>
> 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 <aliguori@us.ibm.com>
>
> 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;

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

[Anthony Liguori]

On 08/21/2010 05:07 AM, Markus Armbruster wrote:
>> 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.
>    

I'm getting rid of the NULL crap too although it's lower on my qdev 
TODO..  sysbus_get_default() is a heck of a lot easier to grep for 
though than NULL so I'd prefer to use this for now.

Regards,

Anthony Liguori

>>       if (loadvm) {
>>           if (load_vmstate(loadvm)<  0) {
>>               autostart = 0;
>>      

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

[Paolo Bonzini]

On 08/20/2010 08:12 PM, Anthony Liguori wrote:
> +/* Returns false to terminate walk; true to continue */
> +typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
> +

Since you're introducing qbus_walk_children, I suggest a different 
interface: qdev_walkerfn should return 0 to walk children, -1 to skip 
walking children, and anything else to terminate walk.  If anything ever 
returns x > 0, qbus_walk_children returns that x, else 
qbus_walk_children returns 0.  This interface is inspired by a similar 
one in GCC and it works well.

If you don't want to introduce the full complication, removing the "-1 
to skip walking children" part would still give the same flexibility WRT 
to the return values, which is the important part.

Paolo

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

[Avi Kivity]

  On 08/20/2010 12:00 PM, Markus Armbruster wrote:
> Alex Williamson<alex.williamson@redhat.com>  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<alex.williamson@redhat.com>
>> ---
>>
>>   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?

Off-topic, but what's the reason for dev->hotplugged's existence?  A 
device is either plugged or not, it is either hotpluggable or not, but 
is there a way to tell, from looking at a plugged device, whether it has 
been hotplugged in the past?

AFAICT it is redundant state and should be removed.

-- 
error compiling committee.c: too many arguments to function

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

[Anthony Liguori]

On 08/23/2010 07:00 AM, Avi Kivity wrote:
> Off-topic, but what's the reason for dev->hotplugged's existence?  A 
> device is either plugged or not, it is either hotpluggable or not, but 
> is there a way to tell, from looking at a plugged device, whether it 
> has been hotplugged in the past?
>
> AFAICT it is redundant state and should be removed.

I've got that in my qdev tree.

http://repo.or.cz/w/qemu/aliguori.git/commit/672720c5c0315d2a5b43f58fd75822cc4a390ae9

Regards,

Anthony Liguori

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

[Anthony Liguori]

On 08/23/2010 06:25 AM, Paolo Bonzini wrote:
> On 08/20/2010 08:12 PM, Anthony Liguori wrote:
>> +/* Returns false to terminate walk; true to continue */
>> +typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
>> +
>
> Since you're introducing qbus_walk_children, I suggest a different 
> interface: qdev_walkerfn should return 0 to walk children, -1 to skip 
> walking children, and anything else to terminate walk.  If anything 
> ever returns x > 0, qbus_walk_children returns that x, else 
> qbus_walk_children returns 0.  This interface is inspired by a similar 
> one in GCC and it works well.

Good suggestion.

Regards,

Anthony Liguori

> If you don't want to introduce the full complication, removing the "-1 
> to skip walking children" part would still give the same flexibility 
> WRT to the return values, which is the important part.
>
> Paolo

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

[Isaku Yamahata]

On Fri, Aug 20, 2010 at 10:56:57AM -0500, Anthony Liguori 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.

I don't see why you re-invent the patch.
Please see the patches I sent out on August 5.
http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00377.html
http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00383.html

Maybe we can merge the patches.
As for your patch, I have some comment.
- bus itself may want its own handler. At lease pci bus needs it.
  And propagating reset signal to children is up to the bus controller.

- child devices should be reset before parent.
  This doesn't matter much though.

-- 
yamahata

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

[Anthony Liguori]

On 08/24/2010 10:07 PM, Isaku Yamahata wrote:
> On Fri, Aug 20, 2010 at 10:56:57AM -0500, Anthony Liguori 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.
>>      
> I don't see why you re-invent the patch.
> Please see the patches I sent out on August 5.
> http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00377.html
> http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00383.html
>    

I honestly didn't connect the two but it's now obvious to me that they 
overlap significantly.

> Maybe we can merge the patches.
> As for your patch, I have some comment.
> - bus itself may want its own handler. At lease pci bus needs it.
>    And propagating reset signal to children is up to the bus controller.
>    

I disagree.  Reset should be equivalent to power off + init and it's not 
something that can be selectively propagated.

There are different types of bus level resets and it may make sense to 
model that in the PCI layer but the qdev reset is a power cycle 
semantically.

I think using a walker pattern is a stronger design than having each 
reset function do it's own transversal because the later has the 
potential to introduce bugs.

Regards,

Anthony Liguori

> - child devices should be reset before parent.
>    This doesn't matter much though.
>
>    

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

[Isaku Yamahata]

On Wed, Aug 25, 2010 at 07:55:27AM -0500, Anthony Liguori wrote:
>> Maybe we can merge the patches.
>> As for your patch, I have some comment.
>> - bus itself may want its own handler. At lease pci bus needs it.
>>    And propagating reset signal to children is up to the bus controller.
>>    
>
> I disagree.  Reset should be equivalent to power off + init and it's not  
> something that can be selectively propagated.
>
> There are different types of bus level resets and it may make sense to  
> model that in the PCI layer but the qdev reset is a power cycle  
> semantically.
>
> I think using a walker pattern is a stronger design than having each  
> reset function do it's own transversal because the later has the  
> potential to introduce bugs.

Warm reset is different from cold reset, so reset isn't equivalent to
a power cycle. Typically registers which report errors must retain the
contents across warm reset, but doesn't across a power cycle.
I can see the reset call back is called when both power on and warm reset,
but I don't see why qdev_reset() is a power cycle semantically.

Although your current patch might work for me at the moment,
the claim that qdev_reset() == a power cycle worries me.

How about adding a argument to distinguish reset type?
Something like QEMU_RESET_COLD, QEMU_RESET_WARM.
-- 
yamahata

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

[Anthony Liguori]

On 08/25/2010 10:17 AM, Isaku Yamahata wrote:
> On Wed, Aug 25, 2010 at 07:55:27AM -0500, Anthony Liguori wrote:
>    
>>> Maybe we can merge the patches.
>>> As for your patch, I have some comment.
>>> - bus itself may want its own handler. At lease pci bus needs it.
>>>     And propagating reset signal to children is up to the bus controller.
>>>
>>>        
>> I disagree.  Reset should be equivalent to power off + init and it's not
>> something that can be selectively propagated.
>>
>> There are different types of bus level resets and it may make sense to
>> model that in the PCI layer but the qdev reset is a power cycle
>> semantically.
>>
>> I think using a walker pattern is a stronger design than having each
>> reset function do it's own transversal because the later has the
>> potential to introduce bugs.
>>      
> Warm reset is different from cold reset, so reset isn't equivalent to
> a power cycle.

But qdev reset is not warm reset.  It's cold reset.  IOW, it's called 
when the device is first powered up.

>   Typically registers which report errors must retain the
> contents across warm reset, but doesn't across a power cycle.
> I can see the reset call back is called when both power on and warm reset,
> but I don't see why qdev_reset() is a power cycle semantically.
>
> Although your current patch might work for me at the moment,
> the claim that qdev_reset() == a power cycle worries me.
>
> How about adding a argument to distinguish reset type?
> Something like QEMU_RESET_COLD, QEMU_RESET_WARM.
>    

Before approaching this, I think we ought to better understand exactly 
what type of reset semantics we would need to support in the future.

I think that starts by understanding exactly what's guaranteed and 
understanding what the use cases are for it.

Regards,

Anthony Liguori

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

[Isaku Yamahata]

On Wed, Aug 25, 2010 at 11:49:19AM -0500, Anthony Liguori wrote:
> On 08/25/2010 10:17 AM, Isaku Yamahata wrote:
>> On Wed, Aug 25, 2010 at 07:55:27AM -0500, Anthony Liguori wrote:
>>    
>>>> Maybe we can merge the patches.
>>>> As for your patch, I have some comment.
>>>> - bus itself may want its own handler. At lease pci bus needs it.
>>>>     And propagating reset signal to children is up to the bus controller.
>>>>
>>>>        
>>> I disagree.  Reset should be equivalent to power off + init and it's not
>>> something that can be selectively propagated.
>>>
>>> There are different types of bus level resets and it may make sense to
>>> model that in the PCI layer but the qdev reset is a power cycle
>>> semantically.
>>>
>>> I think using a walker pattern is a stronger design than having each
>>> reset function do it's own transversal because the later has the
>>> potential to introduce bugs.
>>>      
>> Warm reset is different from cold reset, so reset isn't equivalent to
>> a power cycle.
>
> But qdev reset is not warm reset.  It's cold reset.  IOW, it's called  
> when the device is first powered up.

No. It's not true.
Surely qemu_system_reset() is called on qemu startup from main().
But it is also called from main_loop() as warm reset.
qemu_system_reset_request() is called when warm reset is requested.
That is, qemu_system_reset() doesn't distinguish warm reset from
cold reset. Thus qdev_reset() doesn't.


>>   Typically registers which report errors must retain the
>> contents across warm reset, but doesn't across a power cycle.
>> I can see the reset call back is called when both power on and warm reset,
>> but I don't see why qdev_reset() is a power cycle semantically.
>>
>> Although your current patch might work for me at the moment,
>> the claim that qdev_reset() == a power cycle worries me.
>>
>> How about adding a argument to distinguish reset type?
>> Something like QEMU_RESET_COLD, QEMU_RESET_WARM.
>>    
>
> Before approaching this, I think we ought to better understand exactly  
> what type of reset semantics we would need to support in the future.
>
> I think that starts by understanding exactly what's guaranteed and  
> understanding what the use cases are for it.

Fair enough. How about the followings?
This is just a starting point. I borrowed terminology pci/pcie spec.


reset
	Bring the state of hardware state to consistent state.
	(some state might be left unknown.)
	

system reset
	a hardware mechanism for setting or returning all hardware states
	to the initial conditions.
	Use case:
	In qemu, system_system_reset().


cold reset(power on reset)
	system reset following the application of power.
	Use case:
	In qemu, system_reset()	in main().
	We might want to use this as a power cycle.
	When a device is hot plugged, the device should be cold reset too.
	This is your motivation.
	QEMU_RESET_COLD
	Guarantee:
	The internal status must be same to qdev_init() + qdev_reset()

	
warm reset
	system reset without cycling the supplied power.
	Use case:
	In qemu, system_reset() in main_loop(). There are many places
	which calls qemu_system_reset_request().
	Some state are retained across warm reset. Like PCIe AER, error
	reporting registers need to keep its contents across warm reset
	as OS would examine them and report it when hardware errors caused
	warm reset.
	QEMU_RESET_WARM
	

bus reset
	Reset bus and devices on the bus.
	Bus reset is usually triggered when cold reset, warm reset and
	commanding the bus controller to reset the child bus.
	When bus reset is triggered as command to bus controller,
	the effect is usually same to warm reset on devices on the bus.

	Typically on parallel bus, bus reset is started by asserting
	a designated signal.
	Example: PCI RST#, ATA RESET-, SCSI RST
	
	Use case:
	bus reset as result of programming bus controller. 
	Qemu is currently missing it which I'd like to fill for pci bus.
	ATA and SCSI could benefit from this.
	QEMU_RESET_WARM with bus.
	Guarantee:
	device state under the bus is same as warm reset.


device/function reset:
	Reset triggered by sending reset command to a device.
	This is bus/device specific.
	There might be many reset commands whose effects are different.
	Example: PCI FLR, ATA DEVICE RESET command,
                 scsi bus device reset message.

	This reset is bus specific, so it wouldn't be suitable for qdev
	frame work and could be handled by each bus level.

	
hot reset:
	I just put it here for completeness because pcie defines hot reset.
	A reset propagated in-band across a Link using a Physical Layer
	mechanism. 
	Qemu doesn't emulate physical layer, so we don't care it.
	From software point of view, hot reset has same effect to warm reset.

-- 
yamahata

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

[Anthony Liguori]

On 08/26/2010 03:38 AM, Isaku Yamahata wrote:
>
>> I think that starts by understanding exactly what's guaranteed and
>> understanding what the use cases are for it.
>>      
> Fair enough. How about the followings?
>    

Thanks for enumerating.

> This is just a starting point. I borrowed terminology pci/pcie spec.
>
>
> reset
> 	Bring the state of hardware state to consistent state.
> 	(some state might be left unknown.)
> 	
>
> system reset
> 	a hardware mechanism for setting or returning all hardware states
> 	to the initial conditions.
> 	Use case:
> 	In qemu, system_system_reset().
>
>
> cold reset(power on reset)
> 	system reset following the application of power.
> 	Use case:
> 	In qemu, system_reset()	in main().
> 	We might want to use this as a power cycle.
> 	When a device is hot plugged, the device should be cold reset too.
> 	This is your motivation.
> 	QEMU_RESET_COLD
> 	Guarantee:
> 	The internal status must be same to qdev_init() + qdev_reset()
>    

This is what we do today in QEMU and from a functional perspective it 
covers the type of function we need today.

> 	
> warm reset
> 	system reset without cycling the supplied power.
> 	Use case:
> 	In qemu, system_reset() in main_loop(). There are many places
> 	which calls qemu_system_reset_request().
> 	Some state are retained across warm reset. Like PCIe AER, error
> 	reporting registers need to keep its contents across warm reset
> 	as OS would examine them and report it when hardware errors caused
> 	warm reset.
> 	QEMU_RESET_WARM
>    

With AER, I can't imagine that this matters that much unless we're doing 
PCI passthrough, right?

So maybe the way we should frame this discussion is, what's the type of 
reset semantics that we need to support for PCI passthrough?  The next 
question after that is how do we achieve the different types of reset 
for passthrough devices?

BTW, if you could transfer some of this discussion to a wiki page on 
qemu.org, I think that would be extremely valuable.

Regards,

Anthony Liguori

> bus reset
> 	Reset bus and devices on the bus.
> 	Bus reset is usually triggered when cold reset, warm reset and
> 	commanding the bus controller to reset the child bus.
> 	When bus reset is triggered as command to bus controller,
> 	the effect is usually same to warm reset on devices on the bus.
>
> 	Typically on parallel bus, bus reset is started by asserting
> 	a designated signal.
> 	Example: PCI RST#, ATA RESET-, SCSI RST
> 	
> 	Use case:
> 	bus reset as result of programming bus controller.
> 	Qemu is currently missing it which I'd like to fill for pci bus.
> 	ATA and SCSI could benefit from this.
> 	QEMU_RESET_WARM with bus.
> 	Guarantee:
> 	device state under the bus is same as warm reset.
>
>
> device/function reset:
> 	Reset triggered by sending reset command to a device.
> 	This is bus/device specific.
> 	There might be many reset commands whose effects are different.
> 	Example: PCI FLR, ATA DEVICE RESET command,
>                   scsi bus device reset message.
>
> 	This reset is bus specific, so it wouldn't be suitable for qdev
> 	frame work and could be handled by each bus level.
>
> 	
> hot reset:
> 	I just put it here for completeness because pcie defines hot reset.
> 	A reset propagated in-band across a Link using a Physical Layer
> 	mechanism.
> 	Qemu doesn't emulate physical layer, so we don't care it.
> 	From software point of view, hot reset has same effect to warm reset.
>
>    

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

[Anthony Liguori]

On 08/26/2010 03:38 AM, Isaku Yamahata wrote:
>
> 	QEMU_RESET_COLD
>    

BTW, just from a implementation perspective, I'd rather have multiple 
reset callbacks in qdev instead of having a single callback with a type 
flag.  A type flag implies that every callback has to handle all cases 
whereas with separate callbacks, if a device doesn't implement 
warm_reset we can easily default it to reset (which is a cold reset).

Regards,

Anthony Liguori

> 	Guarantee:
> 	The internal status must be same to qdev_init() + qdev_reset()
>
> 	
> warm reset
> 	system reset without cycling the supplied power.
> 	Use case:
> 	In qemu, system_reset() in main_loop(). There are many places
> 	which calls qemu_system_reset_request().
> 	Some state are retained across warm reset. Like PCIe AER, error
> 	reporting registers need to keep its contents across warm reset
> 	as OS would examine them and report it when hardware errors caused
> 	warm reset.
> 	QEMU_RESET_WARM
> 	
>
> bus reset
> 	Reset bus and devices on the bus.
> 	Bus reset is usually triggered when cold reset, warm reset and
> 	commanding the bus controller to reset the child bus.
> 	When bus reset is triggered as command to bus controller,
> 	the effect is usually same to warm reset on devices on the bus.
>
> 	Typically on parallel bus, bus reset is started by asserting
> 	a designated signal.
> 	Example: PCI RST#, ATA RESET-, SCSI RST
> 	
> 	Use case:
> 	bus reset as result of programming bus controller.
> 	Qemu is currently missing it which I'd like to fill for pci bus.
> 	ATA and SCSI could benefit from this.
> 	QEMU_RESET_WARM with bus.
> 	Guarantee:
> 	device state under the bus is same as warm reset.
>
>
> device/function reset:
> 	Reset triggered by sending reset command to a device.
> 	This is bus/device specific.
> 	There might be many reset commands whose effects are different.
> 	Example: PCI FLR, ATA DEVICE RESET command,
>                   scsi bus device reset message.
>
> 	This reset is bus specific, so it wouldn't be suitable for qdev
> 	frame work and could be handled by each bus level.
>
> 	
> hot reset:
> 	I just put it here for completeness because pcie defines hot reset.
> 	A reset propagated in-band across a Link using a Physical Layer
> 	mechanism.
> 	Qemu doesn't emulate physical layer, so we don't care it.
> 	From software point of view, hot reset has same effect to warm reset.
>
>    

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

[Avi Kivity]

  On 08/25/2010 03:55 PM, Anthony Liguori wrote:
>
>> Maybe we can merge the patches.
>> As for your patch, I have some comment.
>> - bus itself may want its own handler. At lease pci bus needs it.
>>    And propagating reset signal to children is up to the bus controller.
>
> I disagree.  Reset should be equivalent to power off + init and it's 
> not something that can be selectively propagated.

Not all busses propagate reset - SCSI is an example (I think).

-- 
error compiling committee.c: too many arguments to function

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

[Anthony Liguori]

On 08/26/2010 08:15 AM, Avi Kivity wrote:
>  On 08/25/2010 03:55 PM, Anthony Liguori wrote:
>>
>>> Maybe we can merge the patches.
>>> As for your patch, I have some comment.
>>> - bus itself may want its own handler. At lease pci bus needs it.
>>>    And propagating reset signal to children is up to the bus 
>>> controller.
>>
>> I disagree.  Reset should be equivalent to power off + init and it's 
>> not something that can be selectively propagated.
>
> Not all busses propagate reset - SCSI is an example (I think).
>

We're talking about cold reset vs. warm reset.

In the absence of passthrough, I'm struggling to see a useful use-case 
with warm reset.  However, there are many useful things we can do 
assuming a cold reset (like MADV_DONTNEED memory on reboot).

That's not saying we shouldn't do a warm reset, but I'd like to see that 
as an incremental addition to what we have today (like introducing a 
propagating warm reset callback) and thinking through what the actual 
behavior should and shouldn't be.

Regards,

Anthony Liguori

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

[Avi Kivity]

  On 08/26/2010 04:25 PM, Anthony Liguori wrote:
> On 08/26/2010 08:15 AM, Avi Kivity wrote:
>>  On 08/25/2010 03:55 PM, Anthony Liguori wrote:
>>>
>>>> Maybe we can merge the patches.
>>>> As for your patch, I have some comment.
>>>> - bus itself may want its own handler. At lease pci bus needs it.
>>>>    And propagating reset signal to children is up to the bus 
>>>> controller.
>>>
>>> I disagree.  Reset should be equivalent to power off + init and it's 
>>> not something that can be selectively propagated.
>>
>> Not all busses propagate reset - SCSI is an example (I think).
>>
>
> We're talking about cold reset vs. warm reset.
>
> In the absence of passthrough, I'm struggling to see a useful use-case 
> with warm reset.  However, there are many useful things we can do 
> assuming a cold reset (like MADV_DONTNEED memory on reboot).
>
> That's not saying we shouldn't do a warm reset, but I'd like to see 
> that as an incremental addition to what we have today (like 
> introducing a propagating warm reset callback) and thinking through 
> what the actual behavior should and shouldn't be.

Pressing the reset button is a warm reset on real machines, therefore it 
should be a warm reset in qemu.

-- 
error compiling committee.c: too many arguments to function

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

[Blue Swirl]

On Thu, Aug 26, 2010 at 2:29 PM, Avi Kivity <avi@redhat.com> wrote:
>  On 08/26/2010 04:25 PM, Anthony Liguori wrote:
>>
>> On 08/26/2010 08:15 AM, Avi Kivity wrote:
>>>
>>>  On 08/25/2010 03:55 PM, Anthony Liguori wrote:
>>>>
>>>>> Maybe we can merge the patches.
>>>>> As for your patch, I have some comment.
>>>>> - bus itself may want its own handler. At lease pci bus needs it.
>>>>>   And propagating reset signal to children is up to the bus controller.
>>>>
>>>> I disagree.  Reset should be equivalent to power off + init and it's not
>>>> something that can be selectively propagated.
>>>
>>> Not all busses propagate reset - SCSI is an example (I think).
>>>
>>
>> We're talking about cold reset vs. warm reset.
>>
>> In the absence of passthrough, I'm struggling to see a useful use-case
>> with warm reset.  However, there are many useful things we can do assuming a
>> cold reset (like MADV_DONTNEED memory on reboot).
>>
>> That's not saying we shouldn't do a warm reset, but I'd like to see that
>> as an incremental addition to what we have today (like introducing a
>> propagating warm reset callback) and thinking through what the actual
>> behavior should and shouldn't be.
>
> Pressing the reset button is a warm reset on real machines, therefore it
> should be a warm reset in qemu.

I agree. For example Sparc64 CPU uses different reset vector for warm
and cold reset, so system_reset acts like a reset button.

But most real life chips have only one reset signal pin, CPUs may be
the only cases where multiple reset signals are used. I think current
system_reset matches the need well. For most devices, system_reset
initiates the same procedure for cold or warm reset. Those devices
which care about warm reset can count the number of the resets. The
only problem I see is that there is no way to signal cold reset or
power cycle.

Then there are devices which don't have reset signals at all, like
RAM. Their behavior at reset is different compared to power cycling.
Stateless hardware like ROMs do not care about either.

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

[Isaku Yamahata]

I added CC for those who might be interested in this discussion.

On Thu, Aug 26, 2010 at 08:02:38AM -0500, Anthony Liguori wrote:
> On 08/26/2010 03:38 AM, Isaku Yamahata wrote:
>>
>>> I think that starts by understanding exactly what's guaranteed and
>>> understanding what the use cases are for it.
>>>      
>> Fair enough. How about the followings?
>>    
>
> Thanks for enumerating.
>
>> This is just a starting point. I borrowed terminology pci/pcie spec.
>>
>>
>> reset
>> 	Bring the state of hardware state to consistent state.
>> 	(some state might be left unknown.)
>> 	
>>
>> system reset
>> 	a hardware mechanism for setting or returning all hardware states
>> 	to the initial conditions.
>> 	Use case:
>> 	In qemu, system_system_reset().
>>
>>
>> cold reset(power on reset)
>> 	system reset following the application of power.
>> 	Use case:
>> 	In qemu, system_reset()	in main().
>> 	We might want to use this as a power cycle.
>> 	When a device is hot plugged, the device should be cold reset too.
>> 	This is your motivation.
>> 	QEMU_RESET_COLD
>> 	Guarantee:
>> 	The internal status must be same to qdev_init() + qdev_reset()
>>    
>
> This is what we do today in QEMU and from a functional perspective it  
> covers the type of function we need today.
>
>> 	
>> warm reset
>> 	system reset without cycling the supplied power.
>> 	Use case:
>> 	In qemu, system_reset() in main_loop(). There are many places
>> 	which calls qemu_system_reset_request().
>> 	Some state are retained across warm reset. Like PCIe AER, error
>> 	reporting registers need to keep its contents across warm reset
>> 	as OS would examine them and report it when hardware errors caused
>> 	warm reset.
>> 	QEMU_RESET_WARM
>>    
>
> With AER, I can't imagine that this matters that much unless we're doing  
> PCI passthrough, right?

Even without PCI passthrough, AER errors can be injected into emulated pci/pcie
devices in a virtual pcie bus hierarchy from qemu command line.
This is useful to test guest OS AER handler.


> So maybe the way we should frame this discussion is, what's the type of  
> reset semantics that we need to support for PCI passthrough?  The next  
> question after that is how do we achieve the different types of reset  
> for passthrough devices?

What I want is hot reset in pcie terminology on a virtual bus as
PCI_BRIDGE_CTL_BUS_RESET emulation and propagated reset on devices/child
buses which might be a directly assigned.
In direct assigned device case, device-assignment code would be notified the reset.

As hot reset has same effect to warm reset in functionality sense
(the difference is the way to signal it in physical/signal layer which
qemu doesn't care),
I'd like to implement pci bus reset as triggering warm reset on a
(virtual) bus by utilizing qdev frame work.
This would be applicable to ata, scsi, I suppose.


It's another story how to virtualize hot reset on a given directly assigned
pci function or a pcie bus hierarchy. For example, as PCI device assignment
is done per function basis, co-existing functions in the same card shouldn't
be reset.
Another example is, virtual pci bus hierarchy might be reset, but it would
be difficult problem how to map the virtual bus topology to host bus topology.

thanks,

> BTW, if you could transfer some of this discussion to a wiki page on  
> qemu.org, I think that would be extremely valuable.
>
> Regards,
>
> Anthony Liguori
>
>> bus reset
>> 	Reset bus and devices on the bus.
>> 	Bus reset is usually triggered when cold reset, warm reset and
>> 	commanding the bus controller to reset the child bus.
>> 	When bus reset is triggered as command to bus controller,
>> 	the effect is usually same to warm reset on devices on the bus.
>>
>> 	Typically on parallel bus, bus reset is started by asserting
>> 	a designated signal.
>> 	Example: PCI RST#, ATA RESET-, SCSI RST
>> 	
>> 	Use case:
>> 	bus reset as result of programming bus controller.
>> 	Qemu is currently missing it which I'd like to fill for pci bus.
>> 	ATA and SCSI could benefit from this.
>> 	QEMU_RESET_WARM with bus.
>> 	Guarantee:
>> 	device state under the bus is same as warm reset.
>>
>>
>> device/function reset:
>> 	Reset triggered by sending reset command to a device.
>> 	This is bus/device specific.
>> 	There might be many reset commands whose effects are different.
>> 	Example: PCI FLR, ATA DEVICE RESET command,
>>                   scsi bus device reset message.
>>
>> 	This reset is bus specific, so it wouldn't be suitable for qdev
>> 	frame work and could be handled by each bus level.
>>
>> 	
>> hot reset:
>> 	I just put it here for completeness because pcie defines hot reset.
>> 	A reset propagated in-band across a Link using a Physical Layer
>> 	mechanism.
>> 	Qemu doesn't emulate physical layer, so we don't care it.
>> 	From software point of view, hot reset has same effect to warm reset.
>>
>>    
>

-- 
yamahata

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

[Isaku Yamahata]

On Thu, Aug 26, 2010 at 08:02:38AM -0500, Anthony Liguori wrote:
> BTW, if you could transfer some of this discussion to a wiki page on  
> qemu.org, I think that would be extremely valuable.

http://wiki.qemu.org/Features/ResetAPI


-- 
yamahata

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

[Wei Xu]

Isaku and Anthony:

This is excellent discussion! Thanks for forwarding.

Wei Xu

wexu2@cisco.com


On 8/26/10 8:52 PM, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote:

> I added CC for those who might be interested in this discussion.
> 
> On Thu, Aug 26, 2010 at 08:02:38AM -0500, Anthony Liguori wrote:
>> On 08/26/2010 03:38 AM, Isaku Yamahata wrote:
>>> 
>>>> I think that starts by understanding exactly what's guaranteed and
>>>> understanding what the use cases are for it.
>>>>      
>>> Fair enough. How about the followings?
>>>    
>> 
>> Thanks for enumerating.
>> 
>>> This is just a starting point. I borrowed terminology pci/pcie spec.
>>> 
>>> 
>>> reset
>>> Bring the state of hardware state to consistent state.
>>> (some state might be left unknown.)
>>> 
>>> 
>>> system reset
>>> a hardware mechanism for setting or returning all hardware states
>>> to the initial conditions.
>>> Use case:
>>> In qemu, system_system_reset().
>>> 
>>> 
>>> cold reset(power on reset)
>>> system reset following the application of power.
>>> Use case:
>>> In qemu, system_reset() in main().
>>> We might want to use this as a power cycle.
>>> When a device is hot plugged, the device should be cold reset too.
>>> This is your motivation.
>>> QEMU_RESET_COLD
>>> Guarantee:
>>> The internal status must be same to qdev_init() + qdev_reset()
>>>    
>> 
>> This is what we do today in QEMU and from a functional perspective it
>> covers the type of function we need today.
>> 
>>> 
>>> warm reset
>>> system reset without cycling the supplied power.
>>> Use case:
>>> In qemu, system_reset() in main_loop(). There are many places
>>> which calls qemu_system_reset_request().
>>> Some state are retained across warm reset. Like PCIe AER, error
>>> reporting registers need to keep its contents across warm reset
>>> as OS would examine them and report it when hardware errors caused
>>> warm reset.
>>> QEMU_RESET_WARM
>>>    
>> 
>> With AER, I can't imagine that this matters that much unless we're doing
>> PCI passthrough, right?
> 
> Even without PCI passthrough, AER errors can be injected into emulated
> pci/pcie
> devices in a virtual pcie bus hierarchy from qemu command line.
> This is useful to test guest OS AER handler.
> 
> 
>> So maybe the way we should frame this discussion is, what's the type of
>> reset semantics that we need to support for PCI passthrough?  The next
>> question after that is how do we achieve the different types of reset
>> for passthrough devices?
> 
> What I want is hot reset in pcie terminology on a virtual bus as
> PCI_BRIDGE_CTL_BUS_RESET emulation and propagated reset on devices/child
> buses which might be a directly assigned.
> In direct assigned device case, device-assignment code would be notified the
> reset.
> 
> As hot reset has same effect to warm reset in functionality sense
> (the difference is the way to signal it in physical/signal layer which
> qemu doesn't care),
> I'd like to implement pci bus reset as triggering warm reset on a
> (virtual) bus by utilizing qdev frame work.
> This would be applicable to ata, scsi, I suppose.
> 
> 
> It's another story how to virtualize hot reset on a given directly assigned
> pci function or a pcie bus hierarchy. For example, as PCI device assignment
> is done per function basis, co-existing functions in the same card shouldn't
> be reset.
> Another example is, virtual pci bus hierarchy might be reset, but it would
> be difficult problem how to map the virtual bus topology to host bus topology.
> 
> thanks,
> 
>> BTW, if you could transfer some of this discussion to a wiki page on
>> qemu.org, I think that would be extremely valuable.
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>>> bus reset
>>> Reset bus and devices on the bus.
>>> Bus reset is usually triggered when cold reset, warm reset and
>>> commanding the bus controller to reset the child bus.
>>> When bus reset is triggered as command to bus controller,
>>> the effect is usually same to warm reset on devices on the bus.
>>> 
>>> Typically on parallel bus, bus reset is started by asserting
>>> a designated signal.
>>> Example: PCI RST#, ATA RESET-, SCSI RST
>>> 
>>> Use case:
>>> bus reset as result of programming bus controller.
>>> Qemu is currently missing it which I'd like to fill for pci bus.
>>> ATA and SCSI could benefit from this.
>>> QEMU_RESET_WARM with bus.
>>> Guarantee:
>>> device state under the bus is same as warm reset.
>>> 
>>> 
>>> device/function reset:
>>> Reset triggered by sending reset command to a device.
>>> This is bus/device specific.
>>> There might be many reset commands whose effects are different.
>>> Example: PCI FLR, ATA DEVICE RESET command,
>>>                   scsi bus device reset message.
>>> 
>>> This reset is bus specific, so it wouldn't be suitable for qdev
>>> frame work and could be handled by each bus level.
>>> 
>>> 
>>> hot reset:
>>> I just put it here for completeness because pcie defines hot reset.
>>> A reset propagated in-band across a Link using a Physical Layer
>>> mechanism.
>>> Qemu doesn't emulate physical layer, so we don't care it.
>>> From software point of view, hot reset has same effect to warm reset.
>>> 
>>>    
>>