| Message ID | 20191017185110.539-18-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Reduce overhead of cpu_get_tb_cpu_state | expand |
On Thu, 17 Oct 2019 at 19:51, Richard Henderson <richard.henderson@linaro.org> wrote: > > Continue setting, but not relying upon, env->hflags. > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > 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
On 10/18/19 5:32 AM, Peter Maydell wrote: > On Thu, 17 Oct 2019 at 19:51, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> Continue setting, but not relying upon, env->hflags. >> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> 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 ? No, we don't. I assumed that all registers which change TB flags would in fact end the TB. Why did we suppress tb end for Xscale? r~
On Fri, 18 Oct 2019 at 15:30, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 10/18/19 5:32 AM, Peter Maydell wrote: > > On Thu, 17 Oct 2019 at 19:51, Richard Henderson > > <richard.henderson@linaro.org> wrote: > > 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 ? > > No, we don't. I assumed that all registers which change TB flags would in fact > end the TB. > > Why did we suppress tb end for Xscale? The comment in helper.c explains: /* Normally we would always end the TB on an SCTLR write, but Linux * arch/arm/mach-pxa/sleep.S expects two instructions following * an MMU enable to execute from cache. Imitate this behaviour. */ This refers to an older version of the kernel code than the current one: https://elixir.bootlin.com/linux/v2.6.11/source/arch/arm/mach-pxa/sleep.S#L166 which assumed that the two insns after the SCTLR write that enables the MMU run from cache and so don't need to actually be accessible via the addresses that the pre-MMU-enable code runs from. This bit of kernel magic eventually went away with commit 4f5ad99bb5331c57 (kernel v2.6.39) which converted the PXA to use the generic suspend/resume support, which presumably handles the enable-the-MMU transition in a more sensible fashion where the address it's executing from is valid both before and after the MMU is enabled. thanks -- PMM
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index 2d6cd09634..d4bebbe629 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -1789,8 +1789,17 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread, 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) */ s->base.is_jmp = DISAS_UPDATE; - } else if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) { - /* We default to ending the TB on a coprocessor register write, + } + if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) { + /* + * A write to any coprocessor regiser that ends a TB + * must rebuild the hflags for the next TB. + */ + TCGv_i32 tcg_el = tcg_const_i32(s->current_el); + gen_helper_rebuild_hflags_a64(cpu_env, tcg_el); + tcg_temp_free_i32(tcg_el); + /* + * 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). */ diff --git a/target/arm/translate.c b/target/arm/translate.c index 698c594e8c..cb47cd9744 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -6890,6 +6890,8 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn) ri = get_arm_cp_reginfo(s->cp_regs, ENCODE_CP_REG(cpnum, is64, s->ns, crn, crm, opc1, opc2)); if (ri) { + bool need_exit_tb; + /* Check access permissions */ if (!cp_access_ok(s->current_el, ri, isread)) { return 1; @@ -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 + * 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); + /* + * 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); }