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

[Jan Klötzke]

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
>