Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
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
>
> Reviewed-by: Luc Michel
Reviewed-by: Damien Hedde
>
>> ---
>> 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 000..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(>container, 0xe000,
>> 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",
>> +>ras, TYPE_ARMV7M_RAS);
>> +sbd = SYS_BUS_DEVICE(>ras);
>> +if (!sysbus_realize(sbd, errp)) {
>> +return;
>> +}
>> +memory_region_add_subregion_overlap(>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(>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 */
>> -
Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
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
Reviewed-by: Luc Michel
> ---
> 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 000..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(>container, 0xe000,
> 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",
> +>ras, TYPE_ARMV7M_RAS);
> +sbd = SYS_BUS_DEVICE(>ras);
> +if (!sysbus_realize(sbd, errp)) {
> +return;
> +}
> +memory_region_add_subregion_overlap(>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(>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 */
> -
Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
On Sun, 15 Aug 2021 at 18: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
> > ---
> > 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(>container, 0xe000,
> > 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",
> > +>ras, TYPE_ARMV7M_RAS);
> > +sbd = SYS_BUS_DEVICE(>ras);
> > +if (!sysbus_realize(sbd, errp)) {
> > +return;
> > +}
> > +memory_region_add_subregion_overlap(>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
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
---
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(>container, 0xe000,
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",
+>ras, TYPE_ARMV7M_RAS);
+sbd = SYS_BUS_DEVICE(>ras);
+if (!sysbus_realize(sbd, errp)) {
+return;
+}
+memory_region_add_subregion_overlap(>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(>bitband[i]);
--
Thanks,
David / dhildenb
Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
+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
> ---
> 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(>container, 0xe000,
> 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",
> +>ras, TYPE_ARMV7M_RAS);
> +sbd = SYS_BUS_DEVICE(>ras);
> +if (!sysbus_realize(sbd, errp)) {
> +return;
> +}
> +memory_region_add_subregion_overlap(>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(>bitband[i]);
Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
On Thu, Aug 12, 2021 at 7:34 PM 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
Reviewed-by: Alistair Francis
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 000..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(>container, 0xe000,
> 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",
> +>ras, TYPE_ARMV7M_RAS);
> +sbd = SYS_BUS_DEVICE(>ras);
> +if (!sysbus_realize(sbd, errp)) {
> +return;
> +}
> +memory_region_add_subregion_overlap(>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(>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 */
>
Re: [PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
On Thu, 12 Aug 2021 at 12:08, Alexandre IOOSS 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
> > ---
> > 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 000..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
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
---
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 000..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(>container, 0xe000,
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",
+>ras, TYPE_ARMV7M_RAS);
+sbd = SYS_BUS_DEVICE(>ras);
+if (!sysbus_realize(sbd, errp)) {
+return;
+}
+memory_region_add_subregion_overlap(>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(>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
[PATCH for-6.2 01/25] arm: Move M-profile RAS register block into its own device
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
---
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 000..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(>container, 0xe000,
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",
+>ras, TYPE_ARMV7M_RAS);
+sbd = SYS_BUS_DEVICE(>ras);
+if (!sysbus_realize(sbd, errp)) {
+return;
+}
+memory_region_add_subregion_overlap(>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(>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,
-
