[PATCH for-6.1 0/6] arm: Fix a handful of M-profile bugs

[PATCH for-6.1 0/6] arm: Fix a handful of M-profile bugs

[Peter Maydell]

This patchset fixes a handful of minor M-profile bugs:
 * The low 2 bits of SP should not be writeable (they are always 0)
 * Missing 'return' statements for some "tail chain to another
   exception after detecting an error during exception return" cases
 * Alignment faults were being incorrectly reported to the guest
   as MMU faults
 * ISCR.ISRPENDING wasn't being set if there was a pending
   but non-enabled interrupt
 * ISCR.VECTPENDING is 9 bits, not 8
 * ISCR.VECTPENDING was missing the new-for-v8.1M behaviour where
   it hides the identity of a pending Secure exception from a
   NonSecure read of the register

Nothing here is very critical, but they might as well go into
6.1 because they are bugfixes.

thanks
-- PMM

Peter Maydell (6):
  target/arm: Enforce that M-profile SP low 2 bits are always zero
  target/arm: Add missing 'return's after calling v7m_exception_taken()
  target/arm: Report M-profile alignment faults correctly to the guest
  hw/intc/armv7m_nvic: ISCR.ISRPENDING is set for non-enabled pending
    interrupts
  hw/intc/armv7m_nvic: Correct size of ICSR.VECTPENDING
  hw/intc/armv7m_nvic: for v8.1M VECTPENDING hides S exceptions from NS

 hw/intc/armv7m_nvic.c  | 40 ++++++++++++++++++++++++++++------------
 target/arm/gdbstub.c   |  4 ++++
 target/arm/m_helper.c  | 24 ++++++++++++++++++------
 target/arm/translate.c |  3 +++
 4 files changed, 53 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH for-6.1 1/6] target/arm: Enforce that M-profile SP low 2 bits are always zero

[Peter Maydell]

For M-profile, unlike A-profile, the low 2 bits of SP are defined to be
RES0H, which is to say that they must be hardwired to zero so that
guest attempts to write non-zero values to them are ignored.

Implement this behaviour by masking out the low bits:
 * for writes to r13 by the gdbstub
 * for writes to any of the various flavours of SP via MSR
 * for writes to r13 via store_reg() in generated code

Note that all the direct uses of cpu_R[] in translate.c are in places
where the register is definitely not r13 (usually because that has
been checked for as an UNDEFINED or UNPREDICTABLE case and handled as
UNDEF).

All the other writes to regs[13] in C code are either:
 * A-profile only code
 * writes of values we can guarantee to be aligned, such as
   - writes of previous-SP-value plus or minus a 4-aligned constant
   - writes of the value in an SP limit register (which we already
     enforce to be aligned)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is one of those changes where the code changed is clearly OK
and the review is more "did we miss anything?". Optimization of cases
in generated code where we know the value is already aligned would
obviously be possible (eg "ADD SP, SP, #8") but I haven't looked at
that; after all, it's only one extra AND insn.
---
 target/arm/gdbstub.c   |  4 ++++
 target/arm/m_helper.c  | 14 ++++++++------
 target/arm/translate.c |  3 +++
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index a8fff2a3d09..826601b3415 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -84,6 +84,10 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 
     if (n < 16) {
         /* Core integer register.  */
+        if (n == 13 && arm_feature(env, ARM_FEATURE_M)) {
+            /* M profile SP low bits are always 0 */
+            tmp &= ~3;
+        }
         env->regs[n] = tmp;
         return 4;
     }
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 7a1e35ab5b6..f9a9cb466c9 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2563,13 +2563,13 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
             if (!env->v7m.secure) {
                 return;
             }
-            env->v7m.other_ss_msp = val;
+            env->v7m.other_ss_msp = val & ~3;
             return;
         case 0x89: /* PSP_NS */
             if (!env->v7m.secure) {
                 return;
             }
