On Fri, 19 Jan 2024 at 22:40, Jan Klötzke <jan.kloet...@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.kloet...@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
