diff mbox series

target/arm: Fix Rt/Rt2 in ESR_ELx for copro traps from AArch32 to 64

Message ID 20200803165409.15099-1-peter.maydell@linaro.org
State New
Headers show
Series target/arm: Fix Rt/Rt2 in ESR_ELx for copro traps from AArch32 to 64 | expand

Commit Message

Peter Maydell Aug. 3, 2020, 4:54 p.m. UTC 
When a coprocessor instruction in an  AArch32 guest traps to AArch32
Hyp mode, the syndrome register (HSR) includes Rt and Rt2 fields
which are simply copies of the Rt and Rt2 fields from the trapped
instruction.  However, if the instruction is trapped from AArch32 to
an AArch64 higher exception level, the Rt and Rt2 fields in the
syndrome register (ESR_ELx) must be the AArch64 view of the register.
This makes a difference if the AArch32 guest was in a mode other than
User or System and it was using r13 or r14, or if it was in FIQ mode
and using r8-r14.

We don't know at translate time which AArch32 CPU mode we are in, so
we leave the values we generate in our prototype syndrome register
value at translate time as the raw Rt/Rt2 from the instruction, and
instead correct them to the AArch64 view when we find we need to take
an exception from AArch32 to AArch64 with one of these syndrome
values.

Fixes: https://bugs.launchpad.net/qemu/+bug/1879587
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
NB: I don't have a test case for this so am hoping the bug
submitter will test it for me :-)
---
 target/arm/helper.c | 93 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

Comments

Richard Henderson Aug. 4, 2020, 8:38 p.m. UTC | #1 
On 8/3/20 9:54 AM, Peter Maydell wrote:
> +    case 14:
> +        switch (mode) {
> +        case ARM_CPU_MODE_USR:
> +        case ARM_CPU_MODE_SYS:
> +            return 14;
> +        case ARM_CPU_MODE_HYP:
> +            return 16;

Hyp uses LR_usr...

> +        case ARM_CPU_MODE_IRQ:
> +            return 18;
> +        case ARM_CPU_MODE_SVC:
> +            return 20;
> +        case ARM_CPU_MODE_ABT:
> +            return 22;
> +        case ARM_CPU_MODE_UND:
> +            return 24;

... making all of these off-by-2.

> +        case ARM_CPU_MODE_FIQ:
> +            return 30;
> +        default:
> +            g_assert_not_reached();
> +        }
> +    case 15:
> +        return 31;

I don't see that R15 is mapped at all.  Is this really reachable?

Otherwise it looks ok.


r~
Peter Maydell Aug. 5, 2020, 9:34 a.m. UTC | #2 
On Tue, 4 Aug 2020 at 21:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/3/20 9:54 AM, Peter Maydell wrote:
> > +    case 14:
> > +        switch (mode) {
> > +        case ARM_CPU_MODE_USR:
> > +        case ARM_CPU_MODE_SYS:
> > +            return 14;
> > +        case ARM_CPU_MODE_HYP:
> > +            return 16;
>
> Hyp uses LR_usr...
>
> > +        case ARM_CPU_MODE_IRQ:
> > +            return 18;
> > +        case ARM_CPU_MODE_SVC:
> > +            return 20;
> > +        case ARM_CPU_MODE_ABT:
> > +            return 22;
> > +        case ARM_CPU_MODE_UND:
> > +            return 24;
>
> ... making all of these off-by-2.

Yeah, this is the bug fixed in v2.

> > +        case ARM_CPU_MODE_FIQ:
> > +            return 30;
> > +        default:
> > +            g_assert_not_reached();
> > +        }
> > +    case 15:
> > +        return 31;
>
> I don't see that R15 is mapped at all.  Is this really reachable?

It is -- you can get Rt=0b1111 for MRC to APSR_nzcv (as well
as for various UNPREDICTABLE MCR etc cases that will get trapped
here that we have to do something sensible for). Rt=0b1111
is mapped to 0b11111 in the pseudocode (see
AArch64.AArch32SystemAccessTrapSyndrome()); it's just not
documented in the text (yet) that this is the required behaviour.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8ef0fb478f4..60b80228fd7 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9581,6 +9581,67 @@  static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
     take_aarch32_exception(env, new_mode, mask, offset, addr);
 }
 
