| Message ID | 20181029155339.15280-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Minimize TLB flushing for ASID changes | expand |
On 29 October 2018 at 15:53, Richard Henderson <richard.henderson@linaro.org> wrote: > Although we can't do much with ASIDs except remember them, this > will allow cleanups within target/ that should make things clearer. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/cpu-defs.h | 2 ++ > include/exec/exec-all.h | 19 +++++++++++++++++++ > accel/tcg/cputlb.c | 23 +++++++++++++++++++++++ > 3 files changed, 44 insertions(+) > > diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h > index 6a60f94a41..8fbfe8c8e2 100644 > --- a/include/exec/cpu-defs.h > +++ b/include/exec/cpu-defs.h > @@ -152,6 +152,8 @@ typedef struct CPUTLBDesc { > target_ulong large_page_mask; > /* The next index to use in the tlb victim table. */ > size_t vindex; > + /* The current ASID for this tlb. */ > + uint32_t asid; > } CPUTLBDesc; > > /* > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 815e5b1e83..478f488704 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -226,6 +226,21 @@ void tlb_flush_by_mmuidx_all_cpus(CPUState *cpu, uint16_t idxmap); > * depend on when the guests translation ends the TB. > */ > void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu, uint16_t idxmap); > +/** > + * tlb_set_asid_for_mmuidx: > + * @cpu: Originating cpu > + * @asid: Address Space Identifier > + * @idxmap: bitmap of MMU indicies to set to @asid "indices", but it looks like in this header we consistently use "MMU indexes", so better to stick with that. (Similarly below.) > + * @depmap: bitmap of dependent MMU indicies > + * > + * Set an ASID for all of @idxmap. If any previous ASID was different, > + * then we may flush the mmu idx. If a flush is required, then also flush presumably "will flush", rather than "may flush" ? > + * all dependent mmu indicies in @depmap. This latter is typically used > + * for secondary page resolution, for implementing virtualization within > + * the guest. > + */ > +void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, > + uint16_t idxmap, uint16_t dep_idxmap); > /** > * tlb_set_page_with_attrs: > * @cpu: CPU to add this TLB entry for > @@ -311,6 +326,10 @@ static inline void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu, > uint16_t idxmap) > { > } > +static inline void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, > + uint16_t idxmap, uint16_t depmap) > +{ > +} > #endif > > #define CODE_GEN_ALIGN 16 /* must be >= of the size of a icache line */ > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index af6bd8ccf9..60b3dc2de3 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -360,6 +360,29 @@ void tlb_flush_page_all_cpus_synced(CPUState *src, target_ulong addr) > tlb_flush_page_by_mmuidx_all_cpus_synced(src, addr, ALL_MMUIDX_BITS); > } > > +void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, uint16_t idxmap, > + uint16_t depmap) > +{ > + CPUArchState *env = cpu->env_ptr; > + uint16_t work, to_flush = 0; The other functions that work on the tlb defer their actual operation to an async-work type function or do a run-on-cpu if the passed-in CPU is not the current CPU. Do we need to do that here too? > + > + /* > + * We don't support ASIDs except for trivially. > + * If there is any change, then we must flush the TLB. > + */ > + for (work = idxmap; work != 0; work &= work - 1) { > + int mmu_idx = ctz32(work); > + if (env->tlb_d[mmu_idx].asid != asid) { > + env->tlb_d[mmu_idx].asid = asid; > + to_flush = idxmap; > + } > + } So this will flush all the passed in indexes in idxmap if any one of them was previously the wrong ASID. Is that necessary, or could we just flush only the ones which were wrong and not flush any that were already the correct ASID ? > + if (to_flush) { > + to_flush |= depmap; > + tlb_flush_by_mmuidx_async_work(cpu, RUN_ON_CPU_HOST_INT(to_flush)); > + } > +} > + > /* update the TLBs so that writes to code in the virtual page 'addr' > can be detected */ > void tlb_protect_code(ram_addr_t ram_addr) thanks -- PMM
On 11/15/18 7:36 PM, Peter Maydell wrote: > On 29 October 2018 at 15:53, Richard Henderson > <richard.henderson@linaro.org> wrote: >> Although we can't do much with ASIDs except remember them, this >> will allow cleanups within target/ that should make things clearer. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> include/exec/cpu-defs.h | 2 ++ >> include/exec/exec-all.h | 19 +++++++++++++++++++ >> accel/tcg/cputlb.c | 23 +++++++++++++++++++++++ >> 3 files changed, 44 insertions(+) >> >> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h >> index 6a60f94a41..8fbfe8c8e2 100644 >> --- a/include/exec/cpu-defs.h >> +++ b/include/exec/cpu-defs.h >> @@ -152,6 +152,8 @@ typedef struct CPUTLBDesc { >> target_ulong large_page_mask; >> /* The next index to use in the tlb victim table. */ >> size_t vindex; >> + /* The current ASID for this tlb. */ >> + uint32_t asid; >> } CPUTLBDesc; >> >> /* >> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >> index 815e5b1e83..478f488704 100644 >> --- a/include/exec/exec-all.h >> +++ b/include/exec/exec-all.h >> @@ -226,6 +226,21 @@ void tlb_flush_by_mmuidx_all_cpus(CPUState *cpu, uint16_t idxmap); >> * depend on when the guests translation ends the TB. >> */ >> void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu, uint16_t idxmap); >> +/** >> + * tlb_set_asid_for_mmuidx: >> + * @cpu: Originating cpu >> + * @asid: Address Space Identifier >> + * @idxmap: bitmap of MMU indicies to set to @asid > > "indices", but it looks like in this header we consistently > use "MMU indexes", so better to stick with that. (Similarly below.) > >> + * @depmap: bitmap of dependent MMU indicies >> + * >> + * Set an ASID for all of @idxmap. If any previous ASID was different, >> + * then we may flush the mmu idx. If a flush is required, then also flush > > presumably "will flush", rather than "may flush" ? > >> + * all dependent mmu indicies in @depmap. This latter is typically used >> + * for secondary page resolution, for implementing virtualization within >> + * the guest. >> + */ >> +void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, >> + uint16_t idxmap, uint16_t dep_idxmap); >> /** >> * tlb_set_page_with_attrs: >> * @cpu: CPU to add this TLB entry for >> @@ -311,6 +326,10 @@ static inline void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu, >> uint16_t idxmap) >> { >> } >> +static inline void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, >> + uint16_t idxmap, uint16_t depmap) >> +{ >> +} >> #endif >> >> #define CODE_GEN_ALIGN 16 /* must be >= of the size of a icache line */ >> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c >> index af6bd8ccf9..60b3dc2de3 100644 >> --- a/accel/tcg/cputlb.c >> +++ b/accel/tcg/cputlb.c >> @@ -360,6 +360,29 @@ void tlb_flush_page_all_cpus_synced(CPUState *src, target_ulong addr) >> tlb_flush_page_by_mmuidx_all_cpus_synced(src, addr, ALL_MMUIDX_BITS); >> } >> >> +void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, uint16_t idxmap, >> + uint16_t depmap) >> +{ >> + CPUArchState *env = cpu->env_ptr; >> + uint16_t work, to_flush = 0; > > The other functions that work on the tlb defer their > actual operation to an async-work type function or > do a run-on-cpu if the passed-in CPU is not the current > CPU. Do we need to do that here too? I don't *think* so. I would expect an ASID to be set in response to some cpu-local change, like setting TTBR0_EL1. Something that cannot be done across cpus. I could assert_cpu_is_self, if you like. What I *should* be adding as well is cross-cpu asid-specific tlb flushing, a-la TLBI ASID. That would need the run-on-cpu stuff. > So this will flush all the passed in indexes in idxmap > if any one of them was previously the wrong ASID. Is that > necessary, or could we just flush only the ones which > were wrong and not flush any that were already the correct ASID ? It probably wouldn't be necessary. But I also sort of expect the set of indexes to always have the same ASID -- e.g. kernel vs user views of the same address space, e.g. S12NSE0 + S12NSE1. I don't really know what to do when this presumed invariant is violated. r~
On 15 November 2018 at 18:51, Richard Henderson <richard.henderson@linaro.org> wrote: > On 11/15/18 7:36 PM, Peter Maydell wrote: >> On 29 October 2018 at 15:53, Richard Henderson >> <richard.henderson@linaro.org> wrote: >>> Although we can't do much with ASIDs except remember them, this >>> will allow cleanups within target/ that should make things clearer. >>> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> +void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, uint16_t idxmap, >>> + uint16_t depmap) >>> +{ >>> + CPUArchState *env = cpu->env_ptr; >>> + uint16_t work, to_flush = 0; >> >> The other functions that work on the tlb defer their >> actual operation to an async-work type function or >> do a run-on-cpu if the passed-in CPU is not the current >> CPU. Do we need to do that here too? > > I don't *think* so. I would expect an ASID to be set in response to some > cpu-local change, like setting TTBR0_EL1. Something that cannot be done across > cpus. I could assert_cpu_is_self, if you like. Yes, if the function expects to be called only for the current CPU we should assert and document this. > What I *should* be adding as well is cross-cpu asid-specific tlb flushing, a-la > TLBI ASID. That would need the run-on-cpu stuff. > >> So this will flush all the passed in indexes in idxmap >> if any one of them was previously the wrong ASID. Is that >> necessary, or could we just flush only the ones which >> were wrong and not flush any that were already the correct ASID ? > > It probably wouldn't be necessary. But I also sort of expect the set of > indexes to always have the same ASID -- e.g. kernel vs user views of the same > address space, e.g. S12NSE0 + S12NSE1. I don't really know what to do when > this presumed invariant is violated. We should either document the invariant at the cputlb.c level (and assert it, if not too tedious to do), or say that the cputlb code doesn't care, and just only flush the indexes which have the wrong ASID currently, I think. thanks -- PMM
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index 6a60f94a41..8fbfe8c8e2 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -152,6 +152,8 @@ typedef struct CPUTLBDesc { target_ulong large_page_mask; /* The next index to use in the tlb victim table. */ size_t vindex; + /* The current ASID for this tlb. */ + uint32_t asid; } CPUTLBDesc; /* diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 815e5b1e83..478f488704 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -226,6 +226,21 @@ void tlb_flush_by_mmuidx_all_cpus(CPUState *cpu, uint16_t idxmap); * depend on when the guests translation ends the TB. */ void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu, uint16_t idxmap); +/** + * tlb_set_asid_for_mmuidx: + * @cpu: Originating cpu + * @asid: Address Space Identifier + * @idxmap: bitmap of MMU indicies to set to @asid + * @depmap: bitmap of dependent MMU indicies + * + * Set an ASID for all of @idxmap. If any previous ASID was different, + * then we may flush the mmu idx. If a flush is required, then also flush + * all dependent mmu indicies in @depmap. This latter is typically used + * for secondary page resolution, for implementing virtualization within + * the guest. + */ +void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, + uint16_t idxmap, uint16_t dep_idxmap); /** * tlb_set_page_with_attrs: * @cpu: CPU to add this TLB entry for @@ -311,6 +326,10 @@ static inline void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu, uint16_t idxmap) { } +static inline void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, + uint16_t idxmap, uint16_t depmap) +{ +} #endif #define CODE_GEN_ALIGN 16 /* must be >= of the size of a icache line */ diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index af6bd8ccf9..60b3dc2de3 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -360,6 +360,29 @@ void tlb_flush_page_all_cpus_synced(CPUState *src, target_ulong addr) tlb_flush_page_by_mmuidx_all_cpus_synced(src, addr, ALL_MMUIDX_BITS); } +void tlb_set_asid_for_mmuidx(CPUState *cpu, uint32_t asid, uint16_t idxmap, + uint16_t depmap) +{ + CPUArchState *env = cpu->env_ptr; + uint16_t work, to_flush = 0; + + /* + * We don't support ASIDs except for trivially. + * If there is any change, then we must flush the TLB. + */ + for (work = idxmap; work != 0; work &= work - 1) { + int mmu_idx = ctz32(work); + if (env->tlb_d[mmu_idx].asid != asid) { + env->tlb_d[mmu_idx].asid = asid; + to_flush = idxmap; + } + } + if (to_flush) { + to_flush |= depmap; + tlb_flush_by_mmuidx_async_work(cpu, RUN_ON_CPU_HOST_INT(to_flush)); + } +} + /* update the TLBs so that writes to code in the virtual page 'addr' can be detected */ void tlb_protect_code(ram_addr_t ram_addr)
Although we can't do much with ASIDs except remember them, this will allow cleanups within target/ that should make things clearer. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/exec/cpu-defs.h | 2 ++ include/exec/exec-all.h | 19 +++++++++++++++++++ accel/tcg/cputlb.c | 23 +++++++++++++++++++++++ 3 files changed, 44 insertions(+) -- 2.17.2