On Sat, 28 Jan 2023 at 01:47, Richard Henderson <richard.hender...@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