[PATCH 0/2] m68k/q800: make the GLUE chip a QOM device

[PATCH 0/2] m68k/q800: make the GLUE chip a QOM device

[Peter Maydell]

This series is 6.0 material really I think.  It's a bit of cleanup
prompted by a Coverity issue, CID 1421883.  There are another half
dozen or so similar issues, where Coverity is complaining that we
allocate an array of qemu_irqs with qemu_allocate_irqs() in a board
init function -- in this case the 'pic' array in q800_init() -- and
then we return from the board init function and the memory is leaked,
in the sense that nobody has a pointer to it any more.

The leak isn't real, in that board init functions are called only
once, and the array of qemu_irqs really does need to stay around for
the life of the simulation, so these are pretty much insignificant
as Coverity issues go. But this coding style which uses a free-floating
set of qemu_irqs is not very "modern QEMU", so the issues act as
a nudge that we should clean the code up by encapsulating the
interrupt-line behaviour in a QOM device. In the q800 case there
actually is already a GLUEState struct, it just needs to be turned
into a QOM device with GPIO input lines. Patch 2 does that.

Patch 1 fixes a bug I noticed while doing this work -- it's
not valid to connect two qemu_irq lines directly to the same
input (here both ESCC irq lines go to pic[3]) because it produces
weird behaviour like "both lines are asserted but the device
consuming the interrupt sees the line deassert when one of the
two inputs goes low, rather than only when they both go low".
You need to put an explicit OR gate in, assuming that logical-OR
is the desired behaviour, which it usually is.

Tested only with 'make check' and 'make check-acceptance',
but the latter does have a q800 bootup test.

thanks
-- PMM

Peter Maydell (2):
  hw/m68k/q800: Don't connect two qemu_irqs directly to the same input
  hw/m68k/q800.c: Make the GLUE chip an actual QOM device

 hw/m68k/q800.c  | 92 ++++++++++++++++++++++++++++++++++++++++++-------
 hw/m68k/Kconfig |  1 +
 2 files changed, 80 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input

[Peter Maydell]

The q800 board code connects both of the IRQ outputs of the ESCC
to the same pic[3] qemu_irq. Connecting two qemu_irqs outputs directly
to the same input is not valid as it produces subtly wrong behaviour
(for instance if both the IRQ lines are high, and then one goes
low, the PIC input will see this as a high-to-low transition
even though the second IRQ line should still be holding it high).

This kind of wiring needs an explicitly created OR gate; add one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/q800.c  | 12 ++++++++++--
 hw/m68k/Kconfig |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index ce4b47c3e34..dc13007aaf2 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -28,6 +28,7 @@
 #include "hw/hw.h"
 #include "hw/boards.h"
 #include "hw/irq.h"
+#include "hw/or-irq.h"
 #include "elf.h"
 #include "hw/loader.h"
 #include "ui/console.h"
@@ -171,6 +172,7 @@ static void q800_init(MachineState *machine)
     CPUState *cs;
     DeviceState *dev;
     DeviceState *via_dev;
+    DeviceState *escc_orgate;
     SysBusESPState *sysbus_esp;
     ESPState *esp;
     SysBusDevice *sysbus;
@@ -283,8 +285,14 @@ static void q800_init(MachineState *machine)
     qdev_prop_set_uint32(dev, "chnAtype", 0);
     sysbus = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(sysbus, &error_fatal);
-    sysbus_connect_irq(sysbus, 0, pic[3]);
-    sysbus_connect_irq(sysbus, 1, pic[3]);
+
+    /* Logically OR both its IRQs together */
+    escc_orgate = DEVICE(object_new(TYPE_OR_IRQ));
+    object_property_set_int(OBJECT(escc_orgate), "num-lines", 2, &error_fatal);
+    qdev_realize_and_unref(escc_orgate, NULL, &error_fatal);
+    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(escc_orgate, 0));
+    sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(escc_orgate, 1));
+    qdev_connect_gpio_out(DEVICE(escc_orgate), 0, pic[3]);
     sysbus_mmio_map(sysbus, 0, SCC_BASE);
 
     /* SCSI */
diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
index c757e7dfa48..60d7bcfb8f2 100644
--- a/hw/m68k/Kconfig
+++ b/hw/m68k/Kconfig
@@ -22,3 +22,4 @@ config Q800
     select ESCC
     select ESP
     select DP8393X
+    select OR_IRQ
-- 
2.20.1

[PATCH 2/2] hw/m68k/q800.c: Make the GLUE chip an actual QOM device

[Peter Maydell]

The handling of the GLUE (General Logic Unit) device is
currently open-coded. Make this into a proper QOM device.

This minor piece of modernisation gets rid of the free
floating qemu_irq array 'pic', which Coverity points out
is technically leaked when we exit the machine init function.
(The replacement glue device is not leaked because it gets
added to the sysbus, so it's accessible via that.)

Fixes: Coverity CID 1421883
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/q800.c | 82 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 70 insertions(+), 12 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index dc13007aaf2..05bb372f958 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -47,6 +47,7 @@
 #include "sysemu/qtest.h"
 #include "sysemu/runstate.h"
 #include "sysemu/reset.h"
+#include "migration/vmstate.h"
 
 #define MACROM_ADDR     0x40800000
 #define MACROM_SIZE     0x00100000
@@ -94,10 +95,14 @@
  * CPU.
  */
 
-typedef struct {
+#define TYPE_GLUE "q800-glue"
+OBJECT_DECLARE_SIMPLE_TYPE(GLUEState, GLUE)
+
+struct GLUEState {
+    SysBusDevice parent_obj;
     M68kCPU *cpu;
     uint8_t ipr;
-} GLUEState;
+};
 
 static void GLUE_set_irq(void *opaque, int irq, int level)
 {
@@ -119,6 +124,58 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
     m68k_set_irq_level(s->cpu, 0, 0);
 }
 
+static void glue_reset(DeviceState *dev)
+{
+    GLUEState *s = GLUE(dev);
+
+    s->ipr = 0;
+}
+
+static const VMStateDescription vmstate_glue = {
+    .name = "q800-glue",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(ipr, GLUEState),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
+/*
+ * If the m68k CPU implemented its inbound irq lines as GPIO lines
+ * rather than via the m68k_set_irq_level() function we would not need
+ * this cpu link property and could instead provide outbound IRQ lines
+ * that the board could wire up to the CPU.
+ */
+static Property glue_properties[] = {
+    DEFINE_PROP_LINK("cpu", GLUEState, cpu, TYPE_M68K_CPU, M68kCPU *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void glue_init(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    qdev_init_gpio_in(dev, GLUE_set_irq, 8);
+}
+
+static void glue_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &vmstate_glue;
+    dc->reset = glue_reset;
+    device_class_set_props(dc, glue_properties);
+}
+
+static const TypeInfo glue_info = {
+    .name = TYPE_GLUE,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(GLUEState),
+    .instance_init = glue_init,
+    .class_init = glue_class_init,
+};
+
 static void main_cpu_reset(void *opaque)
 {
     M68kCPU *cpu = opaque;
@@ -178,8 +235,7 @@ static void q800_init(MachineState *machine)
     SysBusDevice *sysbus;
     BusState *adb_bus;
     NubusBus *nubus;
-    GLUEState *irq;
-    qemu_irq *pic;
+    DeviceState *glue;
     DriveInfo *dinfo;
 
     linux_boot = (kernel_filename != NULL);
@@ -213,10 +269,9 @@ static void q800_init(MachineState *machine)
     }
 
     /* IRQ Glue */
-
-    irq = g_new0(GLUEState, 1);
-    irq->cpu = cpu;
-    pic = qemu_allocate_irqs(GLUE_set_irq, irq, 8);
+    glue = qdev_new(TYPE_GLUE);
+    object_property_set_link(OBJECT(glue), "cpu", OBJECT(cpu), &error_abort);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(glue), &error_fatal);
 
     /* VIA */
 
@@ -228,8 +283,10 @@ static void q800_init(MachineState *machine)
     sysbus = SYS_BUS_DEVICE(via_dev);
     sysbus_realize_and_unref(sysbus, &error_fatal);
     sysbus_mmio_map(sysbus, 0, VIA_BASE);
-    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 0, pic[0]);
-    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 1, pic[1]);
+    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 0,
+                                qdev_get_gpio_in(glue, 0));
+    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 1,
+                                qdev_get_gpio_in(glue, 1));
 
 
     adb_bus = qdev_get_child_bus(via_dev, "adb.0");
