* [PATCH 0/2] target/xtensa: tidy xtensa memory management @ 2024-01-19 20:46 Max Filippov 2024-01-19 20:46 ` [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures Max Filippov 2024-01-19 20:46 ` [PATCH 2/2] target/xtensa: tidy TLB way variability logic Max Filippov 0 siblings, 2 replies; 8+ messages in thread From: Max Filippov @ 2024-01-19 20:46 UTC (permalink / raw) To: qemu-devel; +Cc: Max Filippov Hello, this series separates xtensa MMU and MPU states and improves variable TLB way logic. Max Filippov (2): target/xtensa: wrap MMU and MPU state into structures target/xtensa: tidy TLB way variability logic target/xtensa/cpu.h | 21 +++++++--- target/xtensa/mmu_helper.c | 74 ++++++++++++++---------------------- target/xtensa/overlay_tool.h | 15 +++++++- 3 files changed, 56 insertions(+), 54 deletions(-) -- 2.39.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures 2024-01-19 20:46 [PATCH 0/2] target/xtensa: tidy xtensa memory management Max Filippov @ 2024-01-19 20:46 ` Max Filippov 2024-01-22 18:29 ` Peter Maydell 2024-01-19 20:46 ` [PATCH 2/2] target/xtensa: tidy TLB way variability logic Max Filippov 1 sibling, 1 reply; 8+ messages in thread From: Max Filippov @ 2024-01-19 20:46 UTC (permalink / raw) To: qemu-devel; +Cc: Max Filippov Make separation of alternative xtensa memory management options state explicit. Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> --- target/xtensa/cpu.h | 18 +++++++++++++---- target/xtensa/mmu_helper.c | 40 +++++++++++++++++++------------------- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h index 8a423706d8c0..497325466397 100644 --- a/target/xtensa/cpu.h +++ b/target/xtensa/cpu.h @@ -326,11 +326,21 @@ typedef struct xtensa_tlb { unsigned nrefillentries; } xtensa_tlb; +typedef struct XtensaMMU { + xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE]; + xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE]; + unsigned autorefill_idx; +} XtensaMMU; + typedef struct xtensa_mpu_entry { uint32_t vaddr; uint32_t attr; } xtensa_mpu_entry; +typedef struct XtensaMPU { + xtensa_mpu_entry fg[MAX_MPU_FOREGROUND_SEGMENTS]; +} XtensaMPU; + typedef struct XtensaGdbReg { int targno; unsigned flags; @@ -526,10 +536,10 @@ struct CPUArchState { uint32_t exclusive_val; #ifndef CONFIG_USER_ONLY - xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE]; - xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE]; - xtensa_mpu_entry mpu_fg[MAX_MPU_FOREGROUND_SEGMENTS]; - unsigned autorefill_idx; + union { + XtensaMMU mmu; + XtensaMPU mpu; + }; bool runstall; AddressSpace *address_space_er; MemoryRegion *system_er; diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c index 2fda4e887cce..d9f845e7fb6f 100644 --- a/target/xtensa/mmu_helper.c +++ b/target/xtensa/mmu_helper.c @@ -250,8 +250,8 @@ static xtensa_tlb_entry *xtensa_tlb_get_entry(CPUXtensaState *env, bool dtlb, assert(wi < tlb->nways && ei < tlb->way_size[wi]); return dtlb ? - env->dtlb[wi] + ei : - env->itlb[wi] + ei; + env->mmu.dtlb[wi] + ei : + env->mmu.itlb[wi] + ei; } static xtensa_tlb_entry *get_tlb_entry(CPUXtensaState *env, @@ -411,11 +411,11 @@ void reset_mmu(CPUXtensaState *env) env->sregs[RASID] = 0x04030201; env->sregs[ITLBCFG] = 0; env->sregs[DTLBCFG] = 0; - env->autorefill_idx = 0; - reset_tlb_mmu_all_ways(env, &env->config->itlb, env->itlb); - reset_tlb_mmu_all_ways(env, &env->config->dtlb, env->dtlb); - reset_tlb_mmu_ways56(env, &env->config->itlb, env->itlb); - reset_tlb_mmu_ways56(env, &env->config->dtlb, env->dtlb); + 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); } else if (xtensa_option_enabled(env->config, XTENSA_OPTION_MPU)) { unsigned i; @@ -430,8 +430,8 @@ void reset_mmu(CPUXtensaState *env) } } else { env->sregs[CACHEATTR] = 0x22222222; - reset_tlb_region_way0(env, env->itlb); - reset_tlb_region_way0(env, env->dtlb); + reset_tlb_region_way0(env, env->mmu.itlb); + reset_tlb_region_way0(env, env->mmu.dtlb); } } @@ -462,7 +462,7 @@ static int xtensa_tlb_lookup(const CPUXtensaState *env, const xtensa_tlb *tlb = dtlb ? &env->config->dtlb : &env->config->itlb; const xtensa_tlb_entry (*entry)[MAX_TLB_WAY_SIZE] = dtlb ? - env->dtlb : env->itlb; + env->mmu.dtlb : env->mmu.itlb; int nhits = 0; unsigned wi; @@ -821,7 +821,7 @@ static int get_physical_addr_mmu(CPUXtensaState *env, bool update_tlb, split_tlb_entry_spec_way(env, vaddr, dtlb, &vpn, wi, &ei); if (update_tlb) { - wi = ++env->autorefill_idx & 0x3; + wi = ++env->mmu.autorefill_idx & 0x3; xtensa_tlb_set_entry(env, dtlb, wi, ei, vpn, pte); env->sregs[EXCVADDR] = vaddr; qemu_log_mask(CPU_LOG_MMU, "%s: autorefill(%08x): %08x -> %08x\n", @@ -957,8 +957,8 @@ void HELPER(wptlb)(CPUXtensaState *env, uint32_t p, uint32_t v) unsigned segment = p & XTENSA_MPU_SEGMENT_MASK; if (segment < env->config->n_mpu_fg_segments) { - env->mpu_fg[segment].vaddr = v & -env->config->mpu_align; - env->mpu_fg[segment].attr = p & XTENSA_MPU_ATTR_MASK; + env->mpu.fg[segment].vaddr = v & -env->config->mpu_align; + env->mpu.fg[segment].attr = p & XTENSA_MPU_ATTR_MASK; env->sregs[MPUENB] = deposit32(env->sregs[MPUENB], segment, 1, v); tlb_flush(env_cpu(env)); } @@ -969,7 +969,7 @@ uint32_t HELPER(rptlb0)(CPUXtensaState *env, uint32_t s) unsigned segment = s & XTENSA_MPU_SEGMENT_MASK; if (segment < env->config->n_mpu_fg_segments) { - return env->mpu_fg[segment].vaddr | + return env->mpu.fg[segment].vaddr | extract32(env->sregs[MPUENB], segment, 1); } else { return 0; @@ -981,7 +981,7 @@ uint32_t HELPER(rptlb1)(CPUXtensaState *env, uint32_t s) unsigned segment = s & XTENSA_MPU_SEGMENT_MASK; if (segment < env->config->n_mpu_fg_segments) { - return env->mpu_fg[segment].attr; + return env->mpu.fg[segment].attr; } else { return 0; } @@ -993,13 +993,13 @@ uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v) unsigned segment = XTENSA_MPU_PROBE_B; unsigned bg_segment; - nhits = xtensa_mpu_lookup(env->mpu_fg, env->config->n_mpu_fg_segments, + nhits = xtensa_mpu_lookup(env->mpu.fg, env->config->n_mpu_fg_segments, v, &segment); if (nhits > 1) { HELPER(exception_cause_vaddr)(env, env->pc, LOAD_STORE_TLB_MULTI_HIT_CAUSE, v); } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) { - return env->mpu_fg[segment].attr | segment | XTENSA_MPU_PROBE_V; + return env->mpu.fg[segment].attr | segment | XTENSA_MPU_PROBE_V; } else { xtensa_mpu_lookup(env->config->mpu_bg, env->config->n_mpu_bg_segments, @@ -1017,14 +1017,14 @@ static int get_physical_addr_mpu(CPUXtensaState *env, unsigned segment; uint32_t attr; - nhits = xtensa_mpu_lookup(env->mpu_fg, env->config->n_mpu_fg_segments, + nhits = xtensa_mpu_lookup(env->mpu.fg, env->config->n_mpu_fg_segments, vaddr, &segment); if (nhits > 1) { return is_write < 2 ? LOAD_STORE_TLB_MULTI_HIT_CAUSE : INST_TLB_MULTI_HIT_CAUSE; } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) { - attr = env->mpu_fg[segment].attr; + attr = env->mpu.fg[segment].attr; } else { xtensa_mpu_lookup(env->config->mpu_bg, env->config->n_mpu_bg_segments, @@ -1205,7 +1205,7 @@ void dump_mmu(CPUXtensaState *env) dump_tlb(env, true); } else if (xtensa_option_enabled(env->config, XTENSA_OPTION_MPU)) { qemu_printf("Foreground map:\n"); - dump_mpu(env, env->mpu_fg, env->config->n_mpu_fg_segments); + dump_mpu(env, env->mpu.fg, env->config->n_mpu_fg_segments); qemu_printf("\nBackground map:\n"); dump_mpu(NULL, env->config->mpu_bg, env->config->n_mpu_bg_segments); } else { -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures 2024-01-19 20:46 ` [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures Max Filippov @ 2024-01-22 18:29 ` Peter Maydell 2024-01-22 18:45 ` Max Filippov 0 siblings, 1 reply; 8+ messages in thread From: Peter Maydell @ 2024-01-22 18:29 UTC (permalink / raw) To: Max Filippov; +Cc: qemu-devel On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote: > > Make separation of alternative xtensa memory management options state > explicit. > > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> > --- > target/xtensa/cpu.h | 18 +++++++++++++---- > target/xtensa/mmu_helper.c | 40 +++++++++++++++++++------------------- > 2 files changed, 34 insertions(+), 24 deletions(-) > > diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h > index 8a423706d8c0..497325466397 100644 > --- a/target/xtensa/cpu.h > +++ b/target/xtensa/cpu.h > @@ -326,11 +326,21 @@ typedef struct xtensa_tlb { > unsigned nrefillentries; > } xtensa_tlb; > > +typedef struct XtensaMMU { > + xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE]; > + xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE]; > + unsigned autorefill_idx; > +} XtensaMMU; > + > typedef struct xtensa_mpu_entry { > uint32_t vaddr; > uint32_t attr; > } xtensa_mpu_entry; > > +typedef struct XtensaMPU { > + xtensa_mpu_entry fg[MAX_MPU_FOREGROUND_SEGMENTS]; > +} XtensaMPU; > + > typedef struct XtensaGdbReg { > int targno; > unsigned flags; > @@ -526,10 +536,10 @@ struct CPUArchState { > uint32_t exclusive_val; > > #ifndef CONFIG_USER_ONLY > - xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE]; > - xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE]; > - xtensa_mpu_entry mpu_fg[MAX_MPU_FOREGROUND_SEGMENTS]; > - unsigned autorefill_idx; > + union { > + XtensaMMU mmu; > + XtensaMPU mpu; > + }; Is it really worth having this be a union ? I suspect it will make adding migration/savevm support later more awkward. > bool runstall; > AddressSpace *address_space_er; > MemoryRegion *system_er; Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures 2024-01-22 18:29 ` Peter Maydell @ 2024-01-22 18:45 ` Max Filippov 2024-01-22 20:45 ` Peter Maydell 0 siblings, 1 reply; 8+ messages in thread From: Max Filippov @ 2024-01-22 18:45 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-devel On Mon, Jan 22, 2024 at 10:29 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote: > > > > Make separation of alternative xtensa memory management options state > > explicit. > > > > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> > > --- > > target/xtensa/cpu.h | 18 +++++++++++++---- > > target/xtensa/mmu_helper.c | 40 +++++++++++++++++++------------------- > > 2 files changed, 34 insertions(+), 24 deletions(-) > > > > diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h > > index 8a423706d8c0..497325466397 100644 > > --- a/target/xtensa/cpu.h > > +++ b/target/xtensa/cpu.h > > @@ -326,11 +326,21 @@ typedef struct xtensa_tlb { > > unsigned nrefillentries; > > } xtensa_tlb; > > > > +typedef struct XtensaMMU { > > + xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE]; > > + xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE]; > > + unsigned autorefill_idx; > > +} XtensaMMU; > > + > > typedef struct xtensa_mpu_entry { > > uint32_t vaddr; > > uint32_t attr; > > } xtensa_mpu_entry; > > > > +typedef struct XtensaMPU { > > + xtensa_mpu_entry fg[MAX_MPU_FOREGROUND_SEGMENTS]; > > +} XtensaMPU; > > + > > typedef struct XtensaGdbReg { > > int targno; > > unsigned flags; > > @@ -526,10 +536,10 @@ struct CPUArchState { > > uint32_t exclusive_val; > > > > #ifndef CONFIG_USER_ONLY > > - xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE]; > > - xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE]; > > - xtensa_mpu_entry mpu_fg[MAX_MPU_FOREGROUND_SEGMENTS]; > > - unsigned autorefill_idx; > > + union { > > + XtensaMMU mmu; > > + XtensaMPU mpu; > > + }; > > Is it really worth having this be a union ? I suspect it will > make adding migration/savevm support later more awkward. I have a draft implementation of savevm for xtensa and I did this part using subsections with the .needed callback checking whether the MMU or MPU option is enabled in the config. I wonder where the awkwardness is expected. > Otherwise > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Thanks! -- Max ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures 2024-01-22 18:45 ` Max Filippov @ 2024-01-22 20:45 ` Peter Maydell 0 siblings, 0 replies; 8+ messages in thread From: Peter Maydell @ 2024-01-22 20:45 UTC (permalink / raw) To: Max Filippov; +Cc: qemu-devel On Mon, 22 Jan 2024 at 18:45, Max Filippov <jcmvbkbc@gmail.com> wrote: > > On Mon, Jan 22, 2024 at 10:29 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote: > > > + union { > > > + XtensaMMU mmu; > > > + XtensaMPU mpu; > > > + }; > > > > Is it really worth having this be a union ? I suspect it will > > make adding migration/savevm support later more awkward. > > I have a draft implementation of savevm for xtensa and I did this part > using subsections with the .needed callback checking whether the > MMU or MPU option is enabled in the config. I wonder where the > awkwardness is expected. Oh, well if it works that's fine I guess. I was kind of thinking that if you didn't have the union you could avoid having subsections entirely. On Arm we don't try to save space in the CPU struct by using unions, even though some fields are A-profile specific and some are R or M-profile specific. -- PMM ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] target/xtensa: tidy TLB way variability logic 2024-01-19 20:46 [PATCH 0/2] target/xtensa: tidy xtensa memory management Max Filippov 2024-01-19 20:46 ` [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures Max Filippov @ 2024-01-19 20:46 ` Max Filippov 2024-01-22 18:42 ` Peter Maydell 1 sibling, 1 reply; 8+ messages in thread From: Max Filippov @ 2024-01-19 20:46 UTC (permalink / raw) To: qemu-devel; +Cc: Max Filippov 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; @@ -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 -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] target/xtensa: tidy TLB way variability logic 2024-01-19 20:46 ` [PATCH 2/2] target/xtensa: tidy TLB way variability logic Max Filippov @ 2024-01-22 18:42 ` Peter Maydell 2024-01-22 19:11 ` Max Filippov 0 siblings, 1 reply; 8+ messages in thread From: Peter Maydell @ 2024-01-22 18:42 UTC (permalink / raw) To: Max Filippov; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] target/xtensa: tidy TLB way variability logic 2024-01-22 18:42 ` Peter Maydell @ 2024-01-22 19:11 ` Max Filippov 0 siblings, 0 replies; 8+ messages in thread From: Max Filippov @ 2024-01-22 19:11 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-devel 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. -- Thanks. -- Max ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-01-22 20:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-19 20:46 [PATCH 0/2] target/xtensa: tidy xtensa memory management Max Filippov 2024-01-19 20:46 ` [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures Max Filippov 2024-01-22 18:29 ` Peter Maydell 2024-01-22 18:45 ` Max Filippov 2024-01-22 20:45 ` Peter Maydell 2024-01-19 20:46 ` [PATCH 2/2] target/xtensa: tidy TLB way variability logic Max Filippov 2024-01-22 18:42 ` Peter Maydell 2024-01-22 19:11 ` Max Filippov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).