[v2,27/28] target/ppc: Remove id_tlbs flag from CPU env

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(-)

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