Re: [PATCH 06/23] target/arm: Make HSTR_EL2 traps take priority over UNDEF-at-EL1

From: Peter Maydell <peter.maydell@linaro.org>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-arm@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH 06/23] target/arm: Make HSTR_EL2 traps take priority over UNDEF-at-EL1
Date: Sat, 28 Jan 2023 14:34:34 +0000	[thread overview]
Message-ID: <CAFEAcA9hRd2ZEvJhS5brfsnw5WqvFqVvDhA-aZpb4uzFyF-xDw@mail.gmail.com> (raw)
In-Reply-To: <351b2481-4e80-57db-f06c-3dbf95db06fe@linaro.org>

On Sat, 28 Jan 2023 at 01:47, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/27/23 07:54, Peter Maydell wrote:
> > +void HELPER(hstr_trap_check)(CPUARMState *env, uint32_t mask, uint32_t syndrome)
> > +{
> > +    if (env->cp15.hstr_el2 & mask) {
> > +        raise_exception(env, EXCP_UDEF, syndrome, 2);
> > +    }
>
> This is so simple...
>
>
> > @@ -4760,6 +4761,28 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
> >           break;
> >       }
> >
> > +    if (s->hstr_active && cpnum == 15 && s->current_el == 1) {
> > +        /*
> > +         * At EL1, check for a HSTR_EL2 trap, which must take precedence
> > +         * over the UNDEF for "no such register" or the UNDEF for "access
> > +         * permissions forbid this EL1 access". HSTR_EL2 traps from EL0
> > +         * only happen if the cpreg doesn't UNDEF at EL0, so we do those in
> > +         * access_check_cp_reg(), after the checks for whether the access
> > +         * configurably trapped to EL1.
> > +         */
> > +        uint32_t maskbit = is64 ? crm : crn;
> > +
> > +        if (maskbit != 4 && maskbit != 14) {
> > +            /* T4 and T14 are RES0 so never cause traps */
> > +            gen_set_condexec(s);
> > +            gen_update_pc(s, 0);
> > +            emitted_update_pc = true;
> > +            gen_helper_hstr_trap_check(cpu_env,
> > +                                       tcg_constant_i32(1 << maskbit),
> > +                                       tcg_constant_i32(syndrome));
> > +        }
>
> How about
>
>      if (maskbit...) {
>          TCGv_i32 t = load_cpu_offset(offsetoflow32(CPUARMState, hstr_el2));
>          DisasLabel *over = gen_disas_label(s);
>
>          tcg_gen_andi_i32(t, t, 1u << maskbit);
>          tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, over.label);
>          tcg_temp_free_i32(t);
>
>          gen_exception_insn(s, 0, EXCP_UDEF, syndrome);
>          set_disas_label(s, over);
>      }
>
> which also eliminates the need for emitted_update_pc.

I really dislike use of brcond in generated TCG, because of the
massive beartrap it sets up where all your temporaries get nuked
but there's no compile-time checking that you didn't try to keep
using one after the brcond. So I generally prefer an approach that
avoids brcond over one that uses it, if it's available.

thanks
-- PMM