| Message ID | 20181009174557.16125-2-cota@braap.org |
|---|---|
| State | New |
| Headers | show |
| Series | per-TLB lock | expand |
On 9 October 2018 at 18:45, Emilio G. Cota <cota@braap.org> wrote: > As far as I can tell tlb_flush does not need to be called > this early. tlb_flush is eventually called after the CPU > has been realized. > > This change paves the way to the introduction of tlb_init, > which will be called from cpu_exec_realizefn. > > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > target/alpha/cpu.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c > index b08078e7fc..a953897fcc 100644 > --- a/target/alpha/cpu.c > +++ b/target/alpha/cpu.c > @@ -201,7 +201,6 @@ static void alpha_cpu_initfn(Object *obj) > CPUAlphaState *env = &cpu->env; > > cs->env_ptr = env; > - tlb_flush(cs); > > env->lock_addr = -1; > #if defined(CONFIG_USER_ONLY) > -- > 2.17.1 Definitely agreed that we don't want to tlb_flush in the target cpu initfn. What's the codepath by which tlb_flush gets called on cpu reset? I had a quick look but couldn't find it... (The other dubious-looking bit of flushing in the target/alpha code is the code that generates calls to tb_flush()... we have very few calls to tb_flush outside the 'core' code and I suspect they could all be avoided.) thanks -- PMM
Emilio G. Cota <cota@braap.org> writes: > As far as I can tell tlb_flush does not need to be called > this early. tlb_flush is eventually called after the CPU > has been realized. > > This change paves the way to the introduction of tlb_init, > which will be called from cpu_exec_realizefn. > > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > target/alpha/cpu.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c > index b08078e7fc..a953897fcc 100644 > --- a/target/alpha/cpu.c > +++ b/target/alpha/cpu.c > @@ -201,7 +201,6 @@ static void alpha_cpu_initfn(Object *obj) > CPUAlphaState *env = &cpu->env; > > cs->env_ptr = env; > - tlb_flush(cs); > > env->lock_addr = -1; > #if defined(CONFIG_USER_ONLY) Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
On Tue, Oct 09, 2018 at 18:55:30 +0100, Peter Maydell wrote: > On 9 October 2018 at 18:45, Emilio G. Cota <cota@braap.org> wrote: (snip) > > @@ -201,7 +201,6 @@ static void alpha_cpu_initfn(Object *obj) > > CPUAlphaState *env = &cpu->env; > > > > cs->env_ptr = env; > > - tlb_flush(cs); > > > > env->lock_addr = -1; > > #if defined(CONFIG_USER_ONLY) > > -- > > 2.17.1 > > Definitely agreed that we don't want to tlb_flush in the > target cpu initfn. > > > What's the codepath by which tlb_flush gets called on > cpu reset? I had a quick look but couldn't find it... From cpu.c: static void cpu_common_reset(CPUState *cpu) { CPUClass *cc = CPU_GET_CLASS(cpu); ... if (tcg_enabled()) { cpu_tb_jmp_cache_clear(cpu); tcg_flush_softmmu_tlb(cpu); } } tcg_flush_softmmu_tlb is defined in translate-all.c: /* This is a wrapper for common code that can not use CONFIG_SOFTMMU */ void tcg_flush_softmmu_tlb(CPUState *cs) { #ifdef CONFIG_SOFTMMU tlb_flush(cs); #endif } > (The other dubious-looking bit of flushing in the > target/alpha code is the code that generates calls > to tb_flush()... we have very few calls to tb_flush > outside the 'core' code and I suspect they could all > be avoided.) If the comment below is accurate, seems fair enough. tb_flush is only called from target/alpha through a helper, generated by: /* PALBR */ tcg_gen_st_i64(vb, cpu_env, offsetof(CPUAlphaState, palbr)); /* Changing the PAL base register implies un-chaining all of the TBs that ended with a CALL_PAL. Since the base register usually only changes during boot, flushing everything works well. */ gen_helper_tb_flush(cpu_env); return DISAS_PC_STALE; Thanks, Emilio
On 9 October 2018 at 19:20, Emilio G. Cota <cota@braap.org> wrote: > On Tue, Oct 09, 2018 at 18:55:30 +0100, Peter Maydell wrote: >> >> What's the codepath by which tlb_flush gets called on >> cpu reset? I had a quick look but couldn't find it... > > From cpu.c: > > static void cpu_common_reset(CPUState *cpu) > { > CPUClass *cc = CPU_GET_CLASS(cpu); > ... > if (tcg_enabled()) { > cpu_tb_jmp_cache_clear(cpu); > > tcg_flush_softmmu_tlb(cpu); > } > } > > tcg_flush_softmmu_tlb is defined in translate-all.c: > > /* This is a wrapper for common code that can not use CONFIG_SOFTMMU */ > void tcg_flush_softmmu_tlb(CPUState *cs) > { > #ifdef CONFIG_SOFTMMU > tlb_flush(cs); > #endif > } Ah, thank you. I missed this because of the indirection via tcg_flush_softmmu_tlb(). >> (The other dubious-looking bit of flushing in the >> target/alpha code is the code that generates calls >> to tb_flush()... we have very few calls to tb_flush >> outside the 'core' code and I suspect they could all >> be avoided.) > > If the comment below is accurate, seems fair enough. > tb_flush is only called from target/alpha through a helper, > generated by: > > /* PALBR */ > tcg_gen_st_i64(vb, cpu_env, offsetof(CPUAlphaState, palbr)); > /* Changing the PAL base register implies un-chaining all of the TBs > that ended with a CALL_PAL. Since the base register usually only > changes during boot, flushing everything works well. */ > gen_helper_tb_flush(cpu_env); > return DISAS_PC_STALE; Mmm, it works (though would it work if the CPU wasn't the only one in the system?). I just have a reflexive dislike of design approaches used by only one thing -- I tend to like to smooth them out so there's more consistency... thanks -- PMM
On 10/9/18 11:25 AM, Peter Maydell wrote: >> /* PALBR */ >> tcg_gen_st_i64(vb, cpu_env, offsetof(CPUAlphaState, palbr)); >> /* Changing the PAL base register implies un-chaining all of the TBs >> that ended with a CALL_PAL. Since the base register usually only >> changes during boot, flushing everything works well. */ >> gen_helper_tb_flush(cpu_env); >> return DISAS_PC_STALE; > > Mmm, it works (though would it work if the CPU wasn't the > only one in the system?). I just have a reflexive dislike > of design approaches used by only one thing -- I tend to > like to smooth them out so there's more consistency... It could be less important now that we have goto_ptr. If you really think I should get rid of it, I will. r~
On 10/9/18 10:45 AM, Emilio G. Cota wrote: > As far as I can tell tlb_flush does not need to be called > this early. tlb_flush is eventually called after the CPU > has been realized. > > This change paves the way to the introduction of tlb_init, > which will be called from cpu_exec_realizefn. > > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > target/alpha/cpu.c | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c index b08078e7fc..a953897fcc 100644 --- a/target/alpha/cpu.c +++ b/target/alpha/cpu.c @@ -201,7 +201,6 @@ static void alpha_cpu_initfn(Object *obj) CPUAlphaState *env = &cpu->env; cs->env_ptr = env; - tlb_flush(cs); env->lock_addr = -1; #if defined(CONFIG_USER_ONLY)
As far as I can tell tlb_flush does not need to be called this early. tlb_flush is eventually called after the CPU has been realized. This change paves the way to the introduction of tlb_init, which will be called from cpu_exec_realizefn. Signed-off-by: Emilio G. Cota <cota@braap.org> --- target/alpha/cpu.c | 1 - 1 file changed, 1 deletion(-)