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

[Thomas Huth]

On 04/11/2020 15.16, BALATON Zoltan wrote:
> 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.

Yes, as far as I can see, there is no way a user could use these devices
from the command line - the "pic" link has to be set up by code. So I'd also
suggest to add the user_creatable = false back again.

 Thomas