| Message ID | 20191017185110.539-20-richard.henderson@linaro.org |
|---|---|
| State | New |
| 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> > --- > v7: Add rebuilds for v7m_msr and nvic_writel to v7m.ccr. > --- > hw/intc/armv7m_nvic.c | 1 + > target/arm/m_helper.c | 6 ++++++ > target/arm/translate.c | 5 ++++- > 3 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c > index 8e93e51e81..a3993e7b72 100644 > --- a/hw/intc/armv7m_nvic.c > +++ b/hw/intc/armv7m_nvic.c > @@ -1604,6 +1604,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, > } > > cpu->env.v7m.ccr[attrs.secure] = value; > + arm_rebuild_hflags(&cpu->env); > break; > case 0xd24: /* System Handler Control and State (SHCSR) */ > if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) { This seems fragile -- we have to remember to add in a call to arm_rebuild_hflags() for every individual case of a memory-mapped system register that we choose to cache in tb flags. It doesn't seem consistent with the choice for A-profile to rebuild hflags for pretty much any sysreg write. Maybe it would be better to just always rebuild hflags at the end of nvic_sysreg_write() ? thanks -- PMM
On 10/18/19 5:25 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> >> --- >> v7: Add rebuilds for v7m_msr and nvic_writel to v7m.ccr. >> --- >> hw/intc/armv7m_nvic.c | 1 + >> target/arm/m_helper.c | 6 ++++++ >> target/arm/translate.c | 5 ++++- >> 3 files changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c >> index 8e93e51e81..a3993e7b72 100644 >> --- a/hw/intc/armv7m_nvic.c >> +++ b/hw/intc/armv7m_nvic.c >> @@ -1604,6 +1604,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, >> } >> >> cpu->env.v7m.ccr[attrs.secure] = value; >> + arm_rebuild_hflags(&cpu->env); >> break; >> case 0xd24: /* System Handler Control and State (SHCSR) */ >> if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) { > > This seems fragile -- we have to remember to add in > a call to arm_rebuild_hflags() for every individual > case of a memory-mapped system register that we choose > to cache in tb flags. It doesn't seem consistent with > the choice for A-profile to rebuild hflags for pretty > much any sysreg write. Maybe it would be better to just > always rebuild hflags at the end of nvic_sysreg_write() ? I thought about that, but there were too many returns out of the middle. I suppose a wrapper function would take care of that. r~
On Fri, 18 Oct 2019 at 15:31, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 10/18/19 5:25 AM, Peter Maydell wrote: > > This seems fragile -- we have to remember to add in > > a call to arm_rebuild_hflags() for every individual > > case of a memory-mapped system register that we choose > > to cache in tb flags. It doesn't seem consistent with > > the choice for A-profile to rebuild hflags for pretty > > much any sysreg write. Maybe it would be better to just > > always rebuild hflags at the end of nvic_sysreg_write() ? > > I thought about that, but there were too many returns out of the middle. I > suppose a wrapper function would take care of that. I hadn't noticed the early returns from nvic_sysreg_write(). You could just turn all the 'return MEMTX_OK's into a goto exit_ok and have that do the update. thanks -- PMM
On Fri, 18 Oct 2019 at 15:52, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 18 Oct 2019 at 15:31, Richard Henderson > <richard.henderson@linaro.org> wrote: > > > > On 10/18/19 5:25 AM, Peter Maydell wrote: > > > This seems fragile -- we have to remember to add in > > > a call to arm_rebuild_hflags() for every individual > > > case of a memory-mapped system register that we choose > > > to cache in tb flags. It doesn't seem consistent with > > > the choice for A-profile to rebuild hflags for pretty > > > much any sysreg write. Maybe it would be better to just > > > always rebuild hflags at the end of nvic_sysreg_write() ? > > > > I thought about that, but there were too many returns out of the middle. I > > suppose a wrapper function would take care of that. > > I hadn't noticed the early returns from nvic_sysreg_write(). > You could just turn all the 'return MEMTX_OK's into a > goto exit_ok and have that do the update. PS: just for clarity, nvic_sysreg_write(), not nvic_writel(). thanks -- PMM
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c index 8e93e51e81..a3993e7b72 100644 --- a/hw/intc/armv7m_nvic.c +++ b/hw/intc/armv7m_nvic.c @@ -1604,6 +1604,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, } cpu->env.v7m.ccr[attrs.secure] = value; + arm_rebuild_hflags(&cpu->env); break; case 0xd24: /* System Handler Control and State (SHCSR) */ if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) { diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c index 27cd2f3f96..f2512e448e 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -494,6 +494,7 @@ void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest) switch_v7m_security_state(env, dest & 1); env->thumb = 1; env->regs[15] = dest & ~1; + arm_rebuild_hflags(env); } void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest) @@ -555,6 +556,7 @@ void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest) switch_v7m_security_state(env, 0); env->thumb = 1; env->regs[15] = dest; + arm_rebuild_hflags(env); } static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool threadmode, @@ -895,6 +897,7 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain, env->regs[14] = lr; env->regs[15] = addr & 0xfffffffe; env->thumb = addr & 1; + arm_rebuild_hflags(env); } static void v7m_update_fpccr(CPUARMState *env, uint32_t frameptr, @@ -1765,6 +1768,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu) /* Otherwise, we have a successful exception exit. */ arm_clear_exclusive(env); + arm_rebuild_hflags(env); qemu_log_mask(CPU_LOG_INT, "...successful exception return\n"); } @@ -1837,6 +1841,7 @@ static bool do_v7m_function_return(ARMCPU *cpu) xpsr_write(env, 0, XPSR_IT); env->thumb = newpc & 1; env->regs[15] = newpc & ~1; + arm_rebuild_hflags(env); qemu_log_mask(CPU_LOG_INT, "...function return successful\n"); return true; @@ -1959,6 +1964,7 @@ static bool v7m_handle_execute_nsc(ARMCPU *cpu) switch_v7m_security_state(env, true); xpsr_write(env, 0, XPSR_IT); env->regs[15] += 4; + arm_rebuild_hflags(env); return true; gen_invep: diff --git a/target/arm/translate.c b/target/arm/translate.c index cb47cd9744..b3720cd59b 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -8325,7 +8325,7 @@ static bool trans_MRS_v7m(DisasContext *s, arg_MRS_v7m *a) static bool trans_MSR_v7m(DisasContext *s, arg_MSR_v7m *a) { - TCGv_i32 addr, reg; + TCGv_i32 addr, reg, el; if (!arm_dc_feature(s, ARM_FEATURE_M)) { return false; @@ -8335,6 +8335,9 @@ static bool trans_MSR_v7m(DisasContext *s, arg_MSR_v7m *a) gen_helper_v7m_msr(cpu_env, addr, reg); tcg_temp_free_i32(addr); tcg_temp_free_i32(reg); + el = tcg_const_i32(s->current_el); + gen_helper_rebuild_hflags_m32(cpu_env, el); + tcg_temp_free_i32(el); gen_lookup_tb(s); return true; }