-            env->v7m.other_ss_psp = val;
+            env->v7m.other_ss_psp = val & ~3;
             return;
         case 0x8a: /* MSPLIM_NS */
             if (!env->v7m.secure) {
@@ -2638,6 +2638,8 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
 
             limit = is_psp ? env->v7m.psplim[false] : env->v7m.msplim[false];
 
+            val &= ~0x3;
+
             if (val < limit) {
                 raise_exception_ra(env, EXCP_STKOF, 0, 1, GETPC());
             }
@@ -2660,16 +2662,16 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
         break;
     case 8: /* MSP */
         if (v7m_using_psp(env)) {
-            env->v7m.other_sp = val;
+            env->v7m.other_sp = val & ~3;
         } else {
-            env->regs[13] = val;
+            env->regs[13] = val & ~3;
         }
         break;
     case 9: /* PSP */
         if (v7m_using_psp(env)) {
-            env->regs[13] = val;
+            env->regs[13] = val & ~3;
         } else {
-            env->v7m.other_sp = val;
+            env->v7m.other_sp = val & ~3;
         }
         break;
     case 10: /* MSPLIM */
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 351afa43a29..80c282669f0 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -291,6 +291,9 @@ void store_reg(DisasContext *s, int reg, TCGv_i32 var)
          */
         tcg_gen_andi_i32(var, var, s->thumb ? ~1 : ~3);
         s->base.is_jmp = DISAS_JUMP;
+    } else if (reg == 13 && arm_dc_feature(s, ARM_FEATURE_M)) {
+        /* For M-profile SP bits [1:0] are always zero */
+        tcg_gen_andi_i32(var, var, ~3);
     }
     tcg_gen_mov_i32(cpu_R[reg], var);
     tcg_temp_free_i32(var);
-- 
2.20.1

Re: [PATCH for-6.1 1/6] target/arm: Enforce that M-profile SP low 2 bits are always zero

[Richard Henderson]

On 7/23/21 6:21 AM, Peter Maydell wrote:
> For M-profile, unlike A-profile, the low 2 bits of SP are defined to be
> RES0H, which is to say that they must be hardwired to zero so that
> guest attempts to write non-zero values to them are ignored.
> 
> Implement this behaviour by masking out the low bits:
>   * for writes to r13 by the gdbstub
>   * for writes to any of the various flavours of SP via MSR
>   * for writes to r13 via store_reg() in generated code
> 
> Note that all the direct uses of cpu_R[] in translate.c are in places
> where the register is definitely not r13 (usually because that has
> been checked for as an UNDEFINED or UNPREDICTABLE case and handled as
> UNDEF).
> 
> All the other writes to regs[13] in C code are either:
>   * A-profile only code
>   * writes of values we can guarantee to be aligned, such as
>     - writes of previous-SP-value plus or minus a 4-aligned constant
>     - writes of the value in an SP limit register (which we already
>       enforce to be aligned)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH for-6.1 2/6] target/arm: Add missing 'return's after calling v7m_exception_taken()

[Peter Maydell]

In do_v7m_exception_exit(), we perform various checks as part of
performing the exception return.  If one of these checks fails, the
architecture requires that we take an appropriate exception on the
existing stackframe.  We implement this by calling
v7m_exception_taken() to set up to take the new exception, and then
immediately returning from do_v7m_exception_exit() without proceeding
any further with the unstack-and-exception-return process.

In a couple of checks that are new in v8.1M, we forgot the "return"
statement, with the effect that if bad code in the guest tripped over
these checks we would set up to take a UsageFault exception but then
blunder on trying to also unstack and return from the original
exception, with the probable result that the guest would crash.

Add the missing return statements.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/m_helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index f9a9cb466c9..f352346a964 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -1554,6 +1554,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
                     qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
                         "stackframe: NSACR prevents clearing FPU registers\n");
                     v7m_exception_taken(cpu, excret, true, false);
+                    return;
                 } else if (!cpacr_pass) {
                     armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
                                             exc_secure);
@@ -1561,6 +1562,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
                     qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
                         "stackframe: CPACR prevents clearing FPU registers\n");
                     v7m_exception_taken(cpu, excret, true, false);
+                    return;
                 }
             }
             /* Clear s0..s15, FPSCR and VPR */
-- 
2.20.1

Re: [PATCH for-6.1 2/6] target/arm: Add missing 'return's after calling v7m_exception_taken()

[Richard Henderson]

On 7/23/21 6:21 AM, Peter Maydell wrote:
> In do_v7m_exception_exit(), we perform various checks as part of
> performing the exception return.  If one of these checks fails, the
> architecture requires that we take an appropriate exception on the
> existing stackframe.  We implement this by calling
> v7m_exception_taken() to set up to take the new exception, and then
> immediately returning from do_v7m_exception_exit() without proceeding
> any further with the unstack-and-exception-return process.
> 
> In a couple of checks that are new in v8.1M, we forgot the "return"
> statement, with the effect that if bad code in the guest tripped over
> these checks we would set up to take a UsageFault exception but then
> blunder on trying to also unstack and return from the original
> exception, with the probable result that the guest would crash.
> 
> Add the missing return statements.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/m_helper.c | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH for-6.1 3/6] target/arm: Report M-profile alignment faults correctly to the guest

[Peter Maydell]

