On Mon, 9 Nov 2020 at 14:15, Philippe Mathieu-Daudé <f4...@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.mayd...@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é <f4...@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
