[PATCH v2 0/6] QOM minor fixes

[PATCH v2 0/6] QOM minor fixes

[Mark Cave-Ayland]

This series started off as a fix for the nd_table misuse in the sparc32-ledma
device as pointed out by Markus, and then I remembered there was similar
issue around the use of serial_hd() in macio. The last patch is one I've had
sitting in a local branch for a while and is a mistake I made during the
original sabre.c split which seems appropriate to include here.

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

v2:
- Rebase onto master
- Add R-B tags from Philippe
- Remove user_creatable=true from patch 5 as pointed out by Zoltan


Mark Cave-Ayland (6):
  sparc32-dma: use object_initialize_child() for espdma and ledma child
    objects
  sparc32-ledma: use object_initialize_child() for lance child object
  sparc32-espdma: use object_initialize_child() for esp child object
  sparc32-ledma: don't reference nd_table directly within the device
  macio: don't reference serial_hd() directly within the device
  sabre: don't call sysbus_mmio_map() in sabre_realize()

 hw/dma/sparc32_dma.c           | 49 +++++++++++++++++-----------------
 hw/misc/macio/macio.c          |  4 ---
 hw/pci-host/sabre.c            |  8 ------
 hw/ppc/mac_newworld.c          |  6 +++++
 hw/ppc/mac_oldworld.c          |  6 +++++
 hw/sparc/sun4m.c               | 21 +++++++++------
 hw/sparc64/sun4u.c             |  7 +++++
 include/hw/sparc/sparc32_dma.h |  8 +++---
 8 files changed, 60 insertions(+), 49 deletions(-)

-- 
2.20.1

[PATCH v2 1/6] sparc32-dma: use object_initialize_child() for espdma and ledma child objects

[Mark Cave-Ayland]

Store the child objects directly within the sparc32-dma object rather than using
link properties.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/dma/sparc32_dma.c           | 15 +++++++++------
 include/hw/sparc/sparc32_dma.h |  4 ++--
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index d20a5bc065..b25a212f7a 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -379,10 +379,9 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    espdma = qdev_new(TYPE_SPARC32_ESPDMA_DEVICE);
+    espdma = DEVICE(&s->espdma);
     object_property_set_link(OBJECT(espdma), "iommu", iommu, &error_abort);
-    object_property_add_child(OBJECT(s), "espdma", OBJECT(espdma));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(espdma), &error_fatal);
+    sysbus_realize(SYS_BUS_DEVICE(espdma), &error_fatal);
 
     esp = DEVICE(object_resolve_path_component(OBJECT(espdma), "esp"));
     sbd = SYS_BUS_DEVICE(esp);
@@ -394,10 +393,9 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->dmamem, 0x0,
                                 sysbus_mmio_get_region(sbd, 0));
 
-    ledma = qdev_new(TYPE_SPARC32_LEDMA_DEVICE);
+    ledma = DEVICE(&s->ledma);
     object_property_set_link(OBJECT(ledma), "iommu", iommu, &error_abort);
-    object_property_add_child(OBJECT(s), "ledma", OBJECT(ledma));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(ledma), &error_fatal);
+    sysbus_realize(SYS_BUS_DEVICE(ledma), &error_fatal);
 
     lance = DEVICE(object_resolve_path_component(OBJECT(ledma), "lance"));
     sbd = SYS_BUS_DEVICE(lance);
@@ -421,6 +419,11 @@ static void sparc32_dma_init(Object *obj)
 
     memory_region_init(&s->dmamem, OBJECT(s), "dma", DMA_SIZE + DMA_ETH_SIZE);
     sysbus_init_mmio(sbd, &s->dmamem);
+
+    object_initialize_child(obj, "espdma", &s->espdma,
+                            TYPE_SPARC32_ESPDMA_DEVICE);
+    object_initialize_child(obj, "ledma", &s->ledma,
+                            TYPE_SPARC32_LEDMA_DEVICE);
 }
 
 static void sparc32_dma_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/sparc/sparc32_dma.h b/include/hw/sparc/sparc32_dma.h