@@ -270,7 +327,7 @@ static void q800_init(MachineState *machine)
     sysbus_realize_and_unref(sysbus, &error_fatal);
     sysbus_mmio_map(sysbus, 0, SONIC_BASE);
     sysbus_mmio_map(sysbus, 1, SONIC_PROM_BASE);
-    sysbus_connect_irq(sysbus, 0, pic[2]);
+    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 2));
 
     /* SCC */
 
@@ -292,7 +349,7 @@ static void q800_init(MachineState *machine)
     qdev_realize_and_unref(escc_orgate, NULL, &error_fatal);
     sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(escc_orgate, 0));
     sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(escc_orgate, 1));
-    qdev_connect_gpio_out(DEVICE(escc_orgate), 0, pic[3]);
+    qdev_connect_gpio_out(DEVICE(escc_orgate), 0, qdev_get_gpio_in(glue, 3));
     sysbus_mmio_map(sysbus, 0, SCC_BASE);
 
     /* SCSI */
@@ -457,6 +514,7 @@ static const TypeInfo q800_machine_typeinfo = {
 static void q800_machine_register_types(void)
 {
     type_register_static(&q800_machine_typeinfo);
+    type_register_static(&glue_info);
 }
 
 type_init(q800_machine_register_types)
-- 
2.20.1

Re: [PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input

[Philippe Mathieu-Daudé]

Cc'ing SPARC maintainers ...

On 11/7/20 12:51 AM, Peter Maydell wrote:
> The q800 board code connects both of the IRQ outputs of the ESCC
> to the same pic[3] qemu_irq. Connecting two qemu_irqs outputs directly
> to the same input is not valid as it produces subtly wrong behaviour
> (for instance if both the IRQ lines are high, and then one goes
> low, the PIC input will see this as a high-to-low transition
> even though the second IRQ line should still be holding it high).
> 
> This kind of wiring needs an explicitly created OR gate; add one.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/q800.c  | 12 ++++++++++--
>  hw/m68k/Kconfig |  1 +
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index ce4b47c3e34..dc13007aaf2 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -28,6 +28,7 @@
>  #include "hw/hw.h"
>  #include "hw/boards.h"
>  #include "hw/irq.h"
> +#include "hw/or-irq.h"
>  #include "elf.h"
>  #include "hw/loader.h"
>  #include "ui/console.h"
> @@ -171,6 +172,7 @@ static void q800_init(MachineState *machine)
>      CPUState *cs;
>      DeviceState *dev;
>      DeviceState *via_dev;
> +    DeviceState *escc_orgate;
>      SysBusESPState *sysbus_esp;
>      ESPState *esp;
>      SysBusDevice *sysbus;
> @@ -283,8 +285,14 @@ static void q800_init(MachineState *machine)
>      qdev_prop_set_uint32(dev, "chnAtype", 0);
>      sysbus = SYS_BUS_DEVICE(dev);
>      sysbus_realize_and_unref(sysbus, &error_fatal);
> -    sysbus_connect_irq(sysbus, 0, pic[3]);
> -    sysbus_connect_irq(sysbus, 1, pic[3]);

... because sun4m_hw_init() has the same issue:

 986     dev = qdev_new(TYPE_ESCC);
...
 996     sysbus_connect_irq(s, 0, slavio_irq[14]);
 997     sysbus_connect_irq(s, 1, slavio_irq[14]);
...
1011     sysbus_connect_irq(s, 0, slavio_irq[15]);
1012     sysbus_connect_irq(s, 1,  slavio_irq[15]);

> +
> +    /* Logically OR both its IRQs together */
> +    escc_orgate = DEVICE(object_new(TYPE_OR_IRQ));
> +    object_property_set_int(OBJECT(escc_orgate), "num-lines", 2, &error_fatal);
> +    qdev_realize_and_unref(escc_orgate, NULL, &error_fatal);
> +    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(escc_orgate, 0));
> +    sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(escc_orgate, 1));
> +    qdev_connect_gpio_out(DEVICE(escc_orgate), 0, pic[3]);
>      sysbus_mmio_map(sysbus, 0, SCC_BASE);
>  
>      /* SCSI */
> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
> index c757e7dfa48..60d7bcfb8f2 100644
> --- a/hw/m68k/Kconfig
> +++ b/hw/m68k/Kconfig
> @@ -22,3 +22,4 @@ config Q800
>      select ESCC
>      select ESP
>      select DP8393X
> +    select OR_IRQ
> 

