[Qemu-devel] [PATCH] hw/intc/armv7m: Support byte and halfword accesses to CFSR

[Qemu-devel] [PATCH] hw/intc/armv7m: Support byte and halfword accesses to CFSR

[Peter Maydell]

The Configurable Fault Status Register for ARMv7M and v8M is
supposed to be byte and halfword accessible, but we were only
implementing word accesses. Add support for the other access
sizes, which are used by the Zephyr RTOS.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reported-by: Andy Gross <andy.gross@linaro.org>
---
 hw/intc/armv7m_nvic.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 5d9c883..dc8e2f1 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -896,13 +896,6 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
             val |= (1 << 8);
         }
         return val;
-    case 0xd28: /* Configurable Fault Status.  */
-        /* The BFSR bits [15:8] are shared between security states
-         * and we store them in the NS copy
-         */
-        val = cpu->env.v7m.cfsr[attrs.secure];
-        val |= cpu->env.v7m.cfsr[M_REG_NS] & R_V7M_CFSR_BFSR_MASK;
-        return val;
     case 0xd2c: /* Hard Fault Status.  */
         return cpu->env.v7m.hfsr;
     case 0xd30: /* Debug Fault Status.  */
@@ -1280,15 +1273,6 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
         s->vectors[ARMV7M_EXCP_DEBUG].active = (value & (1 << 8)) != 0;
         nvic_irq_update(s);
         break;
-    case 0xd28: /* Configurable Fault Status.  */
-        cpu->env.v7m.cfsr[attrs.secure] &= ~value; /* W1C */
-        if (attrs.secure) {
-            /* The BFSR bits [15:8] are shared between security states
-             * and we store them in the NS copy.
-             */
-            cpu->env.v7m.cfsr[M_REG_NS] &= ~(value & R_V7M_CFSR_BFSR_MASK);
-        }
-        break;
     case 0xd2c: /* Hard Fault Status.  */
         cpu->env.v7m.hfsr &= ~value; /* W1C */
         break;
@@ -1667,6 +1651,14 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
             val = deposit32(val, i * 8, 8, get_prio(s, hdlidx, sbank));
         }
         break;
+    case 0xd28 ... 0xd2b: /* Configurable Fault Status (CFSR) */
+        /* The BFSR bits [15:8] are shared between security states
+         * and we store them in the NS copy
+         */
+        val = s->cpu->env.v7m.cfsr[attrs.secure];
+        val |= s->cpu->env.v7m.cfsr[M_REG_NS] & R_V7M_CFSR_BFSR_MASK;
+        val = extract32(val, (offset - 0xd28) * 8, size * 8);
+        break;
     case 0xfe0 ... 0xfff: /* ID.  */
         if (offset & 3) {
             val = 0;
@@ -1765,6 +1757,20 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
         }
         nvic_irq_update(s);
         return MEMTX_OK;
+    case 0xd28 ... 0xd2b: /* Configurable Fault Status (CFSR) */
+        /* All bits are W1C, so construct 32 bit value with 0s in
+         * the parts not written by the access size
+         */
+        value <<= ((offset - 0xd28) * 8);
+
+        s->cpu->env.v7m.cfsr[attrs.secure] &= ~value;
+        if (attrs.secure) {
+            /* The BFSR bits [15:8] are shared between security states
+             * and we store them in the NS copy.
+             */
+            s->cpu->env.v7m.cfsr[M_REG_NS] &= ~(value & R_V7M_CFSR_BFSR_MASK);
+        }
+        return MEMTX_OK;
     }
     if (size == 4) {
         nvic_writel(s, offset, value, attrs);
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH] hw/intc/armv7m: Support byte and halfword accesses to CFSR

[Peter Maydell]

Ping for code review?

thanks
-- PMM

