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-ayl...@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