index e650489414..3348a725f0 100644
--- a/include/hw/sparc/sparc32_dma.h
+++ b/include/hw/sparc/sparc32_dma.h
@@ -48,8 +48,8 @@ struct SPARC32DMAState {
 
     MemoryRegion dmamem;
     MemoryRegion ledma_alias;
-    ESPDMADeviceState *espdma;
-    LEDMADeviceState *ledma;
+    ESPDMADeviceState espdma;
+    LEDMADeviceState ledma;
 };
 
 /* sparc32_dma.c */
-- 
2.20.1

[PATCH v2 2/6] sparc32-ledma: use object_initialize_child() for lance child object

[Mark Cave-Ayland]

Store the child object directly within the sparc32-ledma object rather than
using link properties.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/dma/sparc32_dma.c           | 14 ++++++++------
 include/hw/sparc/sparc32_dma.h |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index b25a212f7a..84196afb95 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -331,24 +331,26 @@ static const TypeInfo sparc32_espdma_device_info = {
 static void sparc32_ledma_device_init(Object *obj)
 {
     DMADeviceState *s = SPARC32_DMA_DEVICE(obj);
+    LEDMADeviceState *ls = SPARC32_LEDMA_DEVICE(obj);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &dma_mem_ops, s,
                           "ledma-mmio", DMA_SIZE);
+
+    object_initialize_child(obj, "lance", &ls->lance, TYPE_LANCE);
 }
 
 static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
 {
-    DeviceState *d;
+    LEDMADeviceState *s = SPARC32_LEDMA_DEVICE(dev);
+    SysBusPCNetState *lance = SYSBUS_PCNET(&s->lance);
     NICInfo *nd = &nd_table[0];
 
     /* FIXME use qdev NIC properties instead of nd_table[] */
     qemu_check_nic_model(nd, TYPE_LANCE);
 
-    d = qdev_new(TYPE_LANCE);
-    object_property_add_child(OBJECT(dev), "lance", OBJECT(d));
-    qdev_set_nic_properties(d, nd);
-    object_property_set_link(OBJECT(d), "dma", OBJECT(dev), &error_abort);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(d), &error_fatal);
+    qdev_set_nic_properties(DEVICE(lance), nd);
+    object_property_set_link(OBJECT(lance), "dma", OBJECT(dev), &error_abort);
+    sysbus_realize(SYS_BUS_DEVICE(lance), &error_fatal);
 }
 
 static void sparc32_ledma_device_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/sparc/sparc32_dma.h b/include/hw/sparc/sparc32_dma.h
index 3348a725f0..80d69cbba2 100644
--- a/include/hw/sparc/sparc32_dma.h
+++ b/include/hw/sparc/sparc32_dma.h
@@ -37,7 +37,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(LEDMADeviceState, SPARC32_LEDMA_DEVICE)
 struct LEDMADeviceState {
     DMADeviceState parent_obj;
 
-    SysBusPCNetState *lance;
+    SysBusPCNetState lance;
 };
 
 #define TYPE_SPARC32_DMA "sparc32-dma"
-- 
2.20.1

[PATCH v2 3/6] sparc32-espdma: use object_initialize_child() for esp child object

[Mark Cave-Ayland]

Store the child object directly within the sparc32-espdma object rather than
using link properties.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/dma/sparc32_dma.c           | 17 ++++++++---------
 include/hw/sparc/sparc32_dma.h |  2 +-
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index 84196afb95..2cbe331959 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -290,27 +290,26 @@ static const TypeInfo sparc32_dma_device_info = {
 static void sparc32_espdma_device_init(Object *obj)
 {
     DMADeviceState *s = SPARC32_DMA_DEVICE(obj);
+    ESPDMADeviceState *es = SPARC32_ESPDMA_DEVICE(obj);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &dma_mem_ops, s,
                           "espdma-mmio", DMA_SIZE);
+
+    object_initialize_child(obj, "esp", &es->esp, TYPE_ESP);
 }
 
 static void sparc32_espdma_device_realize(DeviceState *dev, Error **errp)
 {
-    DeviceState *d;
-    SysBusESPState *sysbus;
-    ESPState *esp;
-
-    d = qdev_new(TYPE_ESP);
-    object_property_add_child(OBJECT(dev), "esp", OBJECT(d));
-    sysbus = ESP(d);
-    esp = &sysbus->esp;
+    ESPDMADeviceState *es = SPARC32_ESPDMA_DEVICE(dev);
+    SysBusESPState *sysbus = ESP(&es->esp);
+    ESPState *esp = &sysbus->esp;
+
     esp->dma_memory_read = espdma_memory_read;
     esp->dma_memory_write = espdma_memory_write;
     esp->dma_opaque = SPARC32_DMA_DEVICE(dev);
     sysbus->it_shift = 2;
     esp->dma_enabled = 1;
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(d), &error_fatal);
+    sysbus_realize(SYS_BUS_DEVICE(sysbus), &error_fatal);
 }
 
 static void sparc32_espdma_device_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/sparc/sparc32_dma.h b/include/hw/sparc/sparc32_dma.h