For M-profile, we weren't reporting alignment faults triggered by the
generic TCG code correctly to the guest.  These get passed into
arm_v7m_cpu_do_interrupt() as an EXCP_DATA_ABORT with an A-profile
style exception.fsr value of 1.  We didn't check for this, and so
they fell through into the default of "assume this is an MPU fault"
and were reported to the guest as a data access violation MPU fault.

Report these alignment faults as UsageFaults which set the UNALIGNED
bit in the UFSR.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The other approach would be to have arm_cpu_do_unaligned_access()
raise the EXCP_UNALIGNED which we already use for Unaligned
UsageFaults which are raised by m-profile specific helper code,
but I think this way is in line with the current design that
generally prefers to report exception information in an A-profile
format and then re-arrange that into the M-profile information
in arm_v7m_cpu_do_interrupt().
---
 target/arm/m_helper.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index f352346a964..20761c94877 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2248,6 +2248,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         env->v7m.sfsr |= R_V7M_SFSR_LSERR_MASK;
         break;
     case EXCP_UNALIGNED:
+        /* Unaligned faults reported by M-profile aware code */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
         env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_UNALIGNED_MASK;
         break;
@@ -2320,6 +2321,13 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
             }
             armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_BUS, false);
             break;
+        case 0x1: /* Alignment fault reported by generic code */
+            qemu_log_mask(CPU_LOG_INT,
+                          "...really UsageFault with UFSR.UNALIGNED\n");
+            env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_UNALIGNED_MASK;
+            armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
+                                    env->v7m.secure);
+            break;
         default:
             /*
              * All other FSR values are either MPU faults or "can't happen
-- 
2.20.1

Re: [PATCH for-6.1 3/6] target/arm: Report M-profile alignment faults correctly to the guest

[Richard Henderson]

On 7/23/21 6:21 AM, Peter Maydell wrote:
> For M-profile, we weren't reporting alignment faults triggered by the
> generic TCG code correctly to the guest.  These get passed into
> arm_v7m_cpu_do_interrupt() as an EXCP_DATA_ABORT with an A-profile
> style exception.fsr value of 1.  We didn't check for this, and so
> they fell through into the default of "assume this is an MPU fault"
> and were reported to the guest as a data access violation MPU fault.
> 
> Report these alignment faults as UsageFaults which set the UNALIGNED
> bit in the UFSR.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> The other approach would be to have arm_cpu_do_unaligned_access()
> raise the EXCP_UNALIGNED which we already use for Unaligned
> UsageFaults which are raised by m-profile specific helper code,
> but I think this way is in line with the current design that
> generally prefers to report exception information in an A-profile
> format and then re-arrange that into the M-profile information
> in arm_v7m_cpu_do_interrupt().
> ---
>   target/arm/m_helper.c | 8 ++++++++
>   1 file changed, 8 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH for-6.1 4/6] hw/intc/armv7m_nvic: ISCR.ISRPENDING is set for non-enabled pending interrupts

[Peter Maydell]

The ISCR.ISRPENDING bit is set when an external interrupt is pending.
This is true whether that external interrupt is enabled or not.
This means that we can't use 's->vectpending == 0' as a shortcut to
"ISRPENDING is zero", because s->vectpending indicates only the
highest priority pending enabled interrupt.

Remove the incorrect optimization so that if there is no pending
enabled interrupt we fall through to scanning through the whole
interrupt array.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 94fe00235af..2aba2136822 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -127,15 +127,14 @@ static bool nvic_isrpending(NVICState *s)
 {
     int irq;
 
-    /* We can shortcut if the highest priority pending interrupt
-     * happens to be external or if there is nothing pending.
+    /*
+     * We can shortcut if the highest priority pending interrupt
+     * happens to be external; if not we need to check the whole
+     * vectors[] array.
      */
     if (s->vectpending > NVIC_FIRST_IRQ) {
         return true;
     }
