target/arm: fix exception syndrome for AArch32 bkpt insn

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

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

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
>