index 80d69cbba2..cde8ec02cb 100644
--- a/include/hw/sparc/sparc32_dma.h
+++ b/include/hw/sparc/sparc32_dma.h
@@ -28,7 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(ESPDMADeviceState, SPARC32_ESPDMA_DEVICE)
 struct ESPDMADeviceState {
     DMADeviceState parent_obj;
 
-    SysBusESPState *esp;
+    SysBusESPState esp;
 };
 
 #define TYPE_SPARC32_LEDMA_DEVICE "sparc32-ledma"
-- 
2.20.1

[PATCH v2 4/6] sparc32-ledma: don't reference nd_table directly within the device

[Mark Cave-Ayland]

Instead use qdev_set_nic_properties() to configure the on-board NIC at the
sun4m machine level.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/dma/sparc32_dma.c |  5 -----
 hw/sparc/sun4m.c     | 21 +++++++++++++--------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index 2cbe331959..b643b413c5 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -342,12 +342,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
 {
     LEDMADeviceState *s = SPARC32_LEDMA_DEVICE(dev);
     SysBusPCNetState *lance = SYSBUS_PCNET(&s->lance);
-    NICInfo *nd = &nd_table[0];
 
-    /* FIXME use qdev NIC properties instead of nd_table[] */
-    qemu_check_nic_model(nd, TYPE_LANCE);
-
-    qdev_set_nic_properties(DEVICE(lance), nd);
     object_property_set_link(OBJECT(lance), "dma", OBJECT(dev), &error_abort);
     sysbus_realize(SYS_BUS_DEVICE(lance), &error_fatal);
 }
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 54a2b2f9ef..6765982fe9 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -319,7 +319,7 @@ static void *iommu_init(hwaddr addr, uint32_t version, qemu_irq irq)
 
 static void *sparc32_dma_init(hwaddr dma_base,
                               hwaddr esp_base, qemu_irq espdma_irq,
-                              hwaddr le_base, qemu_irq ledma_irq)
+                              hwaddr le_base, qemu_irq ledma_irq, NICInfo *nd)
 {
     DeviceState *dma;
     ESPDMADeviceState *espdma;
@@ -328,16 +328,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
     SysBusPCNetState *lance;
 
     dma = qdev_new(TYPE_SPARC32_DMA);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
-
     espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
                                    OBJECT(dma), "espdma"));
     sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
 
     esp = ESP(object_resolve_path_component(OBJECT(espdma), "esp"));
-    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
-    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);
 
     ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(
                                  OBJECT(dma), "ledma"));
@@ -345,6 +340,14 @@ static void *sparc32_dma_init(hwaddr dma_base,
 
     lance = SYSBUS_PCNET(object_resolve_path_component(
                          OBJECT(ledma), "lance"));
+    qdev_set_nic_properties(DEVICE(lance), nd);
+
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
+    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);
+
     sysbus_mmio_map(SYS_BUS_DEVICE(lance), 0, le_base);
 
     return dma;
@@ -850,6 +853,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
     unsigned int max_cpus = machine->smp.max_cpus;
     Object *ram_memdev = object_resolve_path_type(machine->ram_memdev_id,
                                                   TYPE_MEMORY_BACKEND, NULL);
+    NICInfo *nd = &nd_table[0];
 
     if (machine->ram_size > hwdef->max_mem) {
         error_report("Too much memory for this machine: %" PRId64 ","
@@ -910,9 +914,10 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
                         hwdef->iommu_pad_base, hwdef->iommu_pad_len);
     }
 
