Re: [PATCH v2 5/6] macio: don't reference serial_hd() directly within the device

[BALATON Zoltan]

On Wed, 4 Nov 2020, Thomas Huth wrote:
> On 26/09/2020 16.02, Mark Cave-Ayland wrote:
>> Instead use qdev_prop_set_chr() to configure the ESCC serial chardevs at the
>> Mac Old World and New World machine level.
>>
>> Also remove the now obsolete comment referring to the use of serial_hd() and
>> the setting of user_creatable to false accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/misc/macio/macio.c | 4 ----
>>  hw/ppc/mac_newworld.c | 6 ++++++
>>  hw/ppc/mac_oldworld.c | 6 ++++++
>>  3 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index 679722628e..51368884d0 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -109,8 +109,6 @@ static void macio_common_realize(PCIDevice *d, Error 
>> **errp)
>>      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_hd(0));
>> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hd(1));
>>      qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
>>      qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>>      if (!qdev_realize(DEVICE(&s->escc), BUS(&s->macio_bus), errp)) {
>> @@ -458,8 +456,6 @@ static void macio_class_init(ObjectClass *klass, void 
>> *data)
>>      k->class_id = PCI_CLASS_OTHERS << 8;
>>      device_class_set_props(dc, macio_properties);
>>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> -    /* Reason: Uses serial_hds in macio_instance_init */
>> -    dc->user_creatable = false;
>>  }
>
> Hi Mark,
>
> the macio device can now be used to crash QEMU:
>
> $ ./qemu-system-ppc -M sam460ex -device macio-newworld
> Segmentation fault (core dumped)
>
> I guess we should either restore the user_creatable flag or add some sanity
> checks elsewhere?

Looks like it needs to check if pic_dev is set:

$ gdb --args ./qemu-system-ppc -M sam460ex -device macio-newworld
(gdb) r
Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault.
0x0000555555c3d65a in qdev_get_named_gpio_list (dev=0x0, name=0x0)
    at ../hw/core/qdev.c:456
456         QLIST_FOREACH(ngl, &dev->gpios, node) {
(gdb) bt
#0  0x0000555555c3d65a in qdev_get_named_gpio_list (dev=0x0, name=0x0)
    at ../hw/core/qdev.c:456
#1  0x0000555555c3e349 in qdev_get_gpio_in_named (dev=<optimized out>,
    name=<optimized out>, n=36) at ../hw/core/qdev.c:532
#2  0x00005555559c690f in macio_newworld_realize (d=<optimized out>,
    errp=0x7fffffffda40) at ../hw/misc/macio/macio.c:301
#3  0x0000555555946334 in pci_qdev_realize (qdev=0x555556b578e0,
    errp=<optimized out>) at ../hw/pci/pci.c:2125
#4  0x0000555555c3f1ff in device_set_realized (obj=<optimized out>,
    value=true, errp=0x7fffffffdb50) at ../hw/core/qdev.c:886
[...]
(gdb) up
#1  0x0000555555c3e349 in qdev_get_gpio_in_named (dev=<optimized out>,
    name=<optimized out>, n=36) at ../hw/core/qdev.c:532
532         NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
(gdb)
#2  0x00005555559c690f in macio_newworld_realize (d=<optimized out>,
    errp=0x7fffffffda40) at ../hw/misc/macio/macio.c:301
301         sysbus_connect_irq(sysbus_dev, 0, qdev_get_gpio_in(pic_dev,
(gdb) l 285
280         .read = timer_read,
281         .write = timer_write,
282         .endianness = DEVICE_LITTLE_ENDIAN,
283     };
284
285     static void macio_newworld_realize(PCIDevice *d, Error **errp)
286     {
287         MacIOState *s = MACIO(d);
288         NewWorldMacIOState *ns = NEWWORLD_MACIO(d);
289         DeviceState *pic_dev = DEVICE(ns->pic);
(gdb)
290         Error *err = NULL;
291         SysBusDevice *sysbus_dev;
292         MemoryRegion *timer_memory = NULL;
293
294         macio_common_realize(d, &err);
295         if (err) {
296             error_propagate(errp, err);
297             return;
298         }
299
(gdb)
300         sysbus_dev = SYS_BUS_DEVICE(&s->escc);
301         sysbus_connect_irq(sysbus_dev, 0, qdev_get_gpio_in(pic_dev,
302                                                            
NEWWORLD_ESCCB_IRQ));
303         sysbus_connect_irq(sysbus_dev, 1, qdev_get_gpio_in(pic_dev,
304                                                            
NEWWORLD_ESCCA_IRQ));
305
306         /* OpenPIC */
307         sysbus_dev = SYS_BUS_DEVICE(ns->pic);
308         memory_region_add_subregion(&s->bar, 0x40000,
309                                     sysbus_mmio_get_region(sysbus_dev, 
0));

Maybe something like:

if (!pic_dev) {
    error_setg(errp, "some meaningful error message");
    return;
}

before the sysbus_connect_irq calls but unless the user can set this via 
the command line somehow then keeping the user_creatable = false with 
comment adjusted to say that this device needs to be connected by board 
code is probably better.

Regards,
BALATON Zoltan