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

[Damien Hedde]

On 8/17/21 10:25 AM, Luc Michel wrote:
> On 10:33 Thu 12 Aug     , 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>
> 
> Reviewed-by: Luc Michel <luc@lmichel.fr>

Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
> 
>> ---
>>  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.
>> + */
>> +
>> +/*
>> + * This is a model of the RAS register block of an M-profile CPU
>> + * (the registers starting at 0xE0005000 with ERRFRn).
>> + *
>> + * QEMU interface:
>> + *  + sysbus MMIO region 0: the register bank
>> + *
>> + * The QEMU implementation currently provides "minimal RAS" only.
>> + */
>> +
>> +#ifndef HW_MISC_ARMV7M_RAS_H
>> +#define HW_MISC_ARMV7M_RAS_H
>> +
>> +#include "hw/sysbus.h"
>> +
>> +#define TYPE_ARMV7M_RAS "armv7m-ras"
>> +OBJECT_DECLARE_SIMPLE_TYPE(ARMv7MRAS, ARMV7M_RAS)
>> +
>> +struct ARMv7MRAS {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +
>> +    /*< public >*/
>> +    MemoryRegion iomem;
>> +};
>> +
>> +#endif
>> 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);
>> +    }
>> +
>>      for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
>>          if (s->enable_bitband) {
>>              Object *obj = OBJECT(&s->bitband[i]);
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index 1e7ddcb94cb..a5975592dfa 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -2549,56 +2549,6 @@ static const MemoryRegionOps nvic_systick_ops = {
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>  };
>>  
>> -
>> -static MemTxResult ras_read(void *opaque, hwaddr addr,
>> -                            uint64_t *data, unsigned size,
>> -                            MemTxAttrs attrs)
>> -{
>> -    if (attrs.user) {
>> -        return MEMTX_ERROR;
>> -    }
>> -
>> -    switch (addr) {
>> -    case 0xe10: /* ERRIIDR */
>> -        /* architect field = Arm; product/variant/revision 0 */
>> -        *data = 0x43b;
>> -        break;
>> -    case 0xfc8: /* ERRDEVID */
>> -        /* Minimal RAS: we implement 0 error record indexes */
>> -        *data = 0;
>> -        break;
>> -    default:
>> -        qemu_log_mask(LOG_UNIMP, "Read RAS register offset 0x%x\n",
>> -                      (uint32_t)addr);
>> -        *data = 0;
>> -        break;
>> -    }
>> -    return MEMTX_OK;
>> -}
>> -
>> -static MemTxResult ras_write(void *opaque, hwaddr addr,
>> -                             uint64_t value, unsigned size,
>> -                             MemTxAttrs attrs)
>> -{
>> -    if (attrs.user) {
>> -        return MEMTX_ERROR;
>> -    }
>> -
>> -    switch (addr) {
>> -    default:
>> -        qemu_log_mask(LOG_UNIMP, "Write to RAS register offset 0x%x\n",
>> -                      (uint32_t)addr);
>> -        break;
>> -    }
>> -    return MEMTX_OK;
>> -}
>> -
>> -static const MemoryRegionOps ras_ops = {
>> -    .read_with_attrs = ras_read,
>> -    .write_with_attrs = ras_write,
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> -};
>> -
>>  /*
>>   * Unassigned portions of the PPB space are RAZ/WI for privileged
>>   * accesses, and fault for non-privileged accesses.
>> @@ -2946,12 +2896,6 @@ static void armv7m_nvic_realize(DeviceState *dev, 
>> Error **errp)
>>                                              &s->systick_ns_mem, 1);
>>      }
>>  
>> -    if (cpu_isar_feature(aa32_ras, s->cpu)) {
>> -        memory_region_init_io(&s->ras_mem, OBJECT(s),
>> -                              &ras_ops, s, "nvic_ras", 0x1000);
>> -        memory_region_add_subregion(&s->container, 0x5000, &s->ras_mem);
>> -    }
>> -
>>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container);
>>  }
>>  
>> diff --git a/hw/misc/armv7m_ras.c b/hw/misc/armv7m_ras.c
>> new file mode 100644
>> index 00000000000..a2b4f4b8dc8
>> --- /dev/null
>> +++ b/hw/misc/armv7m_ras.c
>> @@ -0,0 +1,93 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/misc/armv7m_ras.h"
>> +#include "qemu/log.h"
>> +
>> +static MemTxResult ras_read(void *opaque, hwaddr addr,
>> +                            uint64_t *data, unsigned size,
>> +                            MemTxAttrs attrs)
>> +{
>> +    if (attrs.user) {
>> +        return MEMTX_ERROR;
>> +    }
>> +
>> +    switch (addr) {
>> +    case 0xe10: /* ERRIIDR */
>> +        /* architect field = Arm; product/variant/revision 0 */
>> +        *data = 0x43b;
>> +        break;
>> +    case 0xfc8: /* ERRDEVID */
>> +        /* Minimal RAS: we implement 0 error record indexes */
>> +        *data = 0;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "Read RAS register offset 0x%x\n",
>> +                      (uint32_t)addr);
>> +        *data = 0;
>> +        break;
>> +    }
>> +    return MEMTX_OK;
>> +}
>> +
>> +static MemTxResult ras_write(void *opaque, hwaddr addr,
>> +                             uint64_t value, unsigned size,
>> +                             MemTxAttrs attrs)
>> +{
>> +    if (attrs.user) {
>> +        return MEMTX_ERROR;
>> +    }
>> +
>> +    switch (addr) {
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "Write to RAS register offset 0x%x\n",
>> +                      (uint32_t)addr);
>> +        break;
>> +    }
>> +    return MEMTX_OK;
>> +}
>> +
>> +static const MemoryRegionOps ras_ops = {
>> +    .read_with_attrs = ras_read,
>> +    .write_with_attrs = ras_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +
>> +static void armv7m_ras_init(Object *obj)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +    ARMv7MRAS *s = ARMV7M_RAS(obj);
>> +
>> +    memory_region_init_io(&s->iomem, obj, &ras_ops,
>> +                          s, "armv7m-ras", 0x1000);
>> +    sysbus_init_mmio(sbd, &s->iomem);
>> +}
>> +
>> +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,
>> +};
>> +
>> +static void armv7m_ras_register_types(void)
>> +{
>> +    type_register_static(&armv7m_ras_info);
>> +}
>> +
>> +type_init(armv7m_ras_register_types);
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 37b1a8e4428..3cac393bb48 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -617,6 +617,7 @@ F: hw/intc/gic_internal.h
>>  F: hw/misc/a9scu.c
>>  F: hw/misc/arm11scu.c
>>  F: hw/misc/arm_l2x0.c
>> +F: hw/misc/armv7m_ras.c
>>  F: hw/timer/a9gtimer*
>>  F: hw/timer/arm*
>>  F: include/hw/arm/arm*.h
>> @@ -626,6 +627,7 @@ F: include/hw/misc/arm11scu.h
>>  F: include/hw/timer/a9gtimer.h
>>  F: include/hw/timer/arm_mptimer.h
>>  F: include/hw/timer/armv7m_systick.h
>> +F: include/hw/misc/armv7m_ras.h
>>  F: tests/qtest/test-arm-mptimer.c
>>  
>>  Exynos
>> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
>> index a53b849a5a0..3f41a3a5b27 100644
>> --- a/hw/misc/meson.build
>> +++ b/hw/misc/meson.build
>> @@ -17,6 +17,8 @@ softmmu_ss.add(when: 'CONFIG_INTEGRATOR_DEBUG', if_true: 
>> files('arm_integrator_d
>>  softmmu_ss.add(when: 'CONFIG_A9SCU', if_true: files('a9scu.c'))
>>  softmmu_ss.add(when: 'CONFIG_ARM11SCU', if_true: files('arm11scu.c'))
>>  
>> +softmmu_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('armv7m_ras.c'))
>> +
>>  # Mac devices
>>  softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
>>  
>> -- 
>> 2.20.1
>>
>