* Re: [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device
2018-03-19 14:00 [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device Thomas Huth
@ 2018-03-20 0:01 ` David Gibson
2018-03-20 1:17 ` Philippe Mathieu-Daudé
2018-03-20 5:05 ` Mark Cave-Ayland
2 siblings, 0 replies; 5+ messages in thread
From: David Gibson @ 2018-03-20 0:01 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, qemu-ppc, Alexander Graf, Mark Cave-Ayland
[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device
2018-03-19 14:00 [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device Thomas Huth
2018-03-20 0:01 ` David Gibson
@ 2018-03-20 1:17 ` Philippe Mathieu-Daudé
2018-03-20 5:05 ` Mark Cave-Ayland
2 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-03-20 1:17 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, David Gibson
Cc: Mark Cave-Ayland, qemu-ppc, Alexander Graf
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);
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device
2018-03-19 14:00 [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device Thomas Huth
2018-03-20 0:01 ` David Gibson
2018-03-20 1:17 ` Philippe Mathieu-Daudé
@ 2018-03-20 5:05 ` Mark Cave-Ayland
2018-03-20 10:22 ` Thomas Huth
2 siblings, 1 reply; 5+ messages in thread
From: Mark Cave-Ayland @ 2018-03-20 5:05 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, David Gibson; +Cc: qemu-ppc, Alexander Graf
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH for-2.12] hw/misc/macio: Fix crash when listing device properties of macio device
2018-03-20 5:05 ` Mark Cave-Ayland
@ 2018-03-20 10:22 ` Thomas Huth
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Huth @ 2018-03-20 10:22 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel, David Gibson; +Cc: qemu-ppc, Alexander Graf
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
^ permalink raw reply [flat|nested] 5+ messages in thread