[PATCH] target/arm: fix missing exception class

[PATCH] target/arm: fix missing exception class

[Jamie Iles]

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(-)

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)
-- 
2.30.2

Re: [PATCH] target/arm: fix missing exception class

[Peter Maydell]

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...

thanks
-- PMM

Re: [PATCH] target/arm: fix missing exception class

[Jamie Iles]

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,

Re: [PATCH] target/arm: fix missing exception class

[Peter Maydell]

On Mon, 24 May 2021 at 13:36, Jamie Iles <jamie@nuviainc.com> wrote:
> On Mon, May 24, 2021 at 10:41:58AM +0100, Peter Maydell wrote:
> > 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);
>  }
>

Broadly speaking, yes. I think it could be done slightly more neatly by
making raise_exception_ra() be:

    CPUState *cs = env_cpu(env);
    /*
     * restore_state_to_opc() will set env->exception.syndrome, so
     * we must restore CPU state here before setting the syndrome
     * the caller passed us, and cannot use cpu_loop_exit_restore().
     */
    cpu_restore_state(cs, ra, true);
    raise_exception(env, excp, syndrome, target_el);

Then there are some follow-up cleanup patches:
 * fold do_raise_exception() into raise_exception(), since that
   is now its only caller
 * mte_check_fail() currently does cpu_restore_state() and then
   raise_exception() with a comment about this being to avoid
   restore_state_to_opc trashing the syndrome; it can now be
   rewritten to just call raise_exception_ra()
 * the v7m_msr helper in m_helper.c does a cpu_restore_state/
   raise_exception sequence than can be rewritten to just call
   raise_exception_ra()

thanks
-- PMM