| Message ID | 20240119212945.2440655-1-jan.kloetzke@kernkonzept.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: fix exception syndrome for AArch32 bkpt insn | expand |
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 >
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; }
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(+)