[Qemu-devel] [PATCH for-1.6] pxa2xx: Avoid object_get_link_property() assertion for "parent_bus"

[Qemu-devel] [PATCH for-1.6] pxa2xx: Avoid object_get_link_property() assertion for "parent_bus"

[Andreas Färber]

pxa2xx_i2c_init() creates a pxa2xx-i2c-slave device on a second i2c-bus,
which has a NULL parent device. This causes an assertion in
object_get_canonical_path() when accessing pxa2xx-i2c-slave's
"parent_bus" link<bus> property in tosa and likely other PXA2xx machines.

Fix this by using the pxa2xx_i2c device, created just before, as parent.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/arm/pxa2xx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 7de6453..17ddd3f 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1479,6 +1479,7 @@ PXA2xxI2CState *pxa2xx_i2c_init(hwaddr base,
     DeviceState *dev;
     SysBusDevice *i2c_dev;
     PXA2xxI2CState *s;
+    i2c_bus *i2cbus;
 
     dev = qdev_create(NULL, TYPE_PXA2XX_I2C);
     qdev_prop_set_uint32(dev, "size", region_size + 1);
@@ -1491,7 +1492,8 @@ PXA2xxI2CState *pxa2xx_i2c_init(hwaddr base,
 
     s = PXA2XX_I2C(i2c_dev);
     /* FIXME: Should the slave device really be on a separate bus?  */
-    dev = i2c_create_slave(i2c_init_bus(NULL, "dummy"), "pxa2xx-i2c-slave", 0);
+    i2cbus = i2c_init_bus(dev, "dummy");
+    dev = i2c_create_slave(i2cbus, "pxa2xx-i2c-slave", 0);
     s->slave = FROM_I2C_SLAVE(PXA2xxI2CSlaveState, I2C_SLAVE(dev));
     s->slave->host = s;
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for-1.6] pxa2xx: Avoid object_get_link_property() assertion for "parent_bus"

[Andreas Färber]

Am 04.08.2013 15:05, schrieb Andreas Färber:
> pxa2xx_i2c_init() creates a pxa2xx-i2c-slave device on a second i2c-bus,
> which has a NULL parent device. This causes an assertion in
> object_get_canonical_path() when accessing pxa2xx-i2c-slave's
> "parent_bus" link<bus> property in tosa and likely other PXA2xx machines.
> 
> Fix this by using the pxa2xx_i2c device, created just before, as parent.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>

CC'ing Anthony as this may be lurking elsewhere, too.

Unfortunately qtest can still only send QMP command but does not return
the response, so we can't generically test walking the QOM composition
tree in my proposed qom-test.

Andreas

> ---
>  hw/arm/pxa2xx.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index 7de6453..17ddd3f 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -1479,6 +1479,7 @@ PXA2xxI2CState *pxa2xx_i2c_init(hwaddr base,
>      DeviceState *dev;
>      SysBusDevice *i2c_dev;
>      PXA2xxI2CState *s;
> +    i2c_bus *i2cbus;
>  
>      dev = qdev_create(NULL, TYPE_PXA2XX_I2C);
>      qdev_prop_set_uint32(dev, "size", region_size + 1);
> @@ -1491,7 +1492,8 @@ PXA2xxI2CState *pxa2xx_i2c_init(hwaddr base,
>  
>      s = PXA2XX_I2C(i2c_dev);
>      /* FIXME: Should the slave device really be on a separate bus?  */
> -    dev = i2c_create_slave(i2c_init_bus(NULL, "dummy"), "pxa2xx-i2c-slave", 0);
> +    i2cbus = i2c_init_bus(dev, "dummy");
> +    dev = i2c_create_slave(i2cbus, "pxa2xx-i2c-slave", 0);
>      s->slave = FROM_I2C_SLAVE(PXA2xxI2CSlaveState, I2C_SLAVE(dev));
>      s->slave->host = s;
>  

-- 
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 for-1.6] pxa2xx: Avoid object_get_link_property() assertion for "parent_bus"

[Andreas Färber]

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

Am 04.08.2013 15:17, schrieb Andreas Färber:
> Am 04.08.2013 15:05, schrieb Andreas Färber:
>> pxa2xx_i2c_init() creates a pxa2xx-i2c-slave device on a second i2c-bus,
>> which has a NULL parent device. This causes an assertion in
>> object_get_canonical_path() when accessing pxa2xx-i2c-slave's
>> "parent_bus" link<bus> property in tosa and likely other PXA2xx machines.
>>
>> Fix this by using the pxa2xx_i2c device, created just before, as parent.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> CC'ing Anthony as this may be lurking elsewhere, too.
> 
> Unfortunately qtest can still only send QMP command but does not return
> the response, so we can't generically test walking the QOM composition
> tree in my proposed qom-test.

Came up with another way to test: The attached patch complementing my
earlier qom-test series (and based on some more -kernel/-bios error
avoidance for qtest) asserts that all buses except SysBus in the machine
default configurations have a parent. SysBus is known to sit on
/machine/unattached/sysbus and thus is fine, too.

Andreas

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

[-- Attachment #2: qtest-parent_bus.diff --]
[-- Type: text/x-patch, Size: 5307 bytes --]

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 9190a7e..26f9c9b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -445,6 +445,7 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
         object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus), NULL);
         object_unref(OBJECT(bus));
     } else if (bus != sysbus_get_default()) {
+        g_assert_not_reached();
         /* TODO: once all bus devices are qdevified,
            only reset handler for main_system_bus should be registered here. */
         qemu_register_reset(qbus_reset_all_fn, bus);
diff --git a/tests/Makefile b/tests/Makefile
index 3deb158..a7e1319 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -103,6 +103,7 @@ check-qtest-microblaze-y += tests/qom-test$(EXESUF)
 check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 check-qtest-moxie-y += tests/qom-test$(EXESUF)
 check-qtest-or32-y += tests/qom-test$(EXESUF)
+check-qtest-s390x-y += tests/qom-test$(EXESUF)
 check-qtest-unicore32-y += tests/qom-test$(EXESUF)
 check-qtest-xtensa-y += tests/qom-test$(EXESUF)
 check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
diff --git a/tests/qom-test.c b/tests/qom-test.c
index b9cd5f5..af93a59 100644
--- a/tests/qom-test.c
+++ b/tests/qom-test.c
@@ -50,7 +50,7 @@ static const char *arm_machines[] = {
     "connex",
     "verdex",
     "z2",
-    /* n800 covered by tmp105-test */
+    "n800",
     "n810",
     "kzm",
     "vexpress-a9",
@@ -78,23 +78,92 @@ int main(int argc, char **argv)
 
     add_test_cases(arch, "none");
 
-    if (strcmp(arch, "arm") == 0) {
+    if (strcmp(arch, "alpha") == 0) {
+        add_test_cases(arch, "clipper");
+    } else if (strcmp(arch, "arm") == 0) {
         for (i = 0; i < ARRAY_SIZE(arm_machines); i++) {
             add_test_cases(arch, arm_machines[i]);
         }
     } else if (strcmp(arch, "cris") == 0) {
         add_test_cases(arch, "axis-dev88");
+    } else if (strcmp(arch, "i386") == 0 ||
+               strcmp(arch, "x86_64") == 0) {
+        add_test_cases(arch, "pc");
+        add_test_cases(arch, "isapc");
+        add_test_cases(arch, "q35");
+    } else if (strcmp(arch, "lm32") == 0) {
+        add_test_cases(arch, "lm32-evr");
+        add_test_cases(arch, "lm32-uclinux");
+        add_test_cases(arch, "milkymist");
     } else if (strcmp(arch, "m68k") == 0) {
         add_test_cases(arch, "mcf5208evb");
         add_test_cases(arch, "an5206");
         add_test_cases(arch, "dummy");
+    } else if (strcmp(arch, "microblaze") == 0 ||
+               strcmp(arch, "microblazeel") == 0) {
+        add_test_cases(arch, "petalogix-ml605");
+        add_test_cases(arch, "petalogix-s3adsp1800");
+    } else if (strcmp(arch, "mips") == 0 ||
+               strcmp(arch, "mipsel") == 0 ||
+               strcmp(arch, "mips64") == 0 ||
+               strcmp(arch, "mips64el") == 0) {
+        add_test_cases(arch, "malta");
+        add_test_cases(arch, "magnum");
+        add_test_cases(arch, "mips");
+        add_test_cases(arch, "mipssim");
+        add_test_cases(arch, "pica61");
+        if (strcmp(arch, "mips64el") == 0) {
+            add_test_cases(arch, "fulong2e");
+        }
+    } else if (strcmp(arch, "moxie") == 0) {
+        add_test_cases(arch, "moxiesim");
     } else if (strcmp(arch, "or32") == 0) {
         add_test_cases(arch, "or32-sim");
+    } else if (strcmp(arch, "ppc") == 0 ||
+               strcmp(arch, "ppc64") == 0 ||
+               strcmp(arch, "ppcemb") == 0) {
+        if (strcmp(arch, "ppcemb") != 0) {
+            add_test_cases(arch, "g3beige");
+            add_test_cases(arch, "mac99");
+            add_test_cases(arch, "prep");
+            add_test_cases(arch, "ref405ep");
+            add_test_cases(arch, "taihu");
+            add_test_cases(arch, "mpc8544ds");
+            add_test_cases(arch, "ppce500");
+        }
+        add_test_cases(arch, "bamboo");
+        add_test_cases(arch, "virtex-ml507");
+        if (strcmp(arch, "ppc64") == 0) {
+            add_test_cases(arch, "pseries");
+        }
+    } else if (strcmp(arch, "s390x") == 0) {
+        add_test_cases(arch, "s390-virtio");
+        add_test_cases(arch, "s390-ccw-virtio");
+    } else if (strcmp(arch, "sh4") == 0 ||
+               strcmp(arch, "sh4eb") == 0) {
+        add_test_cases(arch, "r2d");
+        add_test_cases(arch, "shix");
+    } else if (strcmp(arch, "sparc") == 0) {
+        add_test_cases(arch, "SS-4");
+        add_test_cases(arch, "SS-5");
+        add_test_cases(arch, "SS-10");
+        add_test_cases(arch, "SS-20");
+        add_test_cases(arch, "SS-600MP");
+        add_test_cases(arch, "LX");
+        add_test_cases(arch, "SPARCClassic");
+        add_test_cases(arch, "SPARCbook");
+        add_test_cases(arch, "leon3_generic");
+    } else if (strcmp(arch, "sparc64") == 0) {
+        add_test_cases(arch, "sun4u");
+        add_test_cases(arch, "sun4v");
+        add_test_cases(arch, "Niagara");
     } else if (strcmp(arch, "unicore32") == 0) {
         add_test_cases(arch, "puv3");
     } else if (strcmp(arch, "xtensa") == 0 ||
                strcmp(arch, "xtensaeb") == 0) {
         add_test_cases(arch, "sim");
+        add_test_cases(arch, "lx60");
+        add_test_cases(arch, "lx200");
     }
 
     return g_test_run();

Re: [Qemu-devel] [PATCH for-1.6] pxa2xx: Avoid object_get_link_property() assertion for "parent_bus"

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori