diff mbox series

target/arm: fix missing exception class

Message ID 20210524084201.1665973-1-jamie@nuviainc.com (mailing list archive)
State New, archived
Headers show
Series target/arm: fix missing exception class | expand

Commit Message

Jamie Iles May 24, 2021, 8:42 a.m. UTC
The DAIF and PAC checks used raise_exception_ra to raise an exception
and unwind CPU state but raise_exception_ra is currently designed for
handling data aborts as the syndrome is partially precomputed and
encoded in the TB and then merged in merge_syn_data_abort when handling
the data abort.  Using raise_exception_ra for DAIF and PAC checks
results in an empty syndrome being retrieved from data[2] in
restore_state_to_opc and setting ESR to 0.  This manifested as:

  kvm [571]: Unknown exception class: esr: 0x000000 –
  Unknown/Uncategorized

when launching a KVM guest when the host qemu used a CPU supporting
EL2+pointer authentication and enabling pointer authentication in the
guest.

As the syndrome and exception class is not encoded in the TB, use
cpu_restore_state and raise_exception to restore the CPU state and raise
the exception with the correct syndrome.

Fixes: 0d43e1a2d29a ("target/arm: Add PAuth helpers")
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jamie Iles <jamie@nuviainc.com>
---
 target/arm/helper-a64.c   | 12 +++++++-----
 target/arm/pauth_helper.c |  4 +++-
 2 files changed, 10 insertions(+), 6 deletions(-)

Comments

Jamie Iles May 24, 2021, 12:36 p.m. UTC | #1
Hi Peter,

On Mon, May 24, 2021 at 10:41:58AM +0100, Peter Maydell wrote:
> On Mon, 24 May 2021 at 09:42, Jamie Iles <jamie@nuviainc.com> wrote:
> >
> > The DAIF and PAC checks used raise_exception_ra to raise an exception
> > and unwind CPU state but raise_exception_ra is currently designed for
> > handling data aborts as the syndrome is partially precomputed and
> > encoded in the TB and then merged in merge_syn_data_abort when handling
> > the data abort.  Using raise_exception_ra for DAIF and PAC checks
> > results in an empty syndrome being retrieved from data[2] in
> > restore_state_to_opc and setting ESR to 0.  This manifested as:
> >
> >   kvm [571]: Unknown exception class: esr: 0x000000 –
> >   Unknown/Uncategorized
> >
> > when launching a KVM guest when the host qemu used a CPU supporting
> > EL2+pointer authentication and enabling pointer authentication in the
> > guest.
> 
> raise_exception() and raise_exception_ra() are supposed to have
> the same semantics apart from one of them being passed a return
> address. So perhaps we should look at trying to fix this by
> making raise_exception_ra() not first carefully set and then
> very opaquely unconditionally trash env->exception.syndrome...

The simplest fix for this would be the patch below and that is 
consistent with the TLB fault handling code where we first restore state 
then raise the exception, is this the sort of thing you were thinking 
of?

Thanks,

Jamie

diff --git i/target/arm/op_helper.c w/target/arm/op_helper.c
index efcb60099277..0b85403cb9f4 100644
--- i/target/arm/op_helper.c
+++ w/target/arm/op_helper.c
@@ -63,8 +63,11 @@ void raise_exception(CPUARMState *env, uint32_t excp,
 void raise_exception_ra(CPUARMState *env, uint32_t excp, uint32_t syndrome,
                         uint32_t target_el, uintptr_t ra)
 {
-    CPUState *cs = do_raise_exception(env, excp, syndrome, target_el);
-    cpu_loop_exit_restore(cs, ra);
+    CPUState *cs = env_cpu(env);
+
+    cpu_restore_state(cs, ra, true);
+    do_raise_exception(env, excp, syndrome, target_el);
+    cpu_loop_exit(cs);
 }
 
 uint64_t HELPER(neon_tbl)(CPUARMState *env, uint32_t desc,
diff mbox series

Patch

diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 9cc3b066e28b..83991d783fc1 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -72,11 +72,13 @@  static void daif_check(CPUARMState *env, uint32_t op,
 {
     /* DAIF update to PSTATE. This is OK from EL0 only if UMA is set.  */
     if (arm_current_el(env) == 0 && !(arm_sctlr(env, 0) & SCTLR_UMA)) {
-        raise_exception_ra(env, EXCP_UDEF,
-                           syn_aa64_sysregtrap(0, extract32(op, 0, 3),
-                                               extract32(op, 3, 3), 4,
-                                               imm, 0x1f, 0),
-                           exception_target_el(env), ra);
+        CPUState *cs = env_cpu(env);
+        cpu_restore_state(cs, ra, true);
+        raise_exception(env, EXCP_UDEF,
+                        syn_aa64_sysregtrap(0, extract32(op, 0, 3),
+                                            extract32(op, 3, 3), 4,
+                                            imm, 0x1f, 0),
+                        exception_target_el(env));
     }
 }
 
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index cd6df18150bf..c90a3f765b9c 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -385,7 +385,9 @@  static uint64_t pauth_strip(CPUARMState *env, uint64_t ptr, bool data)
 static void QEMU_NORETURN pauth_trap(CPUARMState *env, int target_el,
                                      uintptr_t ra)
 {
-    raise_exception_ra(env, EXCP_UDEF, syn_pactrap(), target_el, ra);
+    CPUState *cs = env_cpu(env);
+    cpu_restore_state(cs, ra, true);
+    raise_exception(env, EXCP_UDEF, syn_pactrap(), target_el);
 }
 
 static void pauth_check_trap(CPUARMState *env, int el, uintptr_t ra)