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

[Peter Maydell]

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