On Tue, 2024-01-23 at 17:58 +0000, Peter Maydell wrote:
> On Fri, 19 Jan 2024 at 22:40, Jan Klötzke <jan.kloet...@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
>
