target/arm/helper.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
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
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
>
© 2016 - 2026 Red Hat, Inc.