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

[Peter Maydell <peter.maydell@linaro.org>]

On Sun, 15 Aug 2021 at 18:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> +Peter/David
>
> 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/hw/arm/armv7m.c b/hw/arm/armv7m.c
> > index 9ce5c30cd5c..8964730d153 100644
> > --- a/hw/arm/armv7m.c
> > +++ b/hw/arm/armv7m.c
> > @@ -231,6 +231,18 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
> >      memory_region_add_subregion(&s->container, 0xe0000000,
> >                                  sysbus_mmio_get_region(sbd, 0));
> >
> > +    /* If the CPU has RAS support, create the RAS register block */
> > +    if (cpu_isar_feature(aa32_ras, s->cpu)) {
> > +        object_initialize_child(OBJECT(dev), "armv7m-ras",
> > +                                &s->ras, TYPE_ARMV7M_RAS);
> > +        sbd = SYS_BUS_DEVICE(&s->ras);
> > +        if (!sysbus_realize(sbd, errp)) {
> > +            return;
> > +        }
> > +        memory_region_add_subregion_overlap(&s->container, 0xe0005000,
> > +                                            sysbus_mmio_get_region(sbd, 0), 1);
>
> Just curious, is the overlap really needed?

Yes, because this block is currently in the middle of the
PPB-area region provided by the NVIC, and needs to take priority
over it. Once the refactoring is complete, the background-region
will also be created in this armv7m realize function, but the
RAS block still needs to go above it.

> I see the NVIC default
> region is 1 MiB wide. Aren't smaller regions returned first when
> multiple regions have same priority?

As David says, if you don't specify the priority then it's
pot-luck which one you see. Having overlaps and not setting
priorities is a QEMU bug. (We used to have some code to print
a warning about unintentionally overlapping regions, but it was
always disabled with #if 0 and we eventually deleted it in commit
b61359781958. IIRC the reason we never enabled either a warning
or an assertion was because for the PC machine's PCI devices
in particular we thought it might be possible for the guest to
map PCI devices at a silly address and generate overlaps, but
I may well have the details wrong as it was years back.)

-- PMM