[PATCH for-4.2] target/arm: Support EL0 v7m msr/mrs for CONFIG_USER_ONLY

Richard Henderson posted 1 patch 4 years, 4 months ago
Test asan passed
Test checkpatch passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191118194916.3670-1-richard.henderson@linaro.org
Maintainers: Peter Maydell <peter.maydell@linaro.org>
target/arm/cpu.h      |   2 +
target/arm/m_helper.c | 110 ++++++++++++++++++++++++++----------------
2 files changed, 70 insertions(+), 42 deletions(-)
[PATCH for-4.2] target/arm: Support EL0 v7m msr/mrs for CONFIG_USER_ONLY
Posted by Richard Henderson 4 years, 4 months ago
Simply moving the non-stub helper_v7m_mrs/msr outside of
!CONFIG_USER_ONLY is not an option, because of all of the
other system-mode helpers that are called.

But we can split out a few subroutines to handle the few
EL0 accessible registers without duplicating code.

Reported-by: Christophe Lyon <christophe.lyon@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h      |   2 +
 target/arm/m_helper.c | 110 ++++++++++++++++++++++++++----------------
 2 files changed, 70 insertions(+), 42 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e1a66a2d1c..81f5b5b75f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1317,6 +1317,7 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
     if (mask & XPSR_GE) {
         env->GE = (val & XPSR_GE) >> 16;
     }
+#ifndef CONFIG_USER_ONLY
     if (mask & XPSR_T) {
         env->thumb = ((val & XPSR_T) != 0);
     }
@@ -1332,6 +1333,7 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
         /* Note that this only happens on exception exit */
         write_v7m_exception(env, val & XPSR_EXCP);
     }
+#endif
 }
 
 #define HCR_VM        (1ULL << 0)
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index f2512e448e..7c7e8eb62c 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -33,22 +33,80 @@
 #include "exec/cpu_ldst.h"
 #endif
 
+static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask,
+                         uint32_t reg, uint32_t val)
+{
+    /* Only APSR is actually writable */
+    if (!(reg & 4)) {
+        uint32_t apsrmask = 0;
+
+        if (mask & 8) {
+            apsrmask |= XPSR_NZCV | XPSR_Q;
+        }
+        if ((mask & 4) && arm_feature(env, ARM_FEATURE_THUMB_DSP)) {
+            apsrmask |= XPSR_GE;
+        }
+        xpsr_write(env, val, apsrmask);
+    }
+}
+
+static uint32_t v7m_mrs_xpsr(CPUARMState *env, uint32_t reg, unsigned el)
+{
+    uint32_t mask = 0;
+
+    if ((reg & 1) && el) {
+        mask |= XPSR_EXCP; /* IPSR (unpriv. reads as zero) */
+    }
+    if (!(reg & 4)) {
+        mask |= XPSR_NZCV | XPSR_Q; /* APSR */
+        if (arm_feature(env, ARM_FEATURE_THUMB_DSP)) {
+            mask |= XPSR_GE;
+        }
+    }
+    /* EPSR reads as zero */
+    return xpsr_read(env) & mask;
+}
+
+static uint32_t v7m_mrs_control(CPUARMState *env, uint32_t secure)
+{
+    uint32_t value = env->v7m.control[secure];
+
+    if (!secure) {
+        /* SFPA is RAZ/WI from NS; FPCA is stored in the M_REG_S bank */
+        value |= env->v7m.control[M_REG_S] & R_V7M_CONTROL_FPCA_MASK;
+    }
+    return value;
+}
+
 #ifdef CONFIG_USER_ONLY
 
 /* These should probably raise undefined insn exceptions.  */
-void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
+void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
 {
-    ARMCPU *cpu = env_archcpu(env);
+    uint32_t mask = extract32(maskreg, 8, 4);
+    uint32_t reg = extract32(maskreg, 0, 8);
 
-    cpu_abort(CPU(cpu), "v7m_msr %d\n", reg);
+    switch (reg) {
+    case 0 ... 7: /* xPSR sub-fields */
+        v7m_msr_xpsr(env, mask, reg, val);
+        break;
+    case 20: /* CONTROL */
+        /* There are no sub-fields that are actually writable from EL0. */
+        break;
+    }
 }
 
 uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
 {
-    ARMCPU *cpu = env_archcpu(env);
-
-    cpu_abort(CPU(cpu), "v7m_mrs %d\n", reg);
-    return 0;
+    switch (reg) {
+    case 0 ... 7: /* xPSR sub-fields */
+        return v7m_mrs_xpsr(env, reg, 0);
+    case 20: /* CONTROL */
+        return v7m_mrs_control(env, 0);
+    default:
+        /* Unprivileged reads others as zero.  */
+        return 0;
+    }
 }
 
 void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)
