| Message ID | 20161027151030.20863-31-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 10/27/2016 08:10 AM, Alex Bennée wrote: > cputlb owns the TLB entries and knows how to safely update them in > MTTCG. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > target-arm/cpu.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 1b9540e..ff8c594 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -121,7 +121,13 @@ static void arm_cpu_reset(CPUState *s) > > acc->parent_reset(s); > > +#ifdef CONFIG_SOFTMMU > + memset(env, 0, offsetof(CPUARMState, tlb_table)); > + tlb_flush(s, 0); > +#else > memset(env, 0, offsetof(CPUARMState, features)); > +#endif > + Why special case this for softmmu? And don't we (or if not, shouldn't we) handle the tlb_flush generically for reset? r~
Richard Henderson <rth@twiddle.net> writes: > On 10/27/2016 08:10 AM, Alex Bennée wrote: >> cputlb owns the TLB entries and knows how to safely update them in >> MTTCG. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> target-arm/cpu.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >> index 1b9540e..ff8c594 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -121,7 +121,13 @@ static void arm_cpu_reset(CPUState *s) >> >> acc->parent_reset(s); >> >> +#ifdef CONFIG_SOFTMMU >> + memset(env, 0, offsetof(CPUARMState, tlb_table)); >> + tlb_flush(s, 0); >> +#else >> memset(env, 0, offsetof(CPUARMState, features)); >> +#endif >> + > > Why special case this for softmmu? I didn't want to move cpu->features to the other side of CPU_COMMON in cpu.h as there is an explicit statement about being reset. Adding another variable just to be an endpoint of a memset also seemed sub-optimal. > And don't we (or if not, shouldn't we) > handle the tlb_flush generically for reset? Probably. tlb_flush seems to be one of those things liberally sprinkled in the arch code for all sorts of things but certainly cpu_reset is one we could make the call from generic code. > > > r~ -- Alex Bennée
On 28 October 2016 at 09:38, Alex Bennée <alex.bennee@linaro.org> wrote: > > Richard Henderson <rth@twiddle.net> writes: >> And don't we (or if not, shouldn't we) >> handle the tlb_flush generically for reset? > > Probably. tlb_flush seems to be one of those things liberally sprinkled > in the arch code for all sorts of things but certainly cpu_reset is one > we could make the call from generic code. I think the theory I formed last time I looked at it is that if your CPU has no state which would require you to flush the TLB when it changes, then there's no need to flush the TLB on reset -- any entries still in the TLB from before reset are still valid after reset. For ARM a CPU reset will reset state like the ASID and the MMU-enabled bit which require a TLB flush on change, so we have to call tlb_flush here. You could argue that the set of CPUs which don't require a tlb flush on reset are not worth trying to optimise for like this and we should just do it generically. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 28 October 2016 at 09:38, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> Richard Henderson <rth@twiddle.net> writes: >>> And don't we (or if not, shouldn't we) >>> handle the tlb_flush generically for reset? >> >> Probably. tlb_flush seems to be one of those things liberally sprinkled >> in the arch code for all sorts of things but certainly cpu_reset is one >> we could make the call from generic code. > > I think the theory I formed last time I looked at it is that > if your CPU has no state which would require you to flush the > TLB when it changes, then there's no need to flush the TLB > on reset -- any entries still in the TLB from before reset are > still valid after reset. For ARM a CPU reset will reset state > like the ASID and the MMU-enabled bit which require a TLB flush > on change, so we have to call tlb_flush here. > > You could argue that the set of CPUs which don't require a > tlb flush on reset are not worth trying to optimise for > like this and we should just do it generically. Well it can't harm anyone. It would just mean all CPUs whatever their semantics would have to re-fill the TLBs after a reset. I guess that might be a concern for reset latency under TCG but I doubt anyone would notice. -- Alex Bennée
diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 1b9540e..ff8c594 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -121,7 +121,13 @@ static void arm_cpu_reset(CPUState *s) acc->parent_reset(s); +#ifdef CONFIG_SOFTMMU + memset(env, 0, offsetof(CPUARMState, tlb_table)); + tlb_flush(s, 0); +#else memset(env, 0, offsetof(CPUARMState, features)); +#endif + g_hash_table_foreach(cpu->cp_regs, cp_reg_reset, cpu); g_hash_table_foreach(cpu->cp_regs, cp_reg_check_reset, cpu);
cputlb owns the TLB entries and knows how to safely update them in MTTCG. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- target-arm/cpu.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.10.1