[v1] target/arm: fix exception syndrome for AArch32 bkpt insn

[PATCH] target/arm: fix exception syndrome for AArch32 bkpt insn

Posted by Jan Klötzke 10 months, 1 week ago

Debug exceptions that target AArch32 Hyp mode are reported differently
than on AAarch64. Internally, Qemu uses the AArch64 syndromes. Therefore
such exceptions need to be either converted to a prefetch abort
(breakpoints, vector catch) or a data abort (watchpoints).

Signed-off-by: Jan Klötzke <jan.kloetzke@kernkonzept.com>
---
 target/arm/helper.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index e068d35383..71dd60ad2d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11013,6 +11013,26 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
     }
 
     if (env->exception.target_el == 2) {
+        /* Debug exceptions are reported differently on AARCH32 */
+        switch (syn_get_ec(env->exception.syndrome)) {
+        case EC_BREAKPOINT:
+        case EC_BREAKPOINT_SAME_EL:
+        case EC_AA32_BKPT:
+        case EC_VECTORCATCH:
+            env->exception.syndrome = syn_insn_abort(arm_current_el(env) == 2,
+                                                     0, 0, 0x22);
+            break;
+        case EC_WATCHPOINT:
+        case EC_WATCHPOINT_SAME_EL:
+            /*
+             * ISS is compatible between Watchpoints and Data Aborts. Also
+             * retain the lowest EC bit as it signals the originating EL.
+             */
+            env->exception.syndrome &= (1U << (ARM_EL_EC_SHIFT + 1)) - 1U;
+            env->exception.syndrome |= (EC_DATAABORT << ARM_EL_EC_SHIFT)
+                                       | ARM_EL_ISV;
+            break;
+        }
         arm_cpu_do_interrupt_aarch32_hyp(cs);
         return;
     }
-- 
2.39.2

Re: [PATCH] target/arm: fix exception syndrome for AArch32 bkpt insn

Posted by Peter Maydell 10 months, 1 week ago

On Fri, 19 Jan 2024 at 22:40, Jan Klötzke <jan.kloetzke@kernkonzept.com> wrote:
>
> Debug exceptions that target AArch32 Hyp mode are reported differently
> than on AAarch64. Internally, Qemu uses the AArch64 syndromes. Therefore
> such exceptions need to be either converted to a prefetch abort
> (breakpoints, vector catch) or a data abort (watchpoints).
>
> Signed-off-by: Jan Klötzke <jan.kloetzke@kernkonzept.com>

Thanks for the patch; yes, this is definitely a bug.