@@ -2196,35 +2254,14 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
 
 uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
 {
-    uint32_t mask;
     unsigned el = arm_current_el(env);
 
     /* First handle registers which unprivileged can read */
-
     switch (reg) {
     case 0 ... 7: /* xPSR sub-fields */
-        mask = 0;
-        if ((reg & 1) && el) {
-            mask |= XPSR_EXCP; /* IPSR (unpriv. reads as zero) */
-        }
-        if (!(reg & 4)) {
-            mask |= XPSR_NZCV | XPSR_Q; /* APSR */
-            if (arm_feature(env, ARM_FEATURE_THUMB_DSP)) {
-                mask |= XPSR_GE;
-            }
-        }
-        /* EPSR reads as zero */
-        return xpsr_read(env) & mask;
-        break;
+        return v7m_mrs_xpsr(env, reg, el);
     case 20: /* CONTROL */
-    {
-        uint32_t value = env->v7m.control[env->v7m.secure];
-        if (!env->v7m.secure) {
-            /* SFPA is RAZ/WI from NS; FPCA is stored in the M_REG_S bank */
-            value |= env->v7m.control[M_REG_S] & R_V7M_CONTROL_FPCA_MASK;
-        }
-        return value;
-    }
+        return v7m_mrs_control(env, env->v7m.secure);
     case 0x94: /* CONTROL_NS */
         /*
          * We have to handle this here because unprivileged Secure code
@@ -2454,18 +2491,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
 
     switch (reg) {
     case 0 ... 7: /* xPSR sub-fields */
-        /* only APSR is actually writable */
-        if (!(reg & 4)) {
-            uint32_t apsrmask = 0;
-
-            if (mask & 8) {
-                apsrmask |= XPSR_NZCV | XPSR_Q;
-            }
-            if ((mask & 4) && arm_feature(env, ARM_FEATURE_THUMB_DSP)) {
-                apsrmask |= XPSR_GE;
-            }
-            xpsr_write(env, val, apsrmask);
-        }
+        v7m_msr_xpsr(env, mask, reg, val);
         break;
     case 8: /* MSP */
         if (v7m_using_psp(env)) {
-- 
2.17.1


Re: [PATCH for-4.2] target/arm: Support EL0 v7m msr/mrs for CONFIG_USER_ONLY
Posted by Peter Maydell 4 years, 4 months ago
On Mon, 18 Nov 2019 at 19:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Simply moving the non-stub helper_v7m_mrs/msr outside of
> !CONFIG_USER_ONLY is not an option, because of all of the
> other system-mode helpers that are called.
>
> But we can split out a few subroutines to handle the few
> EL0 accessible registers without duplicating code.
>
> Reported-by: Christophe Lyon <christophe.lyon@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h      |   2 +
>  target/arm/m_helper.c | 110 ++++++++++++++++++++++++++----------------
>  2 files changed, 70 insertions(+), 42 deletions(-)
>
>  /* These should probably raise undefined insn exceptions.  */

This comment is now wrong -- all the 'dummy version for user
mode' helpers below it are either "known to never be called" or
have a "works for user-mode" implementation, so we can just delete it.

> -void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
> +void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>  {
> -    ARMCPU *cpu = env_archcpu(env);
> +    uint32_t mask = extract32(maskreg, 8, 4);
> +    uint32_t reg = extract32(maskreg, 0, 8);
>
> -    cpu_abort(CPU(cpu), "v7m_msr %d\n", reg);
> +    switch (reg) {
> +    case 0 ... 7: /* xPSR sub-fields */
> +        v7m_msr_xpsr(env, mask, reg, val);
> +        break;
> +    case 20: /* CONTROL */
> +        /* There are no sub-fields that are actually writable from EL0. */
> +        break;

I feel like somebody's static analysis is probably going to complain
if we don't have a default case here:
   default:
        /* Writes to all other registers from EL0 are ignored */
        break;

Looks good otherwise, so I'll apply it to target-arm.next with those fixups.

thanks
-- PMM