Re: [PATCH v7 17/20] target/arm: Rebuild hflags at MSR writes

[Peter Maydell]

On Thu, 17 Oct 2019 at 19:51, Richard Henderson
<address@hidden> wrote:
>
> Continue setting, but not relying upon, env->hflags.
>
> Reviewed-by: Alex Bennée <address@hidden>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate-a64.c | 13 +++++++++++--
>  target/arm/translate.c     | 28 +++++++++++++++++++++++-----
>  2 files changed, 34 insertions(+), 7 deletions(-)
> @@ -7068,14 +7070,30 @@ static int disas_coproc_insn(DisasContext *s, 
> uint32_t insn)
>              }
>          }
>
> -        if ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) && (ri->type & 
> ARM_CP_IO)) {
> -            /* I/O operations must end the TB here (whether read or write) */
> -            gen_lookup_tb(s);
> -        } else if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
> -            /* We default to ending the TB on a coprocessor register write,
> +        /* I/O operations must end the TB here (whether read or write) */
> +        need_exit_tb = ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) &&
> +                        (ri->type & ARM_CP_IO));
> +
> +        if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
> +            /*
> +             * A write to any coprocessor regiser that ends a TB

(typo: "register")

> +             * must rebuild the hflags for the next TB.
> +             */
> +            TCGv_i32 tcg_el = tcg_const_i32(s->current_el);
> +            if (arm_dc_feature(s, ARM_FEATURE_M)) {
> +                gen_helper_rebuild_hflags_m32(cpu_env, tcg_el);
> +            } else {
> +                gen_helper_rebuild_hflags_a32(cpu_env, tcg_el);
> +            }
> +            tcg_temp_free_i32(tcg_el);

Why only rebuild hflags if !ARM_CP_SUPPRESS_TB_END ?
For instance on the Xscale CPUs we set SUPPRESS_TB_END for the SCTLR,
but some of the SCTLR bits are cached in hflags, right? Do we somehow
arrange to rebuild the hflags when the TB does eventually end ?

> +            /*
> +             * We default to ending the TB on a coprocessor register write,
>               * but allow this to be suppressed by the register definition
>               * (usually only necessary to work around guest bugs).
>               */
> +            need_exit_tb = true;
> +        }
> +        if (need_exit_tb) {
>              gen_lookup_tb(s);
>          }

thanks
-- PMM