On 8 December 2017 at 14:12, Peter Maydell <peter.maydell@linaro.org> wrote:
> The Configurable Fault Status Register for ARMv7M and v8M is
> supposed to be byte and halfword accessible, but we were only
> implementing word accesses. Add support for the other access
> sizes, which are used by the Zephyr RTOS.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reported-by: Andy Gross <andy.gross@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 5d9c883..dc8e2f1 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -896,13 +896,6 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
>              val |= (1 << 8);
>          }
>          return val;
> -    case 0xd28: /* Configurable Fault Status.  */
> -        /* The BFSR bits [15:8] are shared between security states
> -         * and we store them in the NS copy
> -         */
> -        val = cpu->env.v7m.cfsr[attrs.secure];
> -        val |= cpu->env.v7m.cfsr[M_REG_NS] & R_V7M_CFSR_BFSR_MASK;
> -        return val;
>      case 0xd2c: /* Hard Fault Status.  */
>          return cpu->env.v7m.hfsr;
>      case 0xd30: /* Debug Fault Status.  */
> @@ -1280,15 +1273,6 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
>          s->vectors[ARMV7M_EXCP_DEBUG].active = (value & (1 << 8)) != 0;
>          nvic_irq_update(s);
>          break;
> -    case 0xd28: /* Configurable Fault Status.  */
> -        cpu->env.v7m.cfsr[attrs.secure] &= ~value; /* W1C */
> -        if (attrs.secure) {
> -            /* The BFSR bits [15:8] are shared between security states
> -             * and we store them in the NS copy.
> -             */
> -            cpu->env.v7m.cfsr[M_REG_NS] &= ~(value & R_V7M_CFSR_BFSR_MASK);
> -        }
> -        break;
>      case 0xd2c: /* Hard Fault Status.  */
>          cpu->env.v7m.hfsr &= ~value; /* W1C */
>          break;
> @@ -1667,6 +1651,14 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
>              val = deposit32(val, i * 8, 8, get_prio(s, hdlidx, sbank));
>          }
>          break;
> +    case 0xd28 ... 0xd2b: /* Configurable Fault Status (CFSR) */
> +        /* The BFSR bits [15:8] are shared between security states
> +         * and we store them in the NS copy
> +         */
> +        val = s->cpu->env.v7m.cfsr[attrs.secure];
> +        val |= s->cpu->env.v7m.cfsr[M_REG_NS] & R_V7M_CFSR_BFSR_MASK;
> +        val = extract32(val, (offset - 0xd28) * 8, size * 8);
> +        break;
>      case 0xfe0 ... 0xfff: /* ID.  */
>          if (offset & 3) {
>              val = 0;
> @@ -1765,6 +1757,20 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
>          }
>          nvic_irq_update(s);
>          return MEMTX_OK;
> +    case 0xd28 ... 0xd2b: /* Configurable Fault Status (CFSR) */
> +        /* All bits are W1C, so construct 32 bit value with 0s in
> +         * the parts not written by the access size
> +         */
> +        value <<= ((offset - 0xd28) * 8);
> +
> +        s->cpu->env.v7m.cfsr[attrs.secure] &= ~value;
> +        if (attrs.secure) {
> +            /* The BFSR bits [15:8] are shared between security states
> +             * and we store them in the NS copy.
> +             */
> +            s->cpu->env.v7m.cfsr[M_REG_NS] &= ~(value & R_V7M_CFSR_BFSR_MASK);
> +        }
> +        return MEMTX_OK;
>      }
>      if (size == 4) {
>          nvic_writel(s, offset, value, attrs);
> --
> 2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH] hw/intc/armv7m: Support byte and halfword accesses to CFSR

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 4237 bytes --]

Hi Peter,

On 12/08/2017 11:12 AM, Peter Maydell wrote:
> The Configurable Fault Status Register for ARMv7M and v8M is
> supposed to be byte and halfword accessible, but we were only

"aligned halfword"

