| Message ID | 5a24db3d48babe4e855707a01954b1827712772f.1714606359.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc PPC exception and BookE MMU clean ups | expand |
On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote: > This flag for split instruction/data TLBs is only set for 6xx soft TLB > MMU model and not used otherwise so no need to have a separate flag > for that. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/ppc/pegasos2.c | 2 +- > target/ppc/cpu.h | 1 - > target/ppc/cpu_init.c | 19 +++++-------------- > target/ppc/helper_regs.c | 1 - > target/ppc/mmu_common.c | 10 ++-------- > target/ppc/mmu_helper.c | 12 ++---------- > 6 files changed, 10 insertions(+), 35 deletions(-) > > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c > index 04d6decb2b..dfc6fab180 100644 > --- a/hw/ppc/pegasos2.c > +++ b/hw/ppc/pegasos2.c > @@ -984,7 +984,7 @@ static void *build_fdt(MachineState *machine, int *fdt_size) > cpu->env.icache_line_size); > qemu_fdt_setprop_cell(fdt, cp, "i-cache-line-size", > cpu->env.icache_line_size); > - if (cpu->env.id_tlbs) { > + if (cpu->env.tlb_type == TLB_6XX) { Want to just add the standard comment here? /* 6xx has separate TLBs for instructions and data */ Otherwise looks good Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > qemu_fdt_setprop_cell(fdt, cp, "i-tlb-sets", cpu->env.nb_ways); > qemu_fdt_setprop_cell(fdt, cp, "i-tlb-size", cpu->env.tlb_per_way); > qemu_fdt_setprop_cell(fdt, cp, "d-tlb-sets", cpu->env.nb_ways); > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 0ac55d6b25..21e12a4f0d 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1260,7 +1260,6 @@ struct CPUArchState { > int tlb_per_way; /* Speed-up helper: used to avoid divisions at run time */ > int nb_ways; /* Number of ways in the TLB set */ > int last_way; /* Last used way used to allocate TLB in a LRU way */ > - int id_tlbs; /* If 1, MMU has separated TLBs for instructions & data */ > int nb_pids; /* Number of available PID registers */ > int tlb_type; /* Type of TLB we're dealing with */ > ppc_tlb_t tlb; /* TLB is optional. Allocate them only if needed */ > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > index c11a69fd90..07ad788e54 100644 > --- a/target/ppc/cpu_init.c > +++ b/target/ppc/cpu_init.c > @@ -2117,7 +2117,6 @@ static void init_proc_405(CPUPPCState *env) > #if !defined(CONFIG_USER_ONLY) > env->nb_tlb = 64; > env->nb_ways = 1; > - env->id_tlbs = 0; > env->tlb_type = TLB_EMB; > #endif > init_excp_4xx(env); > @@ -2190,7 +2189,6 @@ static void init_proc_440EP(CPUPPCState *env) > #if !defined(CONFIG_USER_ONLY) > env->nb_tlb = 64; > env->nb_ways = 1; > - env->id_tlbs = 0; > env->tlb_type = TLB_EMB; > #endif > init_excp_BookE(env); > @@ -2288,7 +2286,6 @@ static void init_proc_440GP(CPUPPCState *env) > #if !defined(CONFIG_USER_ONLY) > env->nb_tlb = 64; > env->nb_ways = 1; > - env->id_tlbs = 0; > env->tlb_type = TLB_EMB; > #endif > init_excp_BookE(env); > @@ -2362,7 +2359,6 @@ static void init_proc_440x5(CPUPPCState *env) > #if !defined(CONFIG_USER_ONLY) > env->nb_tlb = 64; > env->nb_ways = 1; > - env->id_tlbs = 0; > env->tlb_type = TLB_EMB; > #endif > init_excp_BookE(env); > @@ -2724,7 +2720,6 @@ static void init_proc_e200(CPUPPCState *env) > #if !defined(CONFIG_USER_ONLY) > env->nb_tlb = 64; > env->nb_ways = 1; > - env->id_tlbs = 0; > env->tlb_type = TLB_EMB; > #endif > init_excp_e200(env, 0xFFFF0000UL); > @@ -2843,7 +2838,6 @@ static void init_proc_e500(CPUPPCState *env, int version) > /* Memory management */ > env->nb_pids = 3; > env->nb_ways = 2; > - env->id_tlbs = 0; > switch (version) { > case fsl_e500v1: > tlbncfg[0] = register_tlbncfg(2, 1, 1, 0, 256); > @@ -6800,20 +6794,17 @@ static void init_ppc_proc(PowerPCCPU *cpu) > } > /* Allocate TLBs buffer when needed */ > #if !defined(CONFIG_USER_ONLY) > - if (env->nb_tlb != 0) { > - int nb_tlb = env->nb_tlb; > - if (env->id_tlbs != 0) { > - nb_tlb *= 2; > - } > + if (env->nb_tlb) { > switch (env->tlb_type) { > case TLB_6XX: > - env->tlb.tlb6 = g_new0(ppc6xx_tlb_t, nb_tlb); > + /* 6xx has separate TLBs for instructions and data hence times 2 */ > + env->tlb.tlb6 = g_new0(ppc6xx_tlb_t, 2 * env->nb_tlb); > break; > case TLB_EMB: > - env->tlb.tlbe = g_new0(ppcemb_tlb_t, nb_tlb); > + env->tlb.tlbe = g_new0(ppcemb_tlb_t, env->nb_tlb); > break; > case TLB_MAS: > - env->tlb.tlbm = g_new0(ppcmas_tlb_t, nb_tlb); > + env->tlb.tlbm = g_new0(ppcmas_tlb_t, env->nb_tlb); > break; > } > /* Pre-compute some useful values */ > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c > index 25258986e3..ed583fe9b3 100644 > --- a/target/ppc/helper_regs.c > +++ b/target/ppc/helper_regs.c > @@ -693,7 +693,6 @@ void register_6xx_7xx_soft_tlb(CPUPPCState *env, int nb_tlbs, int nb_ways) > #if !defined(CONFIG_USER_ONLY) > env->nb_tlb = nb_tlbs; > env->nb_ways = nb_ways; > - env->id_tlbs = 1; > env->tlb_type = TLB_6XX; > spr_register(env, SPR_DMISS, "DMISS", > SPR_NOACCESS, SPR_NOACCESS, > diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c > index 204b8af455..a0b34f9637 100644 > --- a/target/ppc/mmu_common.c > +++ b/target/ppc/mmu_common.c > @@ -130,8 +130,8 @@ int ppc6xx_tlb_getnum(CPUPPCState *env, target_ulong eaddr, > nr = (eaddr >> TARGET_PAGE_BITS) & (env->tlb_per_way - 1); > /* Select TLB way */ > nr += env->tlb_per_way * way; > - /* 6xx have separate TLBs for instructions and data */ > - if (is_code && env->id_tlbs == 1) { > + /* 6xx has separate TLBs for instructions and data */ > + if (is_code) { > nr += env->nb_tlb; > } > > @@ -1246,13 +1246,7 @@ static void mmu6xx_dump_mmu(CPUPPCState *env) > mmu6xx_dump_BATs(env, ACCESS_INT); > mmu6xx_dump_BATs(env, ACCESS_CODE); > > - if (env->id_tlbs != 1) { > - qemu_printf("ERROR: 6xx MMU should have separated TLB" > - " for code and data\n"); > - } > - > qemu_printf("\nTLBs [EPN EPN + SIZE]\n"); > - > for (type = 0; type < 2; type++) { > for (way = 0; way < env->nb_ways; way++) { > for (entry = env->nb_tlb * type + env->tlb_per_way * way; > diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c > index 817836b731..87c611888b 100644 > --- a/target/ppc/mmu_helper.c > +++ b/target/ppc/mmu_helper.c > @@ -44,14 +44,8 @@ > static inline void ppc6xx_tlb_invalidate_all(CPUPPCState *env) > { > ppc6xx_tlb_t *tlb; > - int nr, max; > + int nr, max = 2 * env->nb_tlb; > > - /* LOG_SWTLB("Invalidate all TLBs\n"); */ > - /* Invalidate all defined software TLB */ > - max = env->nb_tlb; > - if (env->id_tlbs == 1) { > - max *= 2; > - } > for (nr = 0; nr < max; nr++) { > tlb = &env->tlb.tlb6[nr]; > pte_invalidate(&tlb->pte0); > @@ -307,9 +301,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr) > switch (env->mmu_model) { > case POWERPC_MMU_SOFT_6xx: > ppc6xx_tlb_invalidate_virt(env, addr, 0); > - if (env->id_tlbs == 1) { > - ppc6xx_tlb_invalidate_virt(env, addr, 1); > - } > + ppc6xx_tlb_invalidate_virt(env, addr, 1); > break; > case POWERPC_MMU_32B: > /*
On Tue, 7 May 2024, Nicholas Piggin wrote: > On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote: >> This flag for split instruction/data TLBs is only set for 6xx soft TLB >> MMU model and not used otherwise so no need to have a separate flag >> for that. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/ppc/pegasos2.c | 2 +- >> target/ppc/cpu.h | 1 - >> target/ppc/cpu_init.c | 19 +++++-------------- >> target/ppc/helper_regs.c | 1 - >> target/ppc/mmu_common.c | 10 ++-------- >> target/ppc/mmu_helper.c | 12 ++---------- >> 6 files changed, 10 insertions(+), 35 deletions(-) >> >> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c >> index 04d6decb2b..dfc6fab180 100644 >> --- a/hw/ppc/pegasos2.c >> +++ b/hw/ppc/pegasos2.c >> @@ -984,7 +984,7 @@ static void *build_fdt(MachineState *machine, int *fdt_size) >> cpu->env.icache_line_size); >> qemu_fdt_setprop_cell(fdt, cp, "i-cache-line-size", >> cpu->env.icache_line_size); >> - if (cpu->env.id_tlbs) { >> + if (cpu->env.tlb_type == TLB_6XX) { > > Want to just add the standard comment here? > > /* 6xx has separate TLBs for instructions and data */ I think that comment would be redundant here because it's clear from the i-tlb, d-tlb this adds so I can do without a comment in this machine if you don't mind. (If this was not in my machine I would not mind adding a comment but I'd keep this one simple.) I think comments should only be added for things that are not clear from code. Regards, BALATON Zoltan > Otherwise looks good > > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > >> qemu_fdt_setprop_cell(fdt, cp, "i-tlb-sets", cpu->env.nb_ways); >> qemu_fdt_setprop_cell(fdt, cp, "i-tlb-size", cpu->env.tlb_per_way); >> qemu_fdt_setprop_cell(fdt, cp, "d-tlb-sets", cpu->env.nb_ways);
On Wed May 8, 2024 at 2:02 AM AEST, BALATON Zoltan wrote: > On Tue, 7 May 2024, Nicholas Piggin wrote: > > On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote: > >> This flag for split instruction/data TLBs is only set for 6xx soft TLB > >> MMU model and not used otherwise so no need to have a separate flag > >> for that. > >> > >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > >> --- > >> hw/ppc/pegasos2.c | 2 +- > >> target/ppc/cpu.h | 1 - > >> target/ppc/cpu_init.c | 19 +++++-------------- > >> target/ppc/helper_regs.c | 1 - > >> target/ppc/mmu_common.c | 10 ++-------- > >> target/ppc/mmu_helper.c | 12 ++---------- > >> 6 files changed, 10 insertions(+), 35 deletions(-) > >> > >> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c > >> index 04d6decb2b..dfc6fab180 100644 > >> --- a/hw/ppc/pegasos2.c > >> +++ b/hw/ppc/pegasos2.c > >> @@ -984,7 +984,7 @@ static void *build_fdt(MachineState *machine, int *fdt_size) > >> cpu->env.icache_line_size); > >> qemu_fdt_setprop_cell(fdt, cp, "i-cache-line-size", > >> cpu->env.icache_line_size); > >> - if (cpu->env.id_tlbs) { > >> + if (cpu->env.tlb_type == TLB_6XX) { > > > > Want to just add the standard comment here? > > > > /* 6xx has separate TLBs for instructions and data */ > > I think that comment would be redundant here because it's clear from the > i-tlb, d-tlb this adds so I can do without a comment in this machine if > you don't mind. (If this was not in my machine I would not mind adding a > comment but I'd keep this one simple.) I think comments should only be > added for things that are not clear from code. Yes. "Obvious" stuff just builds up until it's not. If you make a simple inline function to test if tlb is split and it can est TLB_6XX then you don't need the comment. Thanks, Nick
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c index 04d6decb2b..dfc6fab180 100644 --- a/hw/ppc/pegasos2.c +++ b/hw/ppc/pegasos2.c @@ -984,7 +984,7 @@ static void *build_fdt(MachineState *machine, int *fdt_size) cpu->env.icache_line_size); qemu_fdt_setprop_cell(fdt, cp, "i-cache-line-size", cpu->env.icache_line_size); - if (cpu->env.id_tlbs) { + if (cpu->env.tlb_type == TLB_6XX) { qemu_fdt_setprop_cell(fdt, cp, "i-tlb-sets", cpu->env.nb_ways); qemu_fdt_setprop_cell(fdt, cp, "i-tlb-size", cpu->env.tlb_per_way); qemu_fdt_setprop_cell(fdt, cp, "d-tlb-sets", cpu->env.nb_ways); diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 0ac55d6b25..21e12a4f0d 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -1260,7 +1260,6 @@ struct CPUArchState { int tlb_per_way; /* Speed-up helper: used to avoid divisions at run time */ int nb_ways; /* Number of ways in the TLB set */ int last_way; /* Last used way used to allocate TLB in a LRU way */ - int id_tlbs; /* If 1, MMU has separated TLBs for instructions & data */ int nb_pids; /* Number of available PID registers */ int tlb_type; /* Type of TLB we're dealing with */ ppc_tlb_t tlb; /* TLB is optional. Allocate them only if needed */ diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c index c11a69fd90..07ad788e54 100644 --- a/target/ppc/cpu_init.c +++ b/target/ppc/cpu_init.c @@ -2117,7 +2117,6 @@ static void init_proc_405(CPUPPCState *env) #if !defined(CONFIG_USER_ONLY) env->nb_tlb = 64; env->nb_ways = 1; - env->id_tlbs = 0; env->tlb_type = TLB_EMB; #endif init_excp_4xx(env); @@ -2190,7 +2189,6 @@ static void init_proc_440EP(CPUPPCState *env) #if !defined(CONFIG_USER_ONLY) env->nb_tlb = 64; env->nb_ways = 1; - env->id_tlbs = 0; env->tlb_type = TLB_EMB; #endif init_excp_BookE(env); @@ -2288,7 +2286,6 @@ static void init_proc_440GP(CPUPPCState *env) #if !defined(CONFIG_USER_ONLY) env->nb_tlb = 64; env->nb_ways = 1; - env->id_tlbs = 0; env->tlb_type = TLB_EMB; #endif init_excp_BookE(env); @@ -2362,7 +2359,6 @@ static void init_proc_440x5(CPUPPCState *env) #if !defined(CONFIG_USER_ONLY) env->nb_tlb = 64; env->nb_ways = 1; - env->id_tlbs = 0; env->tlb_type = TLB_EMB; #endif init_excp_BookE(env); @@ -2724,7 +2720,6 @@ static void init_proc_e200(CPUPPCState *env) #if !defined(CONFIG_USER_ONLY) env->nb_tlb = 64; env->nb_ways = 1; - env->id_tlbs = 0; env->tlb_type = TLB_EMB; #endif init_excp_e200(env, 0xFFFF0000UL); @@ -2843,7 +2838,6 @@ static void init_proc_e500(CPUPPCState *env, int version) /* Memory management */ env->nb_pids = 3; env->nb_ways = 2; - env->id_tlbs = 0; switch (version) { case fsl_e500v1: tlbncfg[0] = register_tlbncfg(2, 1, 1, 0, 256); @@ -6800,20 +6794,17 @@ static void init_ppc_proc(PowerPCCPU *cpu) } /* Allocate TLBs buffer when needed */ #if !defined(CONFIG_USER_ONLY) - if (env->nb_tlb != 0) { - int nb_tlb = env->nb_tlb; - if (env->id_tlbs != 0) { - nb_tlb *= 2; - } + if (env->nb_tlb) { switch (env->tlb_type) { case TLB_6XX: - env->tlb.tlb6 = g_new0(ppc6xx_tlb_t, nb_tlb); + /* 6xx has separate TLBs for instructions and data hence times 2 */ + env->tlb.tlb6 = g_new0(ppc6xx_tlb_t, 2 * env->nb_tlb); break; case TLB_EMB: - env->tlb.tlbe = g_new0(ppcemb_tlb_t, nb_tlb); + env->tlb.tlbe = g_new0(ppcemb_tlb_t, env->nb_tlb); break; case TLB_MAS: - env->tlb.tlbm = g_new0(ppcmas_tlb_t, nb_tlb); + env->tlb.tlbm = g_new0(ppcmas_tlb_t, env->nb_tlb); break; } /* Pre-compute some useful values */ diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c index 25258986e3..ed583fe9b3 100644 --- a/target/ppc/helper_regs.c +++ b/target/ppc/helper_regs.c @@ -693,7 +693,6 @@ void register_6xx_7xx_soft_tlb(CPUPPCState *env, int nb_tlbs, int nb_ways) #if !defined(CONFIG_USER_ONLY) env->nb_tlb = nb_tlbs; env->nb_ways = nb_ways; - env->id_tlbs = 1; env->tlb_type = TLB_6XX; spr_register(env, SPR_DMISS, "DMISS", SPR_NOACCESS, SPR_NOACCESS, diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c index 204b8af455..a0b34f9637 100644 --- a/target/ppc/mmu_common.c +++ b/target/ppc/mmu_common.c @@ -130,8 +130,8 @@ int ppc6xx_tlb_getnum(CPUPPCState *env, target_ulong eaddr, nr = (eaddr >> TARGET_PAGE_BITS) & (env->tlb_per_way - 1); /* Select TLB way */ nr += env->tlb_per_way * way; - /* 6xx have separate TLBs for instructions and data */ - if (is_code && env->id_tlbs == 1) { + /* 6xx has separate TLBs for instructions and data */ + if (is_code) { nr += env->nb_tlb; } @@ -1246,13 +1246,7 @@ static void mmu6xx_dump_mmu(CPUPPCState *env) mmu6xx_dump_BATs(env, ACCESS_INT); mmu6xx_dump_BATs(env, ACCESS_CODE); - if (env->id_tlbs != 1) { - qemu_printf("ERROR: 6xx MMU should have separated TLB" - " for code and data\n"); - } - qemu_printf("\nTLBs [EPN EPN + SIZE]\n"); - for (type = 0; type < 2; type++) { for (way = 0; way < env->nb_ways; way++) { for (entry = env->nb_tlb * type + env->tlb_per_way * way; diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c index 817836b731..87c611888b 100644 --- a/target/ppc/mmu_helper.c +++ b/target/ppc/mmu_helper.c @@ -44,14 +44,8 @@ static inline void ppc6xx_tlb_invalidate_all(CPUPPCState *env) { ppc6xx_tlb_t *tlb; - int nr, max; + int nr, max = 2 * env->nb_tlb; - /* LOG_SWTLB("Invalidate all TLBs\n"); */ - /* Invalidate all defined software TLB */ - max = env->nb_tlb; - if (env->id_tlbs == 1) { - max *= 2; - } for (nr = 0; nr < max; nr++) { tlb = &env->tlb.tlb6[nr]; pte_invalidate(&tlb->pte0); @@ -307,9 +301,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr) switch (env->mmu_model) { case POWERPC_MMU_SOFT_6xx: ppc6xx_tlb_invalidate_virt(env, addr, 0); - if (env->id_tlbs == 1) { - ppc6xx_tlb_invalidate_virt(env, addr, 1); - } + ppc6xx_tlb_invalidate_virt(env, addr, 1); break; case POWERPC_MMU_32B: /*
This flag for split instruction/data TLBs is only set for 6xx soft TLB MMU model and not used otherwise so no need to have a separate flag for that. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/ppc/pegasos2.c | 2 +- target/ppc/cpu.h | 1 - target/ppc/cpu_init.c | 19 +++++-------------- target/ppc/helper_regs.c | 1 - target/ppc/mmu_common.c | 10 ++-------- target/ppc/mmu_helper.c | 12 ++---------- 6 files changed, 10 insertions(+), 35 deletions(-)