1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device

Thomas Huth posted 1 patch 7 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1521468046-10400-1-git-send-email-thuth@redhat.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 passed
Test s390x passed
hw/misc/macio/macio.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
[Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device
Posted by Thomas Huth 7 years, 7 months ago 
The macio-newworld device can currently be used to abort QEMU unexpectedly:

$ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{ 'execute': 'qmp_capabilities' }
{"return": {}}
{ 'execute': 'device-list-properties',
  'arguments': {'typename': 'macio-newworld'}}
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
Device 'serial0' is in use
Aborted (core dumped)

qdev properties should be set during realize(), not during instance_init(),
so move the related code there to fix this problem.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/misc/macio/macio.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 454244f..b74a657 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -115,6 +115,13 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
     memory_region_add_subregion(&s->bar, 0x16000,
                                 sysbus_mmio_get_region(sysbus_dev, 0));
 
+    qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
+    qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
+    qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
+    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hds[0]);
+    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hds[1]);
+    qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
+    qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
     object_property_set_bool(OBJECT(&s->escc), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
@@ -341,13 +348,6 @@ static void macio_instance_init(Object *obj)
     object_property_add_child(obj, "dbdma", OBJECT(&s->dbdma), NULL);
 
     object_initialize(&s->escc, sizeof(s->escc), TYPE_ESCC);
-    qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
-    qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
-    qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
-    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hds[0]);
-    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hds[1]);
-    qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
-    qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
     qdev_set_parent_bus(DEVICE(&s->escc), sysbus_get_default());
     object_property_add_child(obj, "escc", OBJECT(&s->escc), NULL);
 }
-- 
1.8.3.1
Re: [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device
Posted by David Gibson 7 years, 7 months ago 
On Mon, Mar 19, 2018 at 03:00:46PM +0100, Thomas Huth wrote:
> The macio-newworld device can currently be used to abort QEMU unexpectedly:
> 
> $ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
>  "package": "build-all"}, "capabilities": []}}
> { 'execute': 'qmp_capabilities' }
> {"return": {}}
> { 'execute': 'device-list-properties',
>   'arguments': {'typename': 'macio-newworld'}}
> Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
> Device 'serial0' is in use
> Aborted (core dumped)
> 
> qdev properties should be set during realize(), not during instance_init(),
> so move the related code there to fix this problem.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Applied, thanks.

> ---
>  hw/misc/macio/macio.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 454244f..b74a657 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -115,6 +115,13 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
>      memory_region_add_subregion(&s->bar, 0x16000,
>                                  sysbus_mmio_get_region(sysbus_dev, 0));
>  
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
> +    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hds[0]);
> +    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hds[1]);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>      object_property_set_bool(OBJECT(&s->escc), true, "realized", &err);
>      if (err) {
>          error_propagate(errp, err);
> @@ -341,13 +348,6 @@ static void macio_instance_init(Object *obj)
>      object_property_add_child(obj, "dbdma", OBJECT(&s->dbdma), NULL);
>  
>      object_initialize(&s->escc, sizeof(s->escc), TYPE_ESCC);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hds[0]);
> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hds[1]);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>      qdev_set_parent_bus(DEVICE(&s->escc), sysbus_get_default());
>      object_property_add_child(obj, "escc", OBJECT(&s->escc), NULL);
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device
Posted by Philippe Mathieu-Daudé 7 years, 7 months ago 
On 03/19/2018 03:00 PM, Thomas Huth wrote:
> The macio-newworld device can currently be used to abort QEMU unexpectedly:
> 
> $ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
>  "package": "build-all"}, "capabilities": []}}
> { 'execute': 'qmp_capabilities' }
> {"return": {}}
> { 'execute': 'device-list-properties',
>   'arguments': {'typename': 'macio-newworld'}}
> Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
> Device 'serial0' is in use
> Aborted (core dumped)
> 
> qdev properties should be set during realize(), not during instance_init(),
> so move the related code there to fix this problem.

