[Qemu-devel] [PATCH] qdev: Fix qdev_try_create() for bus-less devices

[Qemu-devel] [PATCH] qdev: Fix qdev_try_create() for bus-less devices

[Andreas Färber]

Commit 7426aa72c36c908a7d0eae3e38568bb0a70de479 (nand: Don't inherit
from Sysbus) made NAND a device rather than SysBus device. This led to all
boards using the device (akita, borzoi, spitz, terrier, tosa, axis-dev88)
crashing on "info qtree".

The difference to the bus-less ARMCPU is that the device was still being
created by qdev_create() rather than object_new(), which makes assumptions
about a NULL BusState being the SysBus. Fix that.

A longer-term solution will be to stop using qdev_create().

Reported-by: Markus Armbruster <armbru@redhat.com>
Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 82a9123..14c8765 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -131,21 +131,27 @@ DeviceState *qdev_create(BusState *bus, const char *name)
 DeviceState *qdev_try_create(BusState *bus, const char *type)
 {
     DeviceState *dev;
+    ObjectClass *oc;
+    DeviceClass *dc;
 
-    if (object_class_by_name(type) == NULL) {
+    oc = object_class_by_name(type);
+    if (oc == NULL) {
         return NULL;
     }
+    dc = DEVICE_CLASS(oc);
     dev = DEVICE(object_new(type));
     if (!dev) {
         return NULL;
     }
 
-    if (!bus) {
+    if (!bus && dc->bus_type && strcmp(dc->bus_type, "System") == 0) {
         bus = sysbus_get_default();
     }
 
-    qdev_set_parent_bus(dev, bus);
-    object_unref(OBJECT(dev));
+    if (bus != NULL) {
+        qdev_set_parent_bus(dev, bus);
+        object_unref(OBJECT(dev));
+    }
     return dev;
 }
 
-- 
1.8.4.5

Re: [Qemu-devel] [PATCH] qdev: Fix qdev_try_create() for bus-less devices

[Paolo Bonzini]

Il 05/02/2014 15:09, Andreas Färber ha scritto:
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 82a9123..14c8765 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -131,21 +131,27 @@ DeviceState *qdev_create(BusState *bus, const char *name)
>  DeviceState *qdev_try_create(BusState *bus, const char *type)
>  {
>      DeviceState *dev;
> +    ObjectClass *oc;
> +    DeviceClass *dc;
>
> -    if (object_class_by_name(type) == NULL) {
> +    oc = object_class_by_name(type);
> +    if (oc == NULL) {
>          return NULL;
>      }
> +    dc = DEVICE_CLASS(oc);
>      dev = DEVICE(object_new(type));
>      if (!dev) {
>          return NULL;
>      }
>
> -    if (!bus) {
> +    if (!bus && dc->bus_type && strcmp(dc->bus_type, "System") == 0) {

Should you check instead if dev is-a TYPE_SYSBUS_DEVICE?

Does this also leave the nand device out of info qtree, or is it still 
dumped?

Paolo

>          bus = sysbus_get_default();
>      }
>
> -    qdev_set_parent_bus(dev, bus);
> -    object_unref(OBJECT(dev));
> +    if (bus != NULL) {
> +        qdev_set_parent_bus(dev, bus);
> +        object_unref(OBJECT(dev));
> +    }
>      return dev;
>  }
>
>

Re: [Qemu-devel] [PATCH] qdev: Fix qdev_try_create() for bus-less devices

[Andreas Färber]

Am 05.02.2014 17:38, schrieb Paolo Bonzini:
> Il 05/02/2014 15:09, Andreas Färber ha scritto:
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 82a9123..14c8765 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -131,21 +131,27 @@ DeviceState *qdev_create(BusState *bus, const
>> char *name)
>>  DeviceState *qdev_try_create(BusState *bus, const char *type)
>>  {
>>      DeviceState *dev;
>> +    ObjectClass *oc;
>> +    DeviceClass *dc;
>>
>> -    if (object_class_by_name(type) == NULL) {
>> +    oc = object_class_by_name(type);
>> +    if (oc == NULL) {
>>          return NULL;
>>      }
>> +    dc = DEVICE_CLASS(oc);
>>      dev = DEVICE(object_new(type));
>>      if (!dev) {
>>          return NULL;
>>      }
>>
>> -    if (!bus) {
>> +    if (!bus && dc->bus_type && strcmp(dc->bus_type, "System") == 0) {
> 
> Should you check instead if dev is-a TYPE_SYSBUS_DEVICE?

Hmm, that would catch TYPE_SYS_BUS_DEVICE-derived types as well, which
might in theory override bus_type.

The ugly thing here is the cyclic dependency betwen qdev and sysbus,
therefore no TYPE_SYSTEM_BUS.

> Does this also leave the nand device out of info qtree, or is it still
> dumped?

As I pointed out several times already, devices that are not on a bus
are generally *not* printed by info qtree. It depends on the old qbus
concept. Therefore info qtree is not of much use anymore and should be
dropped in favor of the qom-list and qom-get operations, which get us
all devices and all properties. I had once attached a qom-tree for that
and need to put that into a proper patch. Best before the next question
pops up. :)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qdev: Fix qdev_try_create() for bus-less devices

[Andreas Färber]

Am 05.02.2014 17:46, schrieb Andreas Färber:
> Am 05.02.2014 17:38, schrieb Paolo Bonzini:
>> Il 05/02/2014 15:09, Andreas Färber ha scritto:
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 82a9123..14c8765 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -131,21 +131,27 @@ DeviceState *qdev_create(BusState *bus, const
>>> char *name)
>>>  DeviceState *qdev_try_create(BusState *bus, const char *type)
>>>  {
>>>      DeviceState *dev;
>>> +    ObjectClass *oc;
>>> +    DeviceClass *dc;
>>>
>>> -    if (object_class_by_name(type) == NULL) {
>>> +    oc = object_class_by_name(type);
>>> +    if (oc == NULL) {
>>>          return NULL;
>>>      }
>>> +    dc = DEVICE_CLASS(oc);
>>>      dev = DEVICE(object_new(type));
>>>      if (!dev) {
>>>          return NULL;
>>>      }
>>>
>>> -    if (!bus) {
>>> +    if (!bus && dc->bus_type && strcmp(dc->bus_type, "System") == 0) {
>>
>> Should you check instead if dev is-a TYPE_SYSBUS_DEVICE?
> 
> Hmm, that would catch TYPE_SYS_BUS_DEVICE-derived types as well, which
> might in theory override bus_type.
> 
> The ugly thing here is the cyclic dependency betwen qdev and sysbus,
> therefore no TYPE_SYSTEM_BUS.

BTW this patch became optional due to the one-line nand fix (which also
does not show the device in legacy info qtree), but I am still in favor
of merging this or a variation thereof to avoid future troubles.

Andreas

> 
>> Does this also leave the nand device out of info qtree, or is it still
>> dumped?
> 
> As I pointed out several times already, devices that are not on a bus
> are generally *not* printed by info qtree. It depends on the old qbus
> concept. Therefore info qtree is not of much use anymore and should be
> dropped in favor of the qom-list and qom-get operations, which get us
> all devices and all properties. I had once attached a qom-tree for that
> and need to put that into a proper patch. Best before the next question
> pops up. :)
> 
> Andreas
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qdev: Fix qdev_try_create() for bus-less devices

[Peter Crosthwaite]

On Thu, Feb 6, 2014 at 2:46 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 05.02.2014 17:38, schrieb Paolo Bonzini:
>> Il 05/02/2014 15:09, Andreas Färber ha scritto:
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 82a9123..14c8765 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -131,21 +131,27 @@ DeviceState *qdev_create(BusState *bus, const
>>> char *name)
>>>  DeviceState *qdev_try_create(BusState *bus, const char *type)
>>>  {
>>>      DeviceState *dev;
>>> +    ObjectClass *oc;
>>> +    DeviceClass *dc;
>>>
>>> -    if (object_class_by_name(type) == NULL) {
>>> +    oc = object_class_by_name(type);
>>> +    if (oc == NULL) {
>>>          return NULL;
>>>      }
>>> +    dc = DEVICE_CLASS(oc);
>>>      dev = DEVICE(object_new(type));
>>>      if (!dev) {
>>>          return NULL;
>>>      }
>>>
>>> -    if (!bus) {
>>> +    if (!bus && dc->bus_type && strcmp(dc->bus_type, "System") == 0) {
>>
>> Should you check instead if dev is-a TYPE_SYSBUS_DEVICE?
>
> Hmm, that would catch TYPE_SYS_BUS_DEVICE-derived types as well, which
> might in theory override bus_type.
>
> The ugly thing here is the cyclic dependency betwen qdev and sysbus,
> therefore no TYPE_SYSTEM_BUS.
>
>> Does this also leave the nand device out of info qtree, or is it still
>> dumped?
>

2c: Leaving it out would be a good thing. No information is better
than wrong information - NAND devices are not on sysbus.

Regards,
Peter

> As I pointed out several times already, devices that are not on a bus
> are generally *not* printed by info qtree. It depends on the old qbus
> concept. Therefore info qtree is not of much use anymore and should be
> dropped in favor of the qom-list and qom-get operations, which get us
> all devices and all properties. I had once attached a qom-tree for that
> and need to put that into a proper patch. Best before the next question
> pops up. :)
>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>