> ---
>  target/arm/helper.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index e068d35383..71dd60ad2d 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11013,6 +11013,26 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
>      }
>
>      if (env->exception.target_el == 2) {
> +        /* Debug exceptions are reported differently on AARCH32 */

Capitalization is "AArch32".

> +        switch (syn_get_ec(env->exception.syndrome)) {
> +        case EC_BREAKPOINT:
> +        case EC_BREAKPOINT_SAME_EL:
> +        case EC_AA32_BKPT:
> +        case EC_VECTORCATCH:
> +            env->exception.syndrome = syn_insn_abort(arm_current_el(env) == 2,
> +                                                     0, 0, 0x22);
> +            break;
> +        case EC_WATCHPOINT:
> +        case EC_WATCHPOINT_SAME_EL:
> +            /*
> +             * ISS is compatible between Watchpoints and Data Aborts. Also
> +             * retain the lowest EC bit as it signals the originating EL.
> +             */
> +            env->exception.syndrome &= (1U << (ARM_EL_EC_SHIFT + 1)) - 1U;

Is this supposed to be clearing out (most of) the EC field?
I'm not sure that's what it's doing. In any case I think we
could write this in a more clearly understandable way using
either some new #defines or functions in syndrome.h or the
deposit64/extract64 functions.

My suggestion is to put in syndrome.h:

#define ARM_EL_EC_LENGTH 6

static inline uint32_t syn_set_ec(uint32_t syn, uint32_t ec)
{
    return deposit32(syn, ARM_EL_EC_SHIFT, ARM_EL_EC_LENGTH, ec);
}

(you'll need to add #include "qemu/bitops.h" too)

and then these cases can be written:

    case EC_WATCHPOINT:
        env->exception.syndrome = syn_set_ec(env->exception.syndrome,
                                             EC_DATAABORT);
        break;
    case EC_WATCHPOINT_SAME_EL:
        env->exception.syndrome = syn_set_ec(env->exception.syndrome,
                                             EC_DATAABORT_SAME_EL);
        break;

> +            env->exception.syndrome |= (EC_DATAABORT << ARM_EL_EC_SHIFT)
> +                                       | ARM_EL_ISV;

I don't think we should be setting ISV here -- the EC_WATCHPOINT
syndromes don't have any of the instruction-syndrome info
and "watchpoint" isn't one of the cases where an AArch32
data-abort syndrome should have ISV set.

> +            break;
> +        }
>          arm_cpu_do_interrupt_aarch32_hyp(cs);
>          return;
>      }
> --

thanks
-- PMM

Re: [PATCH] target/arm: fix exception syndrome for AArch32 bkpt insn

Posted by Jan Klötzke 10 months ago

On Tue, 2024-01-23 at 17:58 +0000, Peter Maydell wrote:
> On Fri, 19 Jan 2024 at 22:40, Jan Klötzke <jan.kloetzke@kernkonzept.com> wrote:
> 
> > ---
> >  target/arm/helper.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index e068d35383..71dd60ad2d 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -11013,6 +11013,26 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
> >      }
> > 
> >      if (env->exception.target_el == 2) {
> > +        /* Debug exceptions are reported differently on AARCH32 */
> 
> Capitalization is "AArch32".

Right.

> > +        switch (syn_get_ec(env->exception.syndrome)) {
> > +        case EC_BREAKPOINT:
> > +        case EC_BREAKPOINT_SAME_EL:
> > +        case EC_AA32_BKPT:
> > +        case EC_VECTORCATCH:
> > +            env->exception.syndrome = syn_insn_abort(arm_current_el(env) == 2,
> > +                                                     0, 0, 0x22);
> > +            break;
> > +        case EC_WATCHPOINT:
> > +        case EC_WATCHPOINT_SAME_EL:
> > +            /*
> > +             * ISS is compatible between Watchpoints and Data Aborts. Also
> > +             * retain the lowest EC bit as it signals the originating EL.
> > +             */
> > +            env->exception.syndrome &= (1U << (ARM_EL_EC_SHIFT + 1)) - 1U;
> 
> Is this supposed to be clearing out (most of) the EC field?
> I'm not sure that's what it's doing.

Yes, this was the intention. But I admit it's barely readable.

> In any case I think we
> could write this in a more clearly understandable way using
> either some new #defines or functions in syndrome.h or the
> deposit64/extract64 functions.
> 
> My suggestion is to put in syndrome.h:
> 
> #define ARM_EL_EC_LENGTH 6
> 
> static inline uint32_t syn_set_ec(uint32_t syn, uint32_t ec)
> {
>     return deposit32(syn, ARM_EL_EC_SHIFT, ARM_EL_EC_LENGTH, ec);
> }
> 
> (you'll need to add #include "qemu/bitops.h" too)
> 
> and then these cases can be written:
> 
>     case EC_WATCHPOINT:
>         env->exception.syndrome = syn_set_ec(env->exception.syndrome,
>                                              EC_DATAABORT);
>         break;
>     case EC_WATCHPOINT_SAME_EL:
>         env->exception.syndrome = syn_set_ec(env->exception.syndrome,
>                                              EC_DATAABORT_SAME_EL);
>         break;

Yes, that is much better. I'll send a V2 shortly.

> 
> > +            env->exception.syndrome |= (EC_DATAABORT << ARM_EL_EC_SHIFT)
> > +                                       | ARM_EL_ISV;
> 
> I don't think we should be setting ISV here -- the EC_WATCHPOINT
> syndromes don't have any of the instruction-syndrome info
> and "watchpoint" isn't one of the cases where an AArch32
> data-abort syndrome should have ISV set.

Indeed. I guess I meant ARM_EL_IL but this is not required either
because syn_watchpoint() already sets it. I'll remove it.

Thanks,
Jan
>