Re: [PATCH v1 1/2] hw/misc: versal: Add a model of the XRAM controller

[Edgar E. Iglesias]

On Mon, Mar 08, 2021 at 04:54:26PM +0000, Peter Maydell wrote:
> On Tue, 2 Mar 2021 at 11:09, Edgar E. Iglesias <edgar.iglesias@gmail.com> 
> wrote:
> >
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Add a model of the Xilinx Versal Accelerator RAM (XRAM).
> > This is mainly a stub to make firmware happy. The size of
> > the RAMs can be probed. The interrupt mask logic is
> > modelled but none of the interrups will ever be raised
> > unless injected.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  include/hw/misc/xlnx-versal-xramc.h | 102 +++++++++++
> >  hw/misc/xlnx-versal-xramc.c         | 253 ++++++++++++++++++++++++++++
> >  hw/misc/meson.build                 |   1 +
> >  3 files changed, 356 insertions(+)
> >  create mode 100644 include/hw/misc/xlnx-versal-xramc.h
> >  create mode 100644 hw/misc/xlnx-versal-xramc.c
> >
> > diff --git a/include/hw/misc/xlnx-versal-xramc.h 
> > b/include/hw/misc/xlnx-versal-xramc.h
> > new file mode 100644
> > index 0000000000..68163cf330
> > --- /dev/null
> > +++ b/include/hw/misc/xlnx-versal-xramc.h
> > @@ -0,0 +1,102 @@
> > +/*
> > + * QEMU model of the Xilinx XRAM Controller.
> > + *
> > + * Copyright (c) 2021 Xilinx Inc.
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > + */
> > +
> > +#ifndef XLNX_VERSAL_XRAMC_H
> > +#define XLNX_VERSAL_XRAMC_H
> > +
> > +#include "qemu/osdep.h"
> 
> Headers must never include osdep.h.
> 
> > +#include "hw/sysbus.h"
> > +#include "hw/register.h"
> > +#include "qemu/bitops.h"
> > +#include "qemu/log.h"
> > +#include "migration/vmstate.h"
> > +#include "hw/irq.h"
> 
> I bet you don't really need all of these includes in the header file;
> some of them belong in the .c file.

Yep, I'll cleanup the header files in v2.


> 
> > +static void xram_ctrl_init(Object *obj)
> > +{
> > +    XlnxXramCtrl *s = XLNX_XRAM_CTRL(obj);
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    RegisterInfoArray *reg_array;
> > +
> > +    memory_region_init(&s->iomem, obj, TYPE_XLNX_XRAM_CTRL,
> > +                       XRAM_CTRL_R_MAX * 4);
> > +    reg_array =
> > +        register_init_block32(DEVICE(obj), xram_ctrl_regs_info,
> > +                              ARRAY_SIZE(xram_ctrl_regs_info),
> > +                              s->regs_info, s->regs,
> > +                              &xram_ctrl_ops,
> > +                              XLNX_XRAM_CTRL_ERR_DEBUG,
> > +                              XRAM_CTRL_R_MAX * 4);
> > +    memory_region_add_subregion(&s->iomem,
> > +                                0x0,
> > +                                &reg_array->mem);
> 
> Isn't this just creating a container region that contains
> exactly one subregion that is the same size as it? Do we
> need to do this so that the reg_array is disposed of via
> refcounting or something ?

TBH I was just copying a pattern here. It looks to me like if
reg_array gets leaked and we're using an unnecesarry container MR.
I'll fix this in v2 (if I understood the life-cycle
of these regs correctly).

Cheers,
Edgar