> implementing word accesses. Add support for the other access
> sizes, which are used by the Zephyr RTOS.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reported-by: Andy Gross <andy.gross@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 5d9c883..dc8e2f1 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -896,13 +896,6 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
>              val |= (1 << 8);
>          }
>          return val;
> -    case 0xd28: /* Configurable Fault Status.  */
> -        /* The BFSR bits [15:8] are shared between security states
> -         * and we store them in the NS copy
> -         */
> -        val = cpu->env.v7m.cfsr[attrs.secure];
> -        val |= cpu->env.v7m.cfsr[M_REG_NS] & R_V7M_CFSR_BFSR_MASK;
> -        return val;
>      case 0xd2c: /* Hard Fault Status.  */
>          return cpu->env.v7m.hfsr;
>      case 0xd30: /* Debug Fault Status.  */
> @@ -1280,15 +1273,6 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
>          s->vectors[ARMV7M_EXCP_DEBUG].active = (value & (1 << 8)) != 0;
>          nvic_irq_update(s);
>          break;
> -    case 0xd28: /* Configurable Fault Status.  */
> -        cpu->env.v7m.cfsr[attrs.secure] &= ~value; /* W1C */
> -        if (attrs.secure) {
> -            /* The BFSR bits [15:8] are shared between security states
> -             * and we store them in the NS copy.
> -             */
> -            cpu->env.v7m.cfsr[M_REG_NS] &= ~(value & R_V7M_CFSR_BFSR_MASK);
> -        }
> -        break;
>      case 0xd2c: /* Hard Fault Status.  */
>          cpu->env.v7m.hfsr &= ~value; /* W1C */
>          break;
> @@ -1667,6 +1651,14 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
>              val = deposit32(val, i * 8, 8, get_prio(s, hdlidx, sbank));
>          }
>          break;
> +    case 0xd28 ... 0xd2b: /* Configurable Fault Status (CFSR) */
> +        /* The BFSR bits [15:8] are shared between security states
> +         * and we store them in the NS copy
> +         */
> +        val = s->cpu->env.v7m.cfsr[attrs.secure];
> +        val |= s->cpu->env.v7m.cfsr[M_REG_NS] & R_V7M_CFSR_BFSR_MASK;
> +        val = extract32(val, (offset - 0xd28) * 8, size * 8);
> +        break;

we have:

static const MemoryRegionOps nvic_sysreg_ops = {
    .read_with_attrs = nvic_sysreg_read,
    .write_with_attrs = nvic_sysreg_write,
    .endianness = DEVICE_NATIVE_ENDIAN,
};

with:
    /* If true, unaligned accesses are supported.  Otherwise unaligned
     * accesses throw machine checks.
     */
    bool unaligned;

So unaligned halfword should throw excp.

We could add an explicit ".unaligned = false,"

>      case 0xfe0 ... 0xfff: /* ID.  */
>          if (offset & 3) {
>              val = 0;
> @@ -1765,6 +1757,20 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
>          }
>          nvic_irq_update(s);
>          return MEMTX_OK;
> +    case 0xd28 ... 0xd2b: /* Configurable Fault Status (CFSR) */
> +        /* All bits are W1C, so construct 32 bit value with 0s in
> +         * the parts not written by the access size
> +         */

This comment is welcomed :)

> +        value <<= ((offset - 0xd28) * 8);
> +
> +        s->cpu->env.v7m.cfsr[attrs.secure] &= ~value;
> +        if (attrs.secure) {
> +            /* The BFSR bits [15:8] are shared between security states
> +             * and we store them in the NS copy.
> +             */
> +            s->cpu->env.v7m.cfsr[M_REG_NS] &= ~(value & R_V7M_CFSR_BFSR_MASK);
> +        }
> +        return MEMTX_OK;
>      }
>      if (size == 4) {
>          nvic_writel(s, offset, value, attrs);

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-arm] [PATCH] hw/intc/armv7m: Support byte and halfword accesses to CFSR

[Peter Maydell]

On 15 January 2018 at 02:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 12/08/2017 11:12 AM, Peter Maydell wrote:
>> The Configurable Fault Status Register for ARMv7M and v8M is
>> supposed to be byte and halfword accessible, but we were only
>
> "aligned halfword"

"halfword" is the phrasing the v8M Arm ARM uses. If unaligned
is meant it's always explicitly stated.

>> implementing word accesses. Add support for the other access
>> sizes, which are used by the Zephyr RTOS.

> we have:
>
> static const MemoryRegionOps nvic_sysreg_ops = {
>     .read_with_attrs = nvic_sysreg_read,
>     .write_with_attrs = nvic_sysreg_write,
>     .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
> with:
>     /* If true, unaligned accesses are supported.  Otherwise unaligned
>      * accesses throw machine checks.
>      */
>     bool unaligned;
>
> So unaligned halfword should throw excp.
>
> We could add an explicit ".unaligned = false,"

That is the default, and assumed by practically every
MemoryRegionOps struct in QEMU...

thanks
-- PMM