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.igles...@gmail.com> > wrote: > > > > From: "Edgar E. Iglesias" <edgar.igles...@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.igles...@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.igles...@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, > > + ®_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