Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device

[Peter Maydell]

On Thu, 12 Aug 2021 at 12:08, Alexandre IOOSS <erdnaxe@crans.org> wrote:
>
>
>
> On 8/12/21 11:33 AM, Peter Maydell wrote:
> > Currently we implement the RAS register block within the NVIC device.
> > It isn't really very tightly coupled with the NVIC proper, so instead
> > move it out into a sysbus device of its own and have the top level
> > ARMv7M container create it and map it into memory at the right
> > address.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   include/hw/arm/armv7m.h       |  2 +
> >   include/hw/intc/armv7m_nvic.h |  1 -
> >   include/hw/misc/armv7m_ras.h  | 37 ++++++++++++++
> >   hw/arm/armv7m.c               | 12 +++++
> >   hw/intc/armv7m_nvic.c         | 56 ---------------------
> >   hw/misc/armv7m_ras.c          | 93 +++++++++++++++++++++++++++++++++++
> >   MAINTAINERS                   |  2 +
> >   hw/misc/meson.build           |  2 +
> >   8 files changed, 148 insertions(+), 57 deletions(-)
> >   create mode 100644 include/hw/misc/armv7m_ras.h
> >   create mode 100644 hw/misc/armv7m_ras.c
> >
> > diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
> > index bc6733c5184..4cae0d7eeaa 100644
> > --- a/include/hw/arm/armv7m.h
> > +++ b/include/hw/arm/armv7m.h
> > @@ -12,6 +12,7 @@
> >
> >   #include "hw/sysbus.h"
> >   #include "hw/intc/armv7m_nvic.h"
> > +#include "hw/misc/armv7m_ras.h"
> >   #include "target/arm/idau.h"
> >   #include "qom/object.h"
> >
> > @@ -58,6 +59,7 @@ struct ARMv7MState {
> >       NVICState nvic;
> >       BitBandState bitband[ARMV7M_NUM_BITBANDS];
> >       ARMCPU *cpu;
> > +    ARMv7MRAS ras;
> >
> >       /* MemoryRegion we pass to the CPU, with our devices layered on
> >        * top of the ones the board provides in board_memory.
> > diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h
> > index 39c71e15936..33b6d8810c7 100644
> > --- a/include/hw/intc/armv7m_nvic.h
> > +++ b/include/hw/intc/armv7m_nvic.h
> > @@ -83,7 +83,6 @@ struct NVICState {
> >       MemoryRegion sysreg_ns_mem;
> >       MemoryRegion systickmem;
> >       MemoryRegion systick_ns_mem;
> > -    MemoryRegion ras_mem;
> >       MemoryRegion container;
> >       MemoryRegion defaultmem;
> >
> > diff --git a/include/hw/misc/armv7m_ras.h b/include/hw/misc/armv7m_ras.h
> > new file mode 100644
> > index 00000000000..f8773e65b14
> > --- /dev/null
> > +++ b/include/hw/misc/armv7m_ras.h
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Arm M-profile RAS block
> > + *
> > + * Copyright (c) 2021 Linaro Limited
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License version 2 or
> > + *  (at your option) any later version.
> > + */
>
> Maybe it would be a good idea here to change to "Arm M-profile RAS
> (Reliability, Availability, and Serviceability) block" to define the
> acronym at least once in the device code?

Yeah, we could perhaps put a comment in there somewhere.
Though really you need to look in the spec to understand
what the block is doing, at which point you'll find out what
the register block is for.

> > +static void armv7m_ras_class_init(ObjectClass *klass, void *data)
> > +{
> > +    /* This device has no state: no need for vmstate or reset */
> > +}
> > +
> > +static const TypeInfo armv7m_ras_info = {
> > +    .name = TYPE_ARMV7M_RAS,
> > +    .parent = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(ARMv7MRAS),
> > +    .instance_init = armv7m_ras_init,
> > +    .class_init = armv7m_ras_class_init,
> > +};
>
> Pure curiosity: is `.class_init` defined because it needs to be defined
> or is it only a good practise in QEMU code to always define it?

It's optional, and in this case it's only there because it makes
a place to put the comment, I guess.

-- PMM