+static int aarch64_regnum(CPUARMState *env, int aarch32_reg)
+{
+    /*
+     * Return the register number of the AArch64 view of the AArch32
+     * register @aarch32_reg. The CPUARMState CPSR is assumed to still
+     * be that of the AArch32 mode the exception came from.
+     */
+    int mode = env->uncached_cpsr & CPSR_M;
+
+    switch (aarch32_reg) {
+    case 0 ... 7:
+        return aarch32_reg;
+    case 8 ... 12:
+        return mode == ARM_CPU_MODE_FIQ ? aarch32_reg + 16 : aarch32_reg;
+    case 13:
+        switch (mode) {
+        case ARM_CPU_MODE_USR:
+        case ARM_CPU_MODE_SYS:
+            return 13;
+        case ARM_CPU_MODE_HYP:
+            return 15;
+        case ARM_CPU_MODE_IRQ:
+            return 17;
+        case ARM_CPU_MODE_SVC:
+            return 19;
+        case ARM_CPU_MODE_ABT:
+            return 21;
+        case ARM_CPU_MODE_UND:
+            return 23;
+        case ARM_CPU_MODE_FIQ:
+            return 29;
+        default:
+            g_assert_not_reached();
+        }
+    case 14:
+        switch (mode) {
+        case ARM_CPU_MODE_USR:
+        case ARM_CPU_MODE_SYS:
+            return 14;
+        case ARM_CPU_MODE_HYP:
+            return 16;
+        case ARM_CPU_MODE_IRQ:
+            return 18;
+        case ARM_CPU_MODE_SVC:
+            return 20;
+        case ARM_CPU_MODE_ABT:
+            return 22;
+        case ARM_CPU_MODE_UND:
+            return 24;
+        case ARM_CPU_MODE_FIQ:
+            return 30;
+        default:
+            g_assert_not_reached();
+        }
+    case 15:
+        return 31;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 /* Handle exception entry to a target EL which is using AArch64 */
 static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
 {
@@ -9591,6 +9652,7 @@  static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
     unsigned int new_mode = aarch64_pstate_mode(new_el, true);
     unsigned int old_mode;
     unsigned int cur_el = arm_current_el(env);
+    int rt;
 
     /*
      * Note that new_el can never be 0.  If cur_el is 0, then
@@ -9645,7 +9707,8 @@  static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
     case EXCP_HVC:
     case EXCP_HYP_TRAP:
     case EXCP_SMC:
-        if (syn_get_ec(env->exception.syndrome) == EC_ADVSIMDFPACCESSTRAP) {
+        switch (syn_get_ec(env->exception.syndrome)) {
+        case EC_ADVSIMDFPACCESSTRAP:
             /*
              * QEMU internal FP/SIMD syndromes from AArch32 include the
              * TA and coproc fields which are only exposed if the exception
@@ -9653,6 +9716,34 @@  static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
              * AArch64 format syndrome.
              */
             env->exception.syndrome &= ~MAKE_64BIT_MASK(0, 20);
+            break;
+        case EC_CP14RTTRAP:
+        case EC_CP15RTTRAP:
+        case EC_CP14DTTRAP:
+            /*
+             * For a trap on AArch32 MRC/MCR/LDC/STC the Rt field is currently
+             * the raw register field from the insn; when taking this to
+             * AArch64 we must convert it to the AArch64 view of the register
+             * number. Notice that we read a 4-bit AArch32 register number and
+             * write back a 5-bit AArch64 one.
+             */
+            rt = extract32(env->exception.syndrome, 5, 4);
+            rt = aarch64_regnum(env, rt);
+            env->exception.syndrome = deposit32(env->exception.syndrome,
+                                                5, 5, rt);
+            break;
+        case EC_CP15RRTTRAP:
+        case EC_CP14RRTTRAP:
+            /* Similarly for MRRC/MCRR traps for Rt and Rt2 fields */
+            rt = extract32(env->exception.syndrome, 5, 4);
+            rt = aarch64_regnum(env, rt);
+            env->exception.syndrome = deposit32(env->exception.syndrome,
+                                                5, 5, rt);
+            rt = extract32(env->exception.syndrome, 10, 4);
+            rt = aarch64_regnum(env, rt);
+            env->exception.syndrome = deposit32(env->exception.syndrome,
+                                                10, 5, rt);
+            break;
         }
         env->cp15.esr_el[new_el] = env->exception.syndrome;
         break;