Oops I missed that.

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/misc/macio/macio.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 454244f..b74a657 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -115,6 +115,13 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
>      memory_region_add_subregion(&s->bar, 0x16000,
>                                  sysbus_mmio_get_region(sysbus_dev, 0));
>  
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
> +    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hds[0]);
> +    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hds[1]);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>      object_property_set_bool(OBJECT(&s->escc), true, "realized", &err);
>      if (err) {
>          error_propagate(errp, err);
> @@ -341,13 +348,6 @@ static void macio_instance_init(Object *obj)
>      object_property_add_child(obj, "dbdma", OBJECT(&s->dbdma), NULL);
>  
>      object_initialize(&s->escc, sizeof(s->escc), TYPE_ESCC);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hds[0]);
> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hds[1]);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>      qdev_set_parent_bus(DEVICE(&s->escc), sysbus_get_default());
>      object_property_add_child(obj, "escc", OBJECT(&s->escc), NULL);
>  }
>
Re: [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device
Posted by Mark Cave-Ayland 7 years, 7 months ago 
On 19/03/18 14:00, Thomas Huth wrote:

> The macio-newworld device can currently be used to abort QEMU unexpectedly:
> 
> $ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
>   "package": "build-all"}, "capabilities": []}}
> { 'execute': 'qmp_capabilities' }
> {"return": {}}
> { 'execute': 'device-list-properties',
>    'arguments': {'typename': 'macio-newworld'}}
> Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
> Device 'serial0' is in use
> Aborted (core dumped)
> 
> qdev properties should be set during realize(), not during instance_init(),
> so move the related code there to fix this problem.

Ah right, presumably this is because of the reference to serial_hds 
again? The patch looks good, although given that it affects 
macio_instance_init() and macio_common_realize() then I would have 
expected this to have failed on the macio-oldworld device too (or 
perhaps you were just unlucky that this was the first macio-*world 
device enumerated).

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/misc/macio/macio.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 454244f..b74a657 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -115,6 +115,13 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
>       memory_region_add_subregion(&s->bar, 0x16000,
>                                   sysbus_mmio_get_region(sysbus_dev, 0));
>   
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
> +    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hds[0]);
> +    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hds[1]);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
> +    qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>       object_property_set_bool(OBJECT(&s->escc), true, "realized", &err);
>       if (err) {
>           error_propagate(errp, err);
> @@ -341,13 +348,6 @@ static void macio_instance_init(Object *obj)
>       object_property_add_child(obj, "dbdma", OBJECT(&s->dbdma), NULL);
>   
>       object_initialize(&s->escc, sizeof(s->escc), TYPE_ESCC);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hds[0]);
> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hds[1]);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
> -    qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>       qdev_set_parent_bus(DEVICE(&s->escc), sysbus_get_default());
>       object_property_add_child(obj, "escc", OBJECT(&s->escc), NULL);
>   }
> 

Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.
Re: [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device
Posted by Thomas Huth 7 years, 7 months ago 
On 20.03.2018 06:05, Mark Cave-Ayland wrote:
> On 19/03/18 14:00, Thomas Huth wrote:
> 
>> The macio-newworld device can currently be used to abort QEMU
>> unexpectedly:
>>
>> $ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
>>   "package": "build-all"}, "capabilities": []}}
>> { 'execute': 'qmp_capabilities' }
>> {"return": {}}
>> { 'execute': 'device-list-properties',
>>    'arguments': {'typename': 'macio-newworld'}}
>> Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
>> Device 'serial0' is in use
>> Aborted (core dumped)
>>
>> qdev properties should be set during realize(), not during
>> instance_init(),
>> so move the related code there to fix this problem.
> 
> Ah right, presumably this is because of the reference to serial_hds
> again?

Right.

> The patch looks good, although given that it affects
> macio_instance_init() and macio_common_realize() then I would have
> expected this to have failed on the macio-oldworld device too (or
> perhaps you were just unlucky that this was the first macio-*world
> device enumerated).

Yes, the bug triggers also with the macio-oldworld device. I just hit it
with the newworld device first.

 Thomas

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.