-    if (s->vectpending == 0) {
-        return false;
-    }
 
     for (irq = NVIC_FIRST_IRQ; irq < s->num_irq; irq++) {
         if (s->vectors[irq].pending) {
-- 
2.20.1

Re: [PATCH for-6.1 4/6] hw/intc/armv7m_nvic: ISCR.ISRPENDING is set for non-enabled pending interrupts

[Richard Henderson]

On 7/23/21 6:21 AM, Peter Maydell wrote:
> The ISCR.ISRPENDING bit is set when an external interrupt is pending.
> This is true whether that external interrupt is enabled or not.
> This means that we can't use 's->vectpending == 0' as a shortcut to
> "ISRPENDING is zero", because s->vectpending indicates only the
> highest priority pending enabled interrupt.
> 
> Remove the incorrect optimization so that if there is no pending
> enabled interrupt we fall through to scanning through the whole
> interrupt array.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/armv7m_nvic.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH for-6.1 5/6] hw/intc/armv7m_nvic: Correct size of ICSR.VECTPENDING

[Peter Maydell]

The VECTPENDING field in the ICSR is 9 bits wide, in bits [20:12] of
the register.  We were incorrectly masking it to 8 bits, so it would
report the wrong value if the pending exception was greater than 256.
Fix the bug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 2aba2136822..c9149a3b221 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -1039,7 +1039,7 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
         /* VECTACTIVE */
         val = cpu->env.v7m.exception;
         /* VECTPENDING */
-        val |= (s->vectpending & 0xff) << 12;
+        val |= (s->vectpending & 0x1ff) << 12;
         /* ISRPENDING - set if any external IRQ is pending */
         if (nvic_isrpending(s)) {
             val |= (1 << 22);
-- 
2.20.1

Re: [PATCH for-6.1 5/6] hw/intc/armv7m_nvic: Correct size of ICSR.VECTPENDING

[Richard Henderson]

On 7/23/21 6:21 AM, Peter Maydell wrote:
> The VECTPENDING field in the ICSR is 9 bits wide, in bits [20:12] of
> the register.  We were incorrectly masking it to 8 bits, so it would
> report the wrong value if the pending exception was greater than 256.
> Fix the bug.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/armv7m_nvic.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH for-6.1 6/6] hw/intc/armv7m_nvic: for v8.1M VECTPENDING hides S exceptions from NS

[Peter Maydell]

In Arm v8.1M the VECTPENDING field in the ICSR has new behaviour: if
the register is accessed NonSecure and the highest priority pending
enabled exception (that would be returned in the VECTPENDING field)
targets Secure, then the VECTPENDING field must read 1 rather than
the exception number of the pending exception. Implement this.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index c9149a3b221..1e7ddcb94cb 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -804,6 +804,16 @@ void armv7m_nvic_acknowledge_irq(void *opaque)
     nvic_irq_update(s);
 }
 
+static bool vectpending_targets_secure(NVICState *s)
+{
+    /* Return true if s->vectpending targets Secure state */
+    if (s->vectpending_is_s_banked) {
+        return true;
+    }
+    return !exc_is_banked(s->vectpending) &&
+        exc_targets_secure(s, s->vectpending);
+}
+
 void armv7m_nvic_get_pending_irq_info(void *opaque,
                                       int *pirq, bool *ptargets_secure)
 {
@@ -813,12 +823,7 @@ void armv7m_nvic_get_pending_irq_info(void *opaque,
 
     assert(pending > ARMV7M_EXCP_RESET && pending < s->num_irq);
 
-    if (s->vectpending_is_s_banked) {
-        targets_secure = true;
-    } else {
-        targets_secure = !exc_is_banked(pending) &&
-            exc_targets_secure(s, pending);
-    }
+    targets_secure = vectpending_targets_secure(s);
 
     trace_nvic_get_pending_irq_info(pending, targets_secure);
 
@@ -1039,7 +1044,19 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
         /* VECTACTIVE */
         val = cpu->env.v7m.exception;
         /* VECTPENDING */
-        val |= (s->vectpending & 0x1ff) << 12;
+        if (s->vectpending) {
+            /*
+             * From v8.1M VECTPENDING must read as 1 if accessed as
+             * NonSecure and the highest priority pending and enabled
+             * exception targets Secure.
+             */
+            int vp = s->vectpending;
+            if (!attrs.secure && arm_feature(&cpu->env, ARM_FEATURE_V8_1M) &&
+                vectpending_targets_secure(s)) {
+                vp = 1;
+            }
+            val |= (vp & 0x1ff) << 12;
+        }
         /* ISRPENDING - set if any external IRQ is pending */
         if (nvic_isrpending(s)) {
             val |= (1 << 22);
-- 
2.20.1

Re: [PATCH for-6.1 6/6] hw/intc/armv7m_nvic: for v8.1M VECTPENDING hides S exceptions from NS

[Richard Henderson]

On 7/23/21 6:21 AM, Peter Maydell wrote:
> In Arm v8.1M the VECTPENDING field in the ICSR has new behaviour: if
> the register is accessed NonSecure and the highest priority pending
> enabled exception (that would be returned in the VECTPENDING field)
> targets Secure, then the VECTPENDING field must read 1 rather than
> the exception number of the pending exception. Implement this.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/armv7m_nvic.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~