+    qemu_check_nic_model(nd, TYPE_LANCE);
     sparc32_dma_init(hwdef->dma_base,
                      hwdef->esp_base, slavio_irq[18],
-                     hwdef->le_base, slavio_irq[16]);
+                     hwdef->le_base, slavio_irq[16], nd);
 
     if (graphic_depth != 8 && graphic_depth != 24) {
         error_report("Unsupported depth: %d", graphic_depth);
@@ -1043,7 +1048,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
                                     machine->initrd_filename,
                                     machine->ram_size, &initrd_size);
 
-    nvram_init(nvram, (uint8_t *)&nd_table[0].macaddr, machine->kernel_cmdline,
+    nvram_init(nvram, (uint8_t *)&nd->macaddr, machine->kernel_cmdline,
                machine->boot_order, machine->ram_size, kernel_size,
                graphic_width, graphic_height, graphic_depth,
                hwdef->nvram_machine_id, "Sun4m");
-- 
2.20.1

Re: [PATCH v2 4/6] sparc32-ledma: don't reference nd_table directly within the device

[Philippe Mathieu-Daudé]

On 9/26/20 4:02 PM, Mark Cave-Ayland wrote:
> Instead use qdev_set_nic_properties() to configure the on-board NIC at the
> sun4m machine level.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/dma/sparc32_dma.c |  5 -----
>  hw/sparc/sun4m.c     | 21 +++++++++++++--------
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
> index 2cbe331959..b643b413c5 100644
> --- a/hw/dma/sparc32_dma.c
> +++ b/hw/dma/sparc32_dma.c
> @@ -342,12 +342,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
>  {
>      LEDMADeviceState *s = SPARC32_LEDMA_DEVICE(dev);
>      SysBusPCNetState *lance = SYSBUS_PCNET(&s->lance);
> -    NICInfo *nd = &nd_table[0];
>  
> -    /* FIXME use qdev NIC properties instead of nd_table[] */
> -    qemu_check_nic_model(nd, TYPE_LANCE);
> -
> -    qdev_set_nic_properties(DEVICE(lance), nd);
>      object_property_set_link(OBJECT(lance), "dma", OBJECT(dev), &error_abort);
>      sysbus_realize(SYS_BUS_DEVICE(lance), &error_fatal);
>  }
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 54a2b2f9ef..6765982fe9 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -319,7 +319,7 @@ static void *iommu_init(hwaddr addr, uint32_t version, qemu_irq irq)
>  
>  static void *sparc32_dma_init(hwaddr dma_base,
>                                hwaddr esp_base, qemu_irq espdma_irq,
> -                              hwaddr le_base, qemu_irq ledma_irq)
> +                              hwaddr le_base, qemu_irq ledma_irq, NICInfo *nd)
>  {
>      DeviceState *dma;
>      ESPDMADeviceState *espdma;
> @@ -328,16 +328,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
>      SysBusPCNetState *lance;
>  
>      dma = qdev_new(TYPE_SPARC32_DMA);
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
> -
>      espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
>                                     OBJECT(dma), "espdma"));
>      sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
>  
>      esp = ESP(object_resolve_path_component(OBJECT(espdma), "esp"));
> -    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
> -    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);
>  
>      ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(
>                                   OBJECT(dma), "ledma"));
> @@ -345,6 +340,14 @@ static void *sparc32_dma_init(hwaddr dma_base,
>  
>      lance = SYSBUS_PCNET(object_resolve_path_component(
>                           OBJECT(ledma), "lance"));
> +    qdev_set_nic_properties(DEVICE(lance), nd);

There is smth odd in how lance is created. It would be clearer to
create TYPE_SPARC32_LEDMA_DEVICE instance_init() of TYPE_SPARC32_DMA.

Can be cleared later, so:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
> +    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);
> +
>      sysbus_mmio_map(SYS_BUS_DEVICE(lance), 0, le_base);
>  
>      return dma;
> @@ -850,6 +853,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>      unsigned int max_cpus = machine->smp.max_cpus;
>      Object *ram_memdev = object_resolve_path_type(machine->ram_memdev_id,
>                                                    TYPE_MEMORY_BACKEND, NULL);
> +    NICInfo *nd = &nd_table[0];
>  
>      if (machine->ram_size > hwdef->max_mem) {
>          error_report("Too much memory for this machine: %" PRId64 ","
> @@ -910,9 +914,10 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>                          hwdef->iommu_pad_base, hwdef->iommu_pad_len);
>      }
>  
> +    qemu_check_nic_model(nd, TYPE_LANCE);
>      sparc32_dma_init(hwdef->dma_base,
>                       hwdef->esp_base, slavio_irq[18],
> -                     hwdef->le_base, slavio_irq[16]);
> +                     hwdef->le_base, slavio_irq[16], nd);
>  
>      if (graphic_depth != 8 && graphic_depth != 24) {
>          error_report("Unsupported depth: %d", graphic_depth);
> @@ -1043,7 +1048,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>                                      machine->initrd_filename,
>                                      machine->ram_size, &initrd_size);
>  
> -    nvram_init(nvram, (uint8_t *)&nd_table[0].macaddr, machine->kernel_cmdline,
> +    nvram_init(nvram, (uint8_t *)&nd->macaddr, machine->kernel_cmdline,
>                 machine->boot_order, machine->ram_size, kernel_size,
>                 graphic_width, graphic_height, graphic_depth,
>                 hwdef->nvram_machine_id, "Sun4m");
> 

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

[Mark Cave-Ayland]

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;
 }
 
 static const TypeInfo macio_bus_info = {
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index e42bd7a626..e59b30e0a7 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -123,6 +123,7 @@ static void ppc_core99_init(MachineState *machine)
     UNINHostState *uninorth_pci;
     PCIBus *pci_bus;
     PCIDevice *macio;
+    ESCCState *escc;
     bool has_pmu, has_adb;
     MACIOIDEState *macio_ide;
     BusState *adb_bus;
@@ -382,6 +383,11 @@ static void ppc_core99_init(MachineState *machine)
     qdev_prop_set_bit(dev, "has-adb", has_adb);
     object_property_set_link(OBJECT(macio), "pic", OBJECT(pic_dev),
                              &error_abort);
+
+    escc = ESCC(object_resolve_path_component(OBJECT(macio), "escc"));
+    qdev_prop_set_chr(DEVICE(escc), "chrA", serial_hd(0));
+    qdev_prop_set_chr(DEVICE(escc), "chrB", serial_hd(1));
+
     pci_realize_and_unref(macio, pci_bus, &error_fatal);
 
     /* We only emulate 2 out of 3 IDE controllers for now */
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 7aba040f1b..25ade63ba3 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -96,6 +96,7 @@ static void ppc_heathrow_init(MachineState *machine)
     PCIBus *pci_bus;
     PCIDevice *macio;
     MACIOIDEState *macio_ide;
+    ESCCState *escc;
     SysBusDevice *s;
     DeviceState *dev, *pic_dev;
     BusState *adb_bus;
@@ -283,6 +284,11 @@ static void ppc_heathrow_init(MachineState *machine)
     qdev_prop_set_uint64(dev, "frequency", tbfreq);
     object_property_set_link(OBJECT(macio), "pic", OBJECT(pic_dev),
                              &error_abort);
+
+    escc = ESCC(object_resolve_path_component(OBJECT(macio), "escc"));
+    qdev_prop_set_chr(DEVICE(escc), "chrA", serial_hd(0));
+    qdev_prop_set_chr(DEVICE(escc), "chrB", serial_hd(1));
+
     pci_realize_and_unref(macio, pci_bus, &error_fatal);
 
     macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
-- 
2.20.1

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

[Thomas Huth]

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?

 Thomas

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

[BALATON Zoltan via]

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

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

[BALATON Zoltan via]

On Wed, 4 Nov 2020, BALATON Zoltan via 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)

