| Message ID | 20240119204608.779541-3-jcmvbkbc@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/xtensa: tidy xtensa memory management | expand |
On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote: > > Whether TLB ways 5 and 6 are variable is not a property of the TLB > instance or a TLB entry instance, it's a property of the xtensa core > configuration. > Remove 'varway56' field from the xtensa_tlb structure and remove > 'variable' field from the xtensa_tlb_entry structure. Add > 'tlb_variable_way' array to the XtensaConfig and use it instead of > removed fields. > > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> > --- > target/xtensa/cpu.h | 3 +-- > target/xtensa/mmu_helper.c | 38 ++++++++++-------------------------- > target/xtensa/overlay_tool.h | 15 ++++++++++++-- > 3 files changed, 24 insertions(+), 32 deletions(-) > > diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h > index 497325466397..24d3f15ea1bf 100644 > --- a/target/xtensa/cpu.h > +++ b/target/xtensa/cpu.h > @@ -316,13 +316,11 @@ typedef struct xtensa_tlb_entry { > uint32_t paddr; > uint8_t asid; > uint8_t attr; > - bool variable; > } xtensa_tlb_entry; > > typedef struct xtensa_tlb { > unsigned nways; > const unsigned way_size[10]; > - bool varway56; > unsigned nrefillentries; > } xtensa_tlb; > > @@ -493,6 +491,7 @@ typedef struct XtensaConfig { > > xtensa_tlb itlb; > xtensa_tlb dtlb; > + bool tlb_variable_way[16]; > > uint32_t mpu_align; > unsigned n_mpu_fg_segments; > diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c > index d9f845e7fb6f..414c2f5ef669 100644 > --- a/target/xtensa/mmu_helper.c > +++ b/target/xtensa/mmu_helper.c > @@ -105,23 +105,19 @@ static uint32_t xtensa_tlb_get_addr_mask(const CPUXtensaState *env, > bool dtlb, uint32_t way) > { > if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) { > - bool varway56 = dtlb ? > - env->config->dtlb.varway56 : > - env->config->itlb.varway56; > - > switch (way) { > case 4: > return 0xfff00000 << get_page_size(env, dtlb, way) * 2; > > case 5: > - if (varway56) { > + if (env->config->tlb_variable_way[5]) { > return 0xf8000000 << get_page_size(env, dtlb, way); > } else { > return 0xf8000000; > } > > case 6: > - if (varway56) { > + if (env->config->tlb_variable_way[6]) { > return 0xf0000000 << (1 - get_page_size(env, dtlb, way)); > } else { > return 0xf0000000; So we now have a tlb_variable_way bool for all 16 possible ways, but the code actually only checks it for ways 5 and 6. Should we have an assertion somewhere that the config doesn't try to set it on ways where it has no effect ? Or is there actually a generic behaviour that would make sense for eg "way 3 is variable-way" that we just don't currently implement? > @@ -150,11 +146,8 @@ static uint32_t get_vpn_mask(const CPUXtensaState *env, bool dtlb, uint32_t way) > return xtensa_tlb_get_addr_mask(env, dtlb, way) << 2; > } else if (way <= 6) { > uint32_t mask = xtensa_tlb_get_addr_mask(env, dtlb, way); > - bool varway56 = dtlb ? > - env->config->dtlb.varway56 : > - env->config->itlb.varway56; > > - if (varway56) { > + if (env->config->tlb_variable_way[5]) { > return mask << (way == 5 ? 2 : 3); > } else { > return mask << 1; This doesn't look right -- this branch of the if-else deals with way == 5 and way == 6, but we're only looking at tlb_variable_way[5]. > @@ -172,10 +165,6 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v, > bool dtlb, uint32_t *vpn, > uint32_t wi, uint32_t *ei) > { > - bool varway56 = dtlb ? > - env->config->dtlb.varway56 : > - env->config->itlb.varway56; > - > if (!dtlb) { > wi &= 7; > } > @@ -195,7 +184,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v, > break; > > case 5: > - if (varway56) { > + if (env->config->tlb_variable_way[5]) { > uint32_t eibase = 27 + get_page_size(env, dtlb, wi); > *ei = (v >> eibase) & 0x3; > } else { > @@ -204,7 +193,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v, > break; > > case 6: > - if (varway56) { > + if (env->config->tlb_variable_way[6]) { > uint32_t eibase = 29 - get_page_size(env, dtlb, wi); > *ei = (v >> eibase) & 0x7; > } else { There's no direct code duplication, but it definitely feels like the logic for "figure out how many bits we're dealing with" is duplicated across these three functions. I think it ought to be possible to have a function (or maybe two) which take account of both the way number and tlb_get_variable_way[] such that all of these three functions then don't need to have a switch on the way or look at tlb_variable_way[] themselves... thanks -- PMM
On Mon, Jan 22, 2024 at 10:42 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote: > > > > Whether TLB ways 5 and 6 are variable is not a property of the TLB > > instance or a TLB entry instance, it's a property of the xtensa core > > configuration. > > Remove 'varway56' field from the xtensa_tlb structure and remove > > 'variable' field from the xtensa_tlb_entry structure. Add > > 'tlb_variable_way' array to the XtensaConfig and use it instead of > > removed fields. > > > > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> > > --- > > target/xtensa/cpu.h | 3 +-- > > target/xtensa/mmu_helper.c | 38 ++++++++++-------------------------- > > target/xtensa/overlay_tool.h | 15 ++++++++++++-- > > 3 files changed, 24 insertions(+), 32 deletions(-) > > > > diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h > > index 497325466397..24d3f15ea1bf 100644 > > --- a/target/xtensa/cpu.h > > +++ b/target/xtensa/cpu.h > > @@ -316,13 +316,11 @@ typedef struct xtensa_tlb_entry { > > uint32_t paddr; > > uint8_t asid; > > uint8_t attr; > > - bool variable; > > } xtensa_tlb_entry; > > > > typedef struct xtensa_tlb { > > unsigned nways; > > const unsigned way_size[10]; > > - bool varway56; > > unsigned nrefillentries; > > } xtensa_tlb; > > > > @@ -493,6 +491,7 @@ typedef struct XtensaConfig { > > > > xtensa_tlb itlb; > > xtensa_tlb dtlb; > > + bool tlb_variable_way[16]; > > > > uint32_t mpu_align; > > unsigned n_mpu_fg_segments; > > diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c > > index d9f845e7fb6f..414c2f5ef669 100644 > > --- a/target/xtensa/mmu_helper.c > > +++ b/target/xtensa/mmu_helper.c > > @@ -105,23 +105,19 @@ static uint32_t xtensa_tlb_get_addr_mask(const CPUXtensaState *env, > > bool dtlb, uint32_t way) > > { > > if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) { > > - bool varway56 = dtlb ? > > - env->config->dtlb.varway56 : > > - env->config->itlb.varway56; > > - > > switch (way) { > > case 4: > > return 0xfff00000 << get_page_size(env, dtlb, way) * 2; > > > > case 5: > > - if (varway56) { > > + if (env->config->tlb_variable_way[5]) { > > return 0xf8000000 << get_page_size(env, dtlb, way); > > } else { > > return 0xf8000000; > > } > > > > case 6: > > - if (varway56) { > > + if (env->config->tlb_variable_way[6]) { > > return 0xf0000000 << (1 - get_page_size(env, dtlb, way)); > > } else { > > return 0xf0000000; > > So we now have a tlb_variable_way bool for all 16 possible > ways, but the code actually only checks it for ways 5 and 6. xtensa_tlb_set_entry checks this for all possible ways. I would say that this is an unfortunate definition of MMU in the xtensa ISA book that uses the variability of the ways 5/6 as a discriminator between MMUv2 and MMUv3. > Should we have an assertion somewhere that the config > doesn't try to set it on ways where it has no effect ? > Or is there actually a generic behaviour that would make > sense for eg "way 3 is variable-way" that we just don't > currently implement? We currently use the TLB structure to implement the following xtensa memory management options: cacheattr, region protection, region translation, MMUv2 and MMUv3. First three only have one variable way, in MMUv2 all ways except 5 and 6 are variable and in MMUv3 all ways are variable. QEMU supports all of it and tlb_variable_way is set properly in all of these cases. > > @@ -150,11 +146,8 @@ static uint32_t get_vpn_mask(const CPUXtensaState *env, bool dtlb, uint32_t way) > > return xtensa_tlb_get_addr_mask(env, dtlb, way) << 2; > > } else if (way <= 6) { > > uint32_t mask = xtensa_tlb_get_addr_mask(env, dtlb, way); > > - bool varway56 = dtlb ? > > - env->config->dtlb.varway56 : > > - env->config->itlb.varway56; > > > > - if (varway56) { > > + if (env->config->tlb_variable_way[5]) { > > return mask << (way == 5 ? 2 : 3); > > } else { > > return mask << 1; > > This doesn't look right -- this branch of the if-else deals > with way == 5 and way == 6, but we're only looking at > tlb_variable_way[5]. Yeah, that's MMUv2 vs MMUv3 check, again. > > @@ -172,10 +165,6 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v, > > bool dtlb, uint32_t *vpn, > > uint32_t wi, uint32_t *ei) > > { > > - bool varway56 = dtlb ? > > - env->config->dtlb.varway56 : > > - env->config->itlb.varway56; > > - > > if (!dtlb) { > > wi &= 7; > > } > > @@ -195,7 +184,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v, > > break; > > > > case 5: > > - if (varway56) { > > + if (env->config->tlb_variable_way[5]) { > > uint32_t eibase = 27 + get_page_size(env, dtlb, wi); > > *ei = (v >> eibase) & 0x3; > > } else { > > @@ -204,7 +193,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v, > > break; > > > > case 6: > > - if (varway56) { > > + if (env->config->tlb_variable_way[6]) { > > uint32_t eibase = 29 - get_page_size(env, dtlb, wi); > > *ei = (v >> eibase) & 0x7; > > } else { > > There's no direct code duplication, but it definitely feels like > the logic for "figure out how many bits we're dealing with" is > duplicated across these three functions. > > I think it ought to be possible to have a function (or maybe two) > which take account of both the way number and tlb_get_variable_way[] > such that all of these three functions then don't need to have > a switch on the way or look at tlb_variable_way[] themselves... Ok, let me have another look at cleaning this up.
diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h index 497325466397..24d3f15ea1bf 100644 --- a/target/xtensa/cpu.h +++ b/target/xtensa/cpu.h @@ -316,13 +316,11 @@ typedef struct xtensa_tlb_entry { uint32_t paddr; uint8_t asid; uint8_t attr; - bool variable; } xtensa_tlb_entry; typedef struct xtensa_tlb { unsigned nways; const unsigned way_size[10]; - bool varway56; unsigned nrefillentries; } xtensa_tlb; @@ -493,6 +491,7 @@ typedef struct XtensaConfig { xtensa_tlb itlb; xtensa_tlb dtlb; + bool tlb_variable_way[16]; uint32_t mpu_align; unsigned n_mpu_fg_segments; diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c index d9f845e7fb6f..414c2f5ef669 100644 --- a/target/xtensa/mmu_helper.c +++ b/target/xtensa/mmu_helper.c @@ -105,23 +105,19 @@ static uint32_t xtensa_tlb_get_addr_mask(const CPUXtensaState *env, bool dtlb, uint32_t way) { if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) { - bool varway56 = dtlb ? - env->config->dtlb.varway56 : - env->config->itlb.varway56; - switch (way) { case 4: return 0xfff00000 << get_page_size(env, dtlb, way) * 2; case 5: - if (varway56) { + if (env->config->tlb_variable_way[5]) { return 0xf8000000 << get_page_size(env, dtlb, way); } else { return 0xf8000000; } case 6: - if (varway56) { + if (env->config->tlb_variable_way[6]) { return 0xf0000000 << (1 - get_page_size(env, dtlb, way)); } else { return 0xf0000000; @@ -150,11 +146,8 @@ static uint32_t get_vpn_mask(const CPUXtensaState *env, bool dtlb, uint32_t way) return xtensa_tlb_get_addr_mask(env, dtlb, way) << 2; } else if (way <= 6) { uint32_t mask = xtensa_tlb_get_addr_mask(env, dtlb, way); - bool varway56 = dtlb ? - env->config->dtlb.varway56 : - env->config->itlb.varway56; - if (varway56) { + if (env->config->tlb_variable_way[5]) { return mask << (way == 5 ? 2 : 3); } else { return mask << 1; @@ -172,10 +165,6 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v, bool dtlb, uint32_t *vpn, uint32_t wi, uint32_t *ei) { - bool varway56 = dtlb ? - env->config->dtlb.varway56 : - env->config->itlb.varway56; - if (!dtlb) { wi &= 7; } @@ -195,7 +184,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v, break; case 5: - if (varway56) { + if (env->config->tlb_variable_way[5]) { uint32_t eibase = 27 + get_page_size(env, dtlb, wi); *ei = (v >> eibase) & 0x3; } else { @@ -204,7 +193,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v, break; case 6: - if (varway56) { + if (env->config->tlb_variable_way[6]) { uint32_t eibase = 29 - get_page_size(env, dtlb, wi); *ei = (v >> eibase) & 0x7; } else { @@ -290,7 +279,7 @@ static void xtensa_tlb_set_entry(CPUXtensaState *env, bool dtlb, xtensa_tlb_entry *entry = xtensa_tlb_get_entry(env, dtlb, wi, ei); if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) { - if (entry->variable) { + if (env->config->tlb_variable_way[wi]) { if (entry->asid) { tlb_flush_page(cs, entry->vaddr); } @@ -338,29 +327,25 @@ static void reset_tlb_mmu_all_ways(CPUXtensaState *env, for (wi = 0; wi < tlb->nways; ++wi) { for (ei = 0; ei < tlb->way_size[wi]; ++ei) { entry[wi][ei].asid = 0; - entry[wi][ei].variable = true; } } } static void reset_tlb_mmu_ways56(CPUXtensaState *env, - const xtensa_tlb *tlb, xtensa_tlb_entry entry[][MAX_TLB_WAY_SIZE]) { - if (!tlb->varway56) { + if (!env->config->tlb_variable_way[5]) { static const xtensa_tlb_entry way5[] = { { .vaddr = 0xd0000000, .paddr = 0, .asid = 1, .attr = 7, - .variable = false, }, { .vaddr = 0xd8000000, .paddr = 0, .asid = 1, .attr = 3, - .variable = false, } }; static const xtensa_tlb_entry way6[] = { @@ -369,13 +354,11 @@ static void reset_tlb_mmu_ways56(CPUXtensaState *env, .paddr = 0xf0000000, .asid = 1, .attr = 7, - .variable = false, }, { .vaddr = 0xf0000000, .paddr = 0xf0000000, .asid = 1, .attr = 3, - .variable = false, } }; memcpy(entry[5], way5, sizeof(way5)); @@ -401,7 +384,6 @@ static void reset_tlb_region_way0(CPUXtensaState *env, entry[0][ei].paddr = ei << 29; entry[0][ei].asid = 1; entry[0][ei].attr = 2; - entry[0][ei].variable = true; } } @@ -414,8 +396,8 @@ void reset_mmu(CPUXtensaState *env) env->mmu.autorefill_idx = 0; reset_tlb_mmu_all_ways(env, &env->config->itlb, env->mmu.itlb); reset_tlb_mmu_all_ways(env, &env->config->dtlb, env->mmu.dtlb); - reset_tlb_mmu_ways56(env, &env->config->itlb, env->mmu.itlb); - reset_tlb_mmu_ways56(env, &env->config->dtlb, env->mmu.dtlb); + reset_tlb_mmu_ways56(env, env->mmu.itlb); + reset_tlb_mmu_ways56(env, env->mmu.dtlb); } else if (xtensa_option_enabled(env->config, XTENSA_OPTION_MPU)) { unsigned i; @@ -521,7 +503,7 @@ void HELPER(itlb)(CPUXtensaState *env, uint32_t v, uint32_t dtlb) if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) { uint32_t wi; xtensa_tlb_entry *entry = get_tlb_entry(env, v, dtlb, &wi); - if (entry && entry->variable && entry->asid) { + if (entry && env->config->tlb_variable_way[wi] && entry->asid) { tlb_flush_page(env_cpu(env), entry->vaddr); entry->asid = 0; } diff --git a/target/xtensa/overlay_tool.h b/target/xtensa/overlay_tool.h index 701c00eed20a..268a7fe1823f 100644 --- a/target/xtensa/overlay_tool.h +++ b/target/xtensa/overlay_tool.h @@ -351,7 +351,6 @@ (refill_way_size), (refill_way_size), \ 4, (way56) ? 4 : 2, (way56) ? 8 : 2, 1, 1, 1, \ }, \ - .varway56 = (way56), \ .nrefillentries = (refill_way_size) * 4, \ } @@ -363,7 +362,19 @@ #define TLB_SECTION \ .itlb = ITLB(XCHAL_HAVE_SPANNING_WAY), \ - .dtlb = DTLB(XCHAL_HAVE_SPANNING_WAY) + .dtlb = DTLB(XCHAL_HAVE_SPANNING_WAY), \ + .tlb_variable_way = { \ + true, \ + XCHAL_HAVE_PTP_MMU, \ + XCHAL_HAVE_PTP_MMU, \ + XCHAL_HAVE_PTP_MMU, \ + XCHAL_HAVE_PTP_MMU, \ + XCHAL_HAVE_PTP_MMU && XCHAL_HAVE_SPANNING_WAY, \ + XCHAL_HAVE_PTP_MMU && XCHAL_HAVE_SPANNING_WAY, \ + XCHAL_HAVE_PTP_MMU, \ + XCHAL_HAVE_PTP_MMU, \ + XCHAL_HAVE_PTP_MMU, \ + } #ifndef XCHAL_SYSROM0_PADDR #define XCHAL_SYSROM0_PADDR 0xfe000000
Whether TLB ways 5 and 6 are variable is not a property of the TLB instance or a TLB entry instance, it's a property of the xtensa core configuration. Remove 'varway56' field from the xtensa_tlb structure and remove 'variable' field from the xtensa_tlb_entry structure. Add 'tlb_variable_way' array to the XtensaConfig and use it instead of removed fields. Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> --- target/xtensa/cpu.h | 3 +-- target/xtensa/mmu_helper.c | 38 ++++++++++-------------------------- target/xtensa/overlay_tool.h | 15 ++++++++++++-- 3 files changed, 24 insertions(+), 32 deletions(-)