[v2,2/6] qdev: reset qdev along with qdev tree

From: Anthony Liguori <anthony@codemonkey.ws>

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 <aliguori@us.ibm.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/qdev.c |   28 +++++++++++++++++++---------
 hw/qdev.h |    4 ++++
 vl.c      |    1 +
 3 files changed, 24 insertions(+), 9 deletions(-)

Am 19.11.2010 10:55, schrieb Isaku Yamahata:
> From: Anthony Liguori <anthony@codemonkey.ws>
>
> 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 <aliguori@us.ibm.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> 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


>
> 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) {

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 <anthony@codemonkey.ws>
> >
> >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 <aliguori@us.ibm.com>
> >Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> >---
> >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) {

On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
> 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<anthony@codemonkey.ws>
>>>
>>> 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<aliguori@us.ibm.com>
>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp>
>>> ---
>>> 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 :)
>    

Honestly, I think that's a fair thing to do.

Regards,

Anthony Liguori

On 17 December 2010 00:24, Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:
> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>> 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 :)

> Honestly, I think that's a fair thing to do.

I'm guessing that would be a transition/conversion to make for the
0.15 release, right?

-- PMM

On 12/17/2010 08:45 AM, Peter Maydell wrote:
> On 17 December 2010 00:24, Anthony Liguori<aliguori@linux.vnet.ibm.com>  wrote:
>    
>> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>>      
>>> 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 :)
>>>        
>    
>> Honestly, I think that's a fair thing to do.
>>      
> I'm guessing that would be a transition/conversion to make for the
> 0.15 release, right?
>    

Yes.

Regards,

Anthony Liguori

> -- PMM
>

Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:

> On 12/17/2010 08:45 AM, Peter Maydell wrote:
>> On 17 December 2010 00:24, Anthony Liguori<aliguori@linux.vnet.ibm.com>  wrote:
>>    
>>> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>>>      
>>>> 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 :)
>>>>        
>>    
>>> Honestly, I think that's a fair thing to do.
>>>      
>> I'm guessing that would be a transition/conversion to make for the
>> 0.15 release, right?
>>    
>
> Yes.

Let's do it.

Am 20.12.2010 13:58, schrieb Markus Armbruster:
> Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:
>
>> On 12/17/2010 08:45 AM, Peter Maydell wrote:
>>> On 17 December 2010 00:24, Anthony 
>>> Liguori<aliguori@linux.vnet.ibm.com> wrote:
>>>
>>>> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>>>>
>>>>> 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 :)
>>>>>
>>>
>>>> Honestly, I think that's a fair thing to do.
>>>>
>>> I'm guessing that would be a transition/conversion to make for the
>>> 0.15 release, right?
>>>
>>
>> Yes.
>
> Let's do it.


I already tried to convert malta / gt64xxx to qdev. While simple
devices are easily converted, I'm still struggling with gt64xxx.

There is some qdev documentation for end users (command line
parameters), but is there qdev documentation for developers, too?
If not, adding a qdev chapter to qemu-tech.texi (preferred), writing a
qdev wiki page, code templates (for systems / buses / bus controllers /
bus devices) or some other form of documentation would be a good start
for release 0.15 (imho).

Regards
Stefan

Stefan Weil <weil@mail.berlios.de> writes:

> Am 20.12.2010 13:58, schrieb Markus Armbruster:
>> Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:
>>
>>> On 12/17/2010 08:45 AM, Peter Maydell wrote:
>>>> On 17 December 2010 00:24, Anthony
>>>> Liguori<aliguori@linux.vnet.ibm.com> wrote:
>>>>
>>>>> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>>>>>
>>>>>> 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 :)
>>>>>>
>>>>
>>>>> Honestly, I think that's a fair thing to do.
>>>>>
>>>> I'm guessing that would be a transition/conversion to make for the
>>>> 0.15 release, right?
>>>>
>>>
>>> Yes.
>>
>> Let's do it.
>
>
> I already tried to convert malta / gt64xxx to qdev. While simple
> devices are easily converted, I'm still struggling with gt64xxx.

Specific problems?

> There is some qdev documentation for end users (command line
> parameters), but is there qdev documentation for developers, too?
> If not, adding a qdev chapter to qemu-tech.texi (preferred), writing a
> qdev wiki page, code templates (for systems / buses / bus controllers /
> bus devices) or some other form of documentation would be a good start
> for release 0.15 (imho).

It's not what you ask for, but perhaps it helps a bit anyway:
http://www.linux-kvm.org/wiki/images/f/fe/2010-forum-armbru-qdev.pdf