Also in macio-oldworld that seems to have the same problem.

Regards,
BALATON Zoltan

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

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

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> 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:
[...]
>> 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.

When you do that, add a comment that explains why.  You might want to
examine existing comments for inspiration.

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

[Mark Cave-Ayland]

On 04/11/2020 12:47, 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?

(goes and looks)

Ah okay it appears to be because the object property link to the PIC is missing, 
which is to be expected as it is only present on the Mac machines.

With the latest round of QOM updates I can see the solution but it's probably a bit 
much now that we've reached rc-0. The easiest thing for the moment is to switch 
user_creatable back to false if this is causing an issue.

Just out of interest how did you find this? My new workflow involves a local "make 
check" with all ppc targets and a pass through Travis-CI and it didn't show up there 
for me (or indeed Peter's pre-merge tests).


ATB,

Mark.

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

[Thomas Huth]

On 04/11/2020 20.29, Mark Cave-Ayland wrote:
> On 04/11/2020 12:47, 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?
> 
> (goes and looks)
> 
> Ah okay it appears to be because the object property link to the PIC is
> missing, which is to be expected as it is only present on the Mac machines.
> 
> With the latest round of QOM updates I can see the solution but it's
> probably a bit much now that we've reached rc-0. The easiest thing for the
> moment is to switch user_creatable back to false if this is causing an issue.

+1 for setting user_creatable back to false ... can you send a patch or
shall I prepare one?

> Just out of interest how did you find this? My new workflow involves a local
> "make check" with all ppc targets and a pass through Travis-CI and it didn't
> show up there for me (or indeed Peter's pre-merge tests).

I've found it with the scripts/device-crash-test script. (You currently need
to apply Eduardo's patch "Check if path is actually an executable file" on
top first to run it)

 Thomas

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

[Mark Cave-Ayland]

On 05/11/2020 05:31, Thomas Huth wrote:

>> (goes and looks)
>>
>> Ah okay it appears to be because the object property link to the PIC is
>> missing, which is to be expected as it is only present on the Mac machines.
>>
>> With the latest round of QOM updates I can see the solution but it's
>> probably a bit much now that we've reached rc-0. The easiest thing for the
>> moment is to switch user_creatable back to false if this is causing an issue.
> 
> +1 for setting user_creatable back to false ... can you send a patch or
> shall I prepare one?

No that's fine, I can come up with a fix over the next couple of days.

>> Just out of interest how did you find this? My new workflow involves a local
>> "make check" with all ppc targets and a pass through Travis-CI and it didn't
>> show up there for me (or indeed Peter's pre-merge tests).
> 
> I've found it with the scripts/device-crash-test script. (You currently need
> to apply Eduardo's patch "Check if path is actually an executable file" on
> top first to run it)

Have you got a link for this? I've tried doing some basic searches in my email client 
and couldn't easily spot it...


ATB,

Mark.

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

[Thomas Huth]

On 06/11/2020 08.35, Mark Cave-Ayland wrote:
> On 05/11/2020 05:31, Thomas Huth wrote:
> 
>>> (goes and looks)
>>>
>>> Ah okay it appears to be because the object property link to the PIC is
>>> missing, which is to be expected as it is only present on the Mac machines.
>>>
>>> With the latest round of QOM updates I can see the solution but it's
>>> probably a bit much now that we've reached rc-0. The easiest thing for the
>>> moment is to switch user_creatable back to false if this is causing an
>>> issue.
>>
>> +1 for setting user_creatable back to false ... can you send a patch or
>> shall I prepare one?
> 
> No that's fine, I can come up with a fix over the next couple of days.

Thanks!

>>> Just out of interest how did you find this? My new workflow involves a local
>>> "make check" with all ppc targets and a pass through Travis-CI and it didn't
>>> show up there for me (or indeed Peter's pre-merge tests).
>>
>> I've found it with the scripts/device-crash-test script. (You currently need
>> to apply Eduardo's patch "Check if path is actually an executable file" on
>> top first to run it)
> 
> Have you got a link for this? I've tried doing some basic searches in my
> email client and couldn't easily spot it...

https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg07549.html

 Thomas

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

[Mark Cave-Ayland]

On 09/11/2020 10:02, Thomas Huth wrote:

>>>> Just out of interest how did you find this? My new workflow involves a local
>>>> "make check" with all ppc targets and a pass through Travis-CI and it didn't
>>>> show up there for me (or indeed Peter's pre-merge tests).
>>>
>>> I've found it with the scripts/device-crash-test script. (You currently need
>>> to apply Eduardo's patch "Check if path is actually an executable file" on
>>> top first to run it)
>>
>> Have you got a link for this? I've tried doing some basic searches in my
>> email client and couldn't easily spot it...
> 
> https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg07549.html

Thanks for this - I gave it a quick run with my patch and I see that there are still 
quite a few failures for qemu-system-ppc, so certainly there are other devices that 
will need attention in future.

I've just realised that I didn't explicitly mark the patch as for-5.2 so I'll resend 
it along with your R-B tag.


ATB,

Mark.

[PATCH v2 6/6] sabre: don't call sysbus_mmio_map() in sabre_realize()

[Mark Cave-Ayland]

The device should not map itself but instead should be mapped to sysbus by the
sun4u machine.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/sabre.c | 8 --------
 hw/sparc64/sun4u.c  | 7 +++++++
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 5ac6283623..5394ad5cd0 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -378,16 +378,8 @@ static void sabre_realize(DeviceState *dev, Error **errp)
 {
     SabreState *s = SABRE(dev);
     PCIHostState *phb = PCI_HOST_BRIDGE(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
     PCIDevice *pci_dev;
 
-    /* sabre_config */
-    sysbus_mmio_map(sbd, 0, s->special_base);
-    /* PCI configuration space */
-    sysbus_mmio_map(sbd, 1, s->special_base + 0x1000000ULL);
-    /* pci_ioport */
-    sysbus_mmio_map(sbd, 2, s->special_base + 0x2000000ULL);
-
     memory_region_init(&s->pci_mmio, OBJECT(s), "pci-mmio", 0x100000000ULL);
     memory_region_add_subregion(get_system_memory(), s->mem_base,
                                 &s->pci_mmio);
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index ad5ca2472a..ebb3b89250 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -588,6 +588,13 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
                              &error_abort);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(sabre), &error_fatal);
 
+    /* sabre_config */
+    sysbus_mmio_map(SYS_BUS_DEVICE(sabre), 0, PBM_SPECIAL_BASE);
+    /* PCI configuration space */
+    sysbus_mmio_map(SYS_BUS_DEVICE(sabre), 1, PBM_SPECIAL_BASE + 0x1000000ULL);
+    /* pci_ioport */
+    sysbus_mmio_map(SYS_BUS_DEVICE(sabre), 2, PBM_SPECIAL_BASE + 0x2000000ULL);
+
     /* Wire up PCI interrupts to CPU */
     for (i = 0; i < IVEC_MAX; i++) {
         qdev_connect_gpio_out_named(DEVICE(sabre), "ivec-irq", i,
-- 
2.20.1

Re: [PATCH v2 0/6] QOM minor fixes

[Mark Cave-Ayland]

On 26/09/2020 15:02, Mark Cave-Ayland wrote:

> This series started off as a fix for the nd_table misuse in the sparc32-ledma
> device as pointed out by Markus, and then I remembered there was similar
> issue around the use of serial_hd() in macio. The last patch is one I've had
> sitting in a local branch for a while and is a mistake I made during the
> original sabre.c split which seems appropriate to include here.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> v2:
> - Rebase onto master
> - Add R-B tags from Philippe
> - Remove user_creatable=true from patch 5 as pointed out by Zoltan
> 
> 
> Mark Cave-Ayland (6):
>    sparc32-dma: use object_initialize_child() for espdma and ledma child
>      objects
>    sparc32-ledma: use object_initialize_child() for lance child object
>    sparc32-espdma: use object_initialize_child() for esp child object
>    sparc32-ledma: don't reference nd_table directly within the device
>    macio: don't reference serial_hd() directly within the device
>    sabre: don't call sysbus_mmio_map() in sabre_realize()
> 
>   hw/dma/sparc32_dma.c           | 49 +++++++++++++++++-----------------
>   hw/misc/macio/macio.c          |  4 ---
>   hw/pci-host/sabre.c            |  8 ------
>   hw/ppc/mac_newworld.c          |  6 +++++
>   hw/ppc/mac_oldworld.c          |  6 +++++
>   hw/sparc/sun4m.c               | 21 +++++++++------
>   hw/sparc64/sun4u.c             |  7 +++++
>   include/hw/sparc/sparc32_dma.h |  8 +++---
>   8 files changed, 60 insertions(+), 49 deletions(-)

I've applied this series (minus patch 5 the macio change which has been merged 
separately via my qemu-macppc branch) to my qemu-sparc branch.


ATB,

Mark.