Re: [PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input

[Philippe Mathieu-Daudé]

> On 11/7/20 12:51 AM, Peter Maydell wrote:
>> The q800 board code connects both of the IRQ outputs of the ESCC
>> to the same pic[3] qemu_irq. Connecting two qemu_irqs outputs directly
>> to the same input is not valid as it produces subtly wrong behaviour
>> (for instance if both the IRQ lines are high, and then one goes
>> low, the PIC input will see this as a high-to-low transition
>> even though the second IRQ line should still be holding it high).
>>
>> This kind of wiring needs an explicitly created OR gate; add one.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/m68k/q800.c  | 12 ++++++++++--
>>  hw/m68k/Kconfig |  1 +
>>  2 files changed, 11 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input

[Laurent Vivier]

Le 07/11/2020 à 00:51, Peter Maydell a écrit :
> The q800 board code connects both of the IRQ outputs of the ESCC
> to the same pic[3] qemu_irq. Connecting two qemu_irqs outputs directly
> to the same input is not valid as it produces subtly wrong behaviour
> (for instance if both the IRQ lines are high, and then one goes
> low, the PIC input will see this as a high-to-low transition
> even though the second IRQ line should still be holding it high).
> 
> This kind of wiring needs an explicitly created OR gate; add one.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/q800.c  | 12 ++++++++++--
>  hw/m68k/Kconfig |  1 +
>  2 files changed, 11 insertions(+), 2 deletions(-)
...

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 2/2] hw/m68k/q800.c: Make the GLUE chip an actual QOM device

[Laurent Vivier]

