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)
>