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

Peter Maydell posted 25 patches 4 years, 6 months ago
Maintainers: Alistair Francis <alistair@alistair23.me>, Peter Maydell <peter.maydell@linaro.org>, Alexandre Iooss <erdnaxe@crans.org>, Subbaraya Sundeep <sundeep.lkml@gmail.com>, Luc Michel <luc@lmichel.fr>, Damien Hedde <damien.hedde@greensocs.com>, Joel Stanley <joel@jms.id.au>
[PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
Posted by Peter Maydell 4 years, 6 months ago
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.
+ */
+
+/*
+ * 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


Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
Posted by Alexandre IOOSS 4 years, 6 months ago

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?

> +
> +/*
> + * 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,
> +};

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?

> +
> +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'))
>   
> 

Looks good to me! My review is weak because I am still new to QEMU codebase.

Reviewed-by: Alexandre Iooss <erdnaxe@crans.org>

-- Alexandre

Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
Posted by Peter Maydell 4 years, 6 months ago
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

Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
Posted by Alistair Francis 4 years, 6 months ago
On Thu, Aug 12, 2021 at 7:34 PM Peter Maydell <peter.maydell@linaro.org> 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: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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
>
>

Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
Posted by Philippe Mathieu-Daudé 4 years, 5 months ago
+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?

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

Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
Posted by David Hildenbrand 4 years, 5 months ago
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


Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
Posted by Peter Maydell 4 years, 5 months ago
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

Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
Posted by Luc Michel 4 years, 5 months ago
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>

> ---
>  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
> 

-- 

Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
Posted by Damien Hedde 4 years, 5 months ago

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
>>
>