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