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

[David Hildenbrand]

On 15.08.21 19:30, Philippe Mathieu-Daudé 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? I see the NVIC default
> region is 1 MiB wide. Aren't smaller regions returned first when
> multiple regions have same priority? This might be one of my
> misunderstandings with QEMU MR/AS APIs. Without looking at the
> code, assuming 2 MRs overlapping with the same priority, is there
> some assumption which one will be hit first?

memory_region_add_subregion() documents "The subregion may not overlap 
with other subregions", however it really just translates to adding with 
priority 0.

memory_region_add_subregion_overlap documents "The subregion may overlap 
with other subregions.  Conflicts are resolved by having a higher 
@priority hide a lower @priority. Subregions without priority are taken 
as @priority 0 ... highest priority wins"

AFAIU, overlaps with same priority (e.g., 0) have undefined behavior and 
we should really be using memory_region_add_subregion_overlap() with 
proper priorities.

> > +    }
> > +
> >       for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
> >           if (s->enable_bitband) {
> >               Object *obj = OBJECT(&s->bitband[i]);


--
Thanks,

David / dhildenb