Le 07/11/2020 à 00:51, Peter Maydell a écrit :
> The handling of the GLUE (General Logic Unit) device is
> currently open-coded. Make this into a proper QOM device.
> 
> This minor piece of modernisation gets rid of the free
> floating qemu_irq array 'pic', which Coverity points out
> is technically leaked when we exit the machine init function.
> (The replacement glue device is not leaked because it gets
> added to the sysbus, so it's accessible via that.)
> 
> Fixes: Coverity CID 1421883
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/q800.c | 82 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 70 insertions(+), 12 deletions(-)
> 

Reviewed-by: Laurent vivier <laurent@vivier.eu>

Re: [PATCH 2/2] hw/m68k/q800.c: Make the GLUE chip an actual QOM device

[Philippe Mathieu-Daudé]

Hi Peter,

On 11/7/20 12:51 AM, Peter Maydell wrote:
> The handling of the GLUE (General Logic Unit) device is
> currently open-coded. Make this into a proper QOM device.
> 
> This minor piece of modernisation gets rid of the free
> floating qemu_irq array 'pic', which Coverity points out
> is technically leaked when we exit the machine init function.
> (The replacement glue device is not leaked because it gets
> added to the sysbus, so it's accessible via that.)
> 
> Fixes: Coverity CID 1421883
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/q800.c | 82 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 70 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index dc13007aaf2..05bb372f958 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -47,6 +47,7 @@
>  #include "sysemu/qtest.h"
>  #include "sysemu/runstate.h"
>  #include "sysemu/reset.h"
> +#include "migration/vmstate.h"
>  
>  #define MACROM_ADDR     0x40800000
>  #define MACROM_SIZE     0x00100000
> @@ -94,10 +95,14 @@
>   * CPU.
>   */
>  
> -typedef struct {
> +#define TYPE_GLUE "q800-glue"
> +OBJECT_DECLARE_SIMPLE_TYPE(GLUEState, GLUE)
> +
> +struct GLUEState {
> +    SysBusDevice parent_obj;
>      M68kCPU *cpu;
>      uint8_t ipr;
> -} GLUEState;
> +};
>  
>  static void GLUE_set_irq(void *opaque, int irq, int level)
>  {
> @@ -119,6 +124,58 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
>      m68k_set_irq_level(s->cpu, 0, 0);
>  }
>  
> +static void glue_reset(DeviceState *dev)
> +{
> +    GLUEState *s = GLUE(dev);
> +
> +    s->ipr = 0;
> +}
> +
> +static const VMStateDescription vmstate_glue = {
> +    .name = "q800-glue",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(ipr, GLUEState),
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
> +/*
> + * If the m68k CPU implemented its inbound irq lines as GPIO lines
> + * rather than via the m68k_set_irq_level() function we would not need
> + * this cpu link property and could instead provide outbound IRQ lines
> + * that the board could wire up to the CPU.
> + */
> +static Property glue_properties[] = {
> +    DEFINE_PROP_LINK("cpu", GLUEState, cpu, TYPE_M68K_CPU, M68kCPU *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void glue_init(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +
> +    qdev_init_gpio_in(dev, GLUE_set_irq, 8);
> +}
> +
> +static void glue_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &vmstate_glue;
> +    dc->reset = glue_reset;

Don't we need a realize() handler checking s->cpu is non-NULL?

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +    device_class_set_props(dc, glue_properties);
> +}
> +
> +static const TypeInfo glue_info = {
> +    .name = TYPE_GLUE,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(GLUEState),
> +    .instance_init = glue_init,
> +    .class_init = glue_class_init,
> +};
> +
>  static void main_cpu_reset(void *opaque)
>  {
>      M68kCPU *cpu = opaque;
> @@ -178,8 +235,7 @@ static void q800_init(MachineState *machine)
>      SysBusDevice *sysbus;
>      BusState *adb_bus;
>      NubusBus *nubus;
> -    GLUEState *irq;
> -    qemu_irq *pic;
> +    DeviceState *glue;
>      DriveInfo *dinfo;
>  
>      linux_boot = (kernel_filename != NULL);
> @@ -213,10 +269,9 @@ static void q800_init(MachineState *machine)
>      }
>  
>      /* IRQ Glue */
> -
> -    irq = g_new0(GLUEState, 1);
> -    irq->cpu = cpu;
> -    pic = qemu_allocate_irqs(GLUE_set_irq, irq, 8);
> +    glue = qdev_new(TYPE_GLUE);
> +    object_property_set_link(OBJECT(glue), "cpu", OBJECT(cpu), &error_abort);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(glue), &error_fatal);
>  
>      /* VIA */
>  
> @@ -228,8 +283,10 @@ static void q800_init(MachineState *machine)
>      sysbus = SYS_BUS_DEVICE(via_dev);
>      sysbus_realize_and_unref(sysbus, &error_fatal);
>      sysbus_mmio_map(sysbus, 0, VIA_BASE);
> -    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 0, pic[0]);
> -    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 1, pic[1]);
> +    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 0,
> +                                qdev_get_gpio_in(glue, 0));
> +    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 1,
> +                                qdev_get_gpio_in(glue, 1));
>  
>  
>      adb_bus = qdev_get_child_bus(via_dev, "adb.0");
> @@ -270,7 +327,7 @@ static void q800_init(MachineState *machine)
>      sysbus_realize_and_unref(sysbus, &error_fatal);
>      sysbus_mmio_map(sysbus, 0, SONIC_BASE);
>      sysbus_mmio_map(sysbus, 1, SONIC_PROM_BASE);
> -    sysbus_connect_irq(sysbus, 0, pic[2]);
> +    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 2));
>  
>      /* SCC */
>  
> @@ -292,7 +349,7 @@ static void q800_init(MachineState *machine)
>      qdev_realize_and_unref(escc_orgate, NULL, &error_fatal);
>      sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(escc_orgate, 0));
>      sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(escc_orgate, 1));
> -    qdev_connect_gpio_out(DEVICE(escc_orgate), 0, pic[3]);
> +    qdev_connect_gpio_out(DEVICE(escc_orgate), 0, qdev_get_gpio_in(glue, 3));
>      sysbus_mmio_map(sysbus, 0, SCC_BASE);
>  
>      /* SCSI */
> @@ -457,6 +514,7 @@ static const TypeInfo q800_machine_typeinfo = {
>  static void q800_machine_register_types(void)
>  {
>      type_register_static(&q800_machine_typeinfo);
> +    type_register_static(&glue_info);
>  }
>  
>  type_init(q800_machine_register_types)
> 

Re: [PATCH 2/2] hw/m68k/q800.c: Make the GLUE chip an actual QOM device

[Peter Maydell]

On Mon, 9 Nov 2020 at 14:15, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> On 11/7/20 12:51 AM, Peter Maydell wrote:
> > The handling of the GLUE (General Logic Unit) device is
> > currently open-coded. Make this into a proper QOM device.
> >
> > This minor piece of modernisation gets rid of the free
> > floating qemu_irq array 'pic', which Coverity points out
> > is technically leaked when we exit the machine init function.
> > (The replacement glue device is not leaked because it gets
> > added to the sysbus, so it's accessible via that.)
> >
> > Fixes: Coverity CID 1421883
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/m68k/q800.c | 82 ++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 70 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> > index dc13007aaf2..05bb372f958 100644
> > --- a/hw/m68k/q800.c
> > +++ b/hw/m68k/q800.c
> > @@ -47,6 +47,7 @@
> >  #include "sysemu/qtest.h"
> >  #include "sysemu/runstate.h"
> >  #include "sysemu/reset.h"
> > +#include "migration/vmstate.h"
> >
> >  #define MACROM_ADDR     0x40800000
> >  #define MACROM_SIZE     0x00100000
> > @@ -94,10 +95,14 @@
> >   * CPU.
> >   */
> >
> > -typedef struct {
> > +#define TYPE_GLUE "q800-glue"
> > +OBJECT_DECLARE_SIMPLE_TYPE(GLUEState, GLUE)
> > +
> > +struct GLUEState {
> > +    SysBusDevice parent_obj;
> >      M68kCPU *cpu;
> >      uint8_t ipr;
> > -} GLUEState;
> > +};
> >
> >  static void GLUE_set_irq(void *opaque, int irq, int level)
> >  {
> > @@ -119,6 +124,58 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
> >      m68k_set_irq_level(s->cpu, 0, 0);
> >  }
> >
> > +static void glue_reset(DeviceState *dev)
> > +{
> > +    GLUEState *s = GLUE(dev);
> > +
> > +    s->ipr = 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_glue = {
> > +    .name = "q800-glue",
> > +    .version_id = 0,
> > +    .minimum_version_id = 0,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8(ipr, GLUEState),
> > +        VMSTATE_END_OF_LIST(),
> > +    },
> > +};
> > +
> > +/*
> > + * If the m68k CPU implemented its inbound irq lines as GPIO lines
> > + * rather than via the m68k_set_irq_level() function we would not need
> > + * this cpu link property and could instead provide outbound IRQ lines
> > + * that the board could wire up to the CPU.
> > + */
> > +static Property glue_properties[] = {
> > +    DEFINE_PROP_LINK("cpu", GLUEState, cpu, TYPE_M68K_CPU, M68kCPU *),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void glue_init(Object *obj)
> > +{
> > +    DeviceState *dev = DEVICE(obj);
> > +
> > +    qdev_init_gpio_in(dev, GLUE_set_irq, 8);
> > +}
> > +
> > +static void glue_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->vmsd = &vmstate_glue;
> > +    dc->reset = glue_reset;
>
> Don't we need a realize() handler checking s->cpu is non-NULL?
>
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Doesn't seem very necessary to me -- it's a sysbus device
used for a special purpose, the one user has to wire
it up correctly, and the failure mode if they get it wrong
is pretty obvious. But I guess we could add in the explicit
error check.

thanks
-- PMM

Re: [PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input

[Mark Cave-Ayland]

On 07/11/2020 14:52, Philippe Mathieu-Daudé wrote:

> Cc'ing SPARC maintainers ...
> 
> On 11/7/20 12:51 AM, Peter Maydell wrote:
>> The q800 board code connects both of the IRQ outputs of the ESCC
>> to the same pic[3] qemu_irq. Connecting two qemu_irqs outputs directly
>> to the same input is not valid as it produces subtly wrong behaviour
>> (for instance if both the IRQ lines are high, and then one goes
>> low, the PIC input will see this as a high-to-low transition
>> even though the second IRQ line should still be holding it high).
>>
>> This kind of wiring needs an explicitly created OR gate; add one.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/m68k/q800.c  | 12 ++++++++++--
>>   hw/m68k/Kconfig |  1 +
>>   2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>> index ce4b47c3e34..dc13007aaf2 100644
>> --- a/hw/m68k/q800.c
>> +++ b/hw/m68k/q800.c
>> @@ -28,6 +28,7 @@
>>   #include "hw/hw.h"
>>   #include "hw/boards.h"
>>   #include "hw/irq.h"
>> +#include "hw/or-irq.h"
>>   #include "elf.h"
>>   #include "hw/loader.h"
>>   #include "ui/console.h"
>> @@ -171,6 +172,7 @@ static void q800_init(MachineState *machine)
>>       CPUState *cs;
>>       DeviceState *dev;
>>       DeviceState *via_dev;
>> +    DeviceState *escc_orgate;
>>       SysBusESPState *sysbus_esp;
>>       ESPState *esp;
>>       SysBusDevice *sysbus;
>> @@ -283,8 +285,14 @@ static void q800_init(MachineState *machine)
>>       qdev_prop_set_uint32(dev, "chnAtype", 0);
>>       sysbus = SYS_BUS_DEVICE(dev);
>>       sysbus_realize_and_unref(sysbus, &error_fatal);
>> -    sysbus_connect_irq(sysbus, 0, pic[3]);
>> -    sysbus_connect_irq(sysbus, 1, pic[3]);
> 
> ... because sun4m_hw_init() has the same issue:
> 
>   986     dev = qdev_new(TYPE_ESCC);
> ...
>   996     sysbus_connect_irq(s, 0, slavio_irq[14]);
>   997     sysbus_connect_irq(s, 1, slavio_irq[14]);
> ...
> 1011     sysbus_connect_irq(s, 0, slavio_irq[15]);
> 1012     sysbus_connect_irq(s, 1,  slavio_irq[15]);

Good spot. I'm fairly confident the code has been like this for a long time, so I 
don't see this is a being a critical fix for 5.2. I'll add it to my 6.0 TODO list.


ATB,

Mark.

Re: [PATCH 0/2] m68k/q800: make the GLUE chip a QOM device

[Peter Maydell]

On Fri, 6 Nov 2020 at 23:51, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This series is 6.0 material really I think.  It's a bit of cleanup
> prompted by a Coverity issue, CID 1421883.  There are another half
> dozen or so similar issues, where Coverity is complaining that we
> allocate an array of qemu_irqs with qemu_allocate_irqs() in a board
> init function -- in this case the 'pic' array in q800_init() -- and
> then we return from the board init function and the memory is leaked,
> in the sense that nobody has a pointer to it any more.
>
> The leak isn't real, in that board init functions are called only
> once, and the array of qemu_irqs really does need to stay around for
> the life of the simulation, so these are pretty much insignificant
> as Coverity issues go. But this coding style which uses a free-floating
> set of qemu_irqs is not very "modern QEMU", so the issues act as
> a nudge that we should clean the code up by encapsulating the
> interrupt-line behaviour in a QOM device. In the q800 case there
> actually is already a GLUEState struct, it just needs to be turned
> into a QOM device with GPIO input lines. Patch 2 does that.
>
> Patch 1 fixes a bug I noticed while doing this work -- it's
> not valid to connect two qemu_irq lines directly to the same
> input (here both ESCC irq lines go to pic[3]) because it produces
> weird behaviour like "both lines are asserted but the device
> consuming the interrupt sees the line deassert when one of the
> two inputs goes low, rather than only when they both go low".
> You need to put an explicit OR gate in, assuming that logical-OR
> is the desired behaviour, which it usually is.
>
> Tested only with 'make check' and 'make check-acceptance',
> but the latter does have a q800 bootup test.

Laurent, did you want to take this series as an m68k patchset,
or would you rather I just put it in via the target-arm queue?

thanks
-- PMM

Re: [PATCH 0/2] m68k/q800: make the GLUE chip a QOM device

[Laurent Vivier]

Le 07/11/2020 à 00:51, Peter Maydell a écrit :
> This series is 6.0 material really I think.  It's a bit of cleanup
> prompted by a Coverity issue, CID 1421883.  There are another half
> dozen or so similar issues, where Coverity is complaining that we
> allocate an array of qemu_irqs with qemu_allocate_irqs() in a board
> init function -- in this case the 'pic' array in q800_init() -- and
> then we return from the board init function and the memory is leaked,
> in the sense that nobody has a pointer to it any more.
> 
> The leak isn't real, in that board init functions are called only
> once, and the array of qemu_irqs really does need to stay around for
> the life of the simulation, so these are pretty much insignificant
> as Coverity issues go. But this coding style which uses a free-floating
> set of qemu_irqs is not very "modern QEMU", so the issues act as
> a nudge that we should clean the code up by encapsulating the
> interrupt-line behaviour in a QOM device. In the q800 case there
> actually is already a GLUEState struct, it just needs to be turned
> into a QOM device with GPIO input lines. Patch 2 does that.
> 
> Patch 1 fixes a bug I noticed while doing this work -- it's
> not valid to connect two qemu_irq lines directly to the same
> input (here both ESCC irq lines go to pic[3]) because it produces
> weird behaviour like "both lines are asserted but the device
> consuming the interrupt sees the line deassert when one of the
> two inputs goes low, rather than only when they both go low".
> You need to put an explicit OR gate in, assuming that logical-OR
> is the desired behaviour, which it usually is.
> 
> Tested only with 'make check' and 'make check-acceptance',
> but the latter does have a q800 bootup test.
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>   hw/m68k/q800: Don't connect two qemu_irqs directly to the same input
>   hw/m68k/q800.c: Make the GLUE chip an actual QOM device
> 
>  hw/m68k/q800.c  | 92 ++++++++++++++++++++++++++++++++++++++++++-------
>  hw/m68k/Kconfig |  1 +
>  2 files changed, 80 insertions(+), 13 deletions(-)
> 

Applied to my m68k-for-6.0 branch

Thanks,
Laurent