diff mbox

[PATCHv2,06/10] target-ppc: Remove unused mmu models from ppc_tlb_invalidate_one

Message ID 1453889591-30968-7-git-send-email-david@gibson.dropbear.id.au
State New, archived
Headers show

Commit Message

David Gibson Jan. 27, 2016, 10:13 a.m. UTC
ppc_tlb_invalidate_one() has a big switch handling many different MMU
types.  However, most of those branches can never be reached:

It is called from 3 places: from remove_hpte() and h_protect() in
spapr_hcall.c (which always has a 64-bit hash MMU type), and from
helper_tlbie() in mmu_helper.c.

Calls to helper_tlbie() are generated from gen_tlbiel, gen_tlbiel and
gen_tlbiva.  The first two are only used with the PPC_MEM_TLBIE flag,
set only with 32-bit or 64-bit hash MMU models, and gen_tlbiva() is
used only on 440 and 460 models with the BookE mmu model.

These means the exhaustive list of MMU types which may call
ppc_tlb_invalidate_one() is: POWERPC_MMU_SOFT_6xx, POWERPC_MMU_601,
POWERPC_MMU_32B, POWERPC_MMU_SOFT_74xx, POWERPC_MMU_64B, POWERPC_MMU_2_03,
POWERPC_MMU_2_06, POWERPC_MMU_2_07 and POWERPC_MMU_BOOKE.

Clean up by removing logic for all other MMU types from
ppc_tlb_invalidate_one().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/mmu_helper.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

Comments

Laurent Vivier Jan. 27, 2016, 6:06 p.m. UTC | #1
On 27/01/2016 11:13, David Gibson wrote:
> ppc_tlb_invalidate_one() has a big switch handling many different MMU
> types.  However, most of those branches can never be reached:
> 
> It is called from 3 places: from remove_hpte() and h_protect() in
> spapr_hcall.c (which always has a 64-bit hash MMU type), and from
> helper_tlbie() in mmu_helper.c.
> 
> Calls to helper_tlbie() are generated from gen_tlbiel, gen_tlbiel and
> gen_tlbiva.  The first two are only used with the PPC_MEM_TLBIE flag,
> set only with 32-bit or 64-bit hash MMU models, and gen_tlbiva() is
> used only on 440 and 460 models with the BookE mmu model.
> 
> These means the exhaustive list of MMU types which may call
> ppc_tlb_invalidate_one() is: POWERPC_MMU_SOFT_6xx, POWERPC_MMU_601,
> POWERPC_MMU_32B, POWERPC_MMU_SOFT_74xx, POWERPC_MMU_64B, POWERPC_MMU_2_03,
> POWERPC_MMU_2_06, POWERPC_MMU_2_07 and POWERPC_MMU_BOOKE.
> 
> Clean up by removing logic for all other MMU types from
> ppc_tlb_invalidate_one().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target-ppc/mmu_helper.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index c040b17..82ebe5d 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -1971,25 +1971,10 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
>              ppc6xx_tlb_invalidate_virt(env, addr, 1);
>          }
>          break;
> -    case POWERPC_MMU_SOFT_4xx:
> -    case POWERPC_MMU_SOFT_4xx_Z:
> -        ppc4xx_tlb_invalidate_virt(env, addr, env->spr[SPR_40x_PID]);

This function is now called by no one. Perhaps it should move to the
next patch in helper_tlbiva() (according to your comments) ?

> -        break;
> -    case POWERPC_MMU_REAL:
> -        cpu_abort(CPU(cpu), "No TLB for PowerPC 4xx in real mode\n");
> -        break;
> -    case POWERPC_MMU_MPC8xx:
> -        /* XXX: TODO */
> -        cpu_abort(CPU(cpu), "MPC8xx MMU model is not implemented\n");
> -        break;
>      case POWERPC_MMU_BOOKE:
>          /* XXX: TODO */
>          cpu_abort(CPU(cpu), "BookE MMU model is not implemented\n");
>          break;
> -    case POWERPC_MMU_BOOKE206:
> -        /* XXX: TODO */
> -        cpu_abort(CPU(cpu), "BookE 2.06 MMU model is not implemented\n");
> -        break;
>      case POWERPC_MMU_32B:
>      case POWERPC_MMU_601:
>          /* tlbie invalidate TLBs for all segments */
> @@ -2031,9 +2016,8 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
>          break;
>  #endif /* defined(TARGET_PPC64) */
>      default:
> -        /* XXX: TODO */
> -        cpu_abort(CPU(cpu), "Unknown MMU model\n");
> -        break;
> +        /* Should never reach here with other MMU models */
> +        assert(0);
>      }
>  #else
>      ppc_tlb_invalidate_all(env);
>
David Gibson Jan. 27, 2016, 11:47 p.m. UTC | #2
On Wed, Jan 27, 2016 at 07:06:26PM +0100, Laurent Vivier wrote:
> On 27/01/2016 11:13, David Gibson wrote:
> > ppc_tlb_invalidate_one() has a big switch handling many different MMU
> > types.  However, most of those branches can never be reached:
> > 
> > It is called from 3 places: from remove_hpte() and h_protect() in
> > spapr_hcall.c (which always has a 64-bit hash MMU type), and from
> > helper_tlbie() in mmu_helper.c.
> > 
> > Calls to helper_tlbie() are generated from gen_tlbiel, gen_tlbiel and
> > gen_tlbiva.  The first two are only used with the PPC_MEM_TLBIE flag,
> > set only with 32-bit or 64-bit hash MMU models, and gen_tlbiva() is
> > used only on 440 and 460 models with the BookE mmu model.
> > 
> > These means the exhaustive list of MMU types which may call
> > ppc_tlb_invalidate_one() is: POWERPC_MMU_SOFT_6xx, POWERPC_MMU_601,
> > POWERPC_MMU_32B, POWERPC_MMU_SOFT_74xx, POWERPC_MMU_64B, POWERPC_MMU_2_03,
> > POWERPC_MMU_2_06, POWERPC_MMU_2_07 and POWERPC_MMU_BOOKE.
> > 
> > Clean up by removing logic for all other MMU types from
> > ppc_tlb_invalidate_one().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target-ppc/mmu_helper.c | 20 ++------------------
> >  1 file changed, 2 insertions(+), 18 deletions(-)
> > 
> > diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> > index c040b17..82ebe5d 100644
> > --- a/target-ppc/mmu_helper.c
> > +++ b/target-ppc/mmu_helper.c
> > @@ -1971,25 +1971,10 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
> >              ppc6xx_tlb_invalidate_virt(env, addr, 1);
> >          }
> >          break;
> > -    case POWERPC_MMU_SOFT_4xx:
> > -    case POWERPC_MMU_SOFT_4xx_Z:
> > -        ppc4xx_tlb_invalidate_virt(env, addr, env->spr[SPR_40x_PID]);
> 
> This function is now called by no one.

Ah, yes.  Well, actually it was already called by no one, but now it's obvious.

> Perhaps it should move to the
> next patch in helper_tlbiva() (according to your comments) ?

Uh... I'm not exactly sure what you're suggesting.  Moving it to the
next patch doesn't really make sense - this is about the 4xx MMU type
which is *not* the same as the BookE MMU type used on 44x and 46x
(yes, that's confusing - one of the dangers of using an "xx" name).

Hmm.. not sure what to do with this - ppc4xx_tlb_invalidate_virt()
should be removed, but I don't know that it's worth respinning the
whole series just for that.
Benjamin Herrenschmidt Jan. 28, 2016, 4:20 a.m. UTC | #3
On Wed, 2016-01-27 at 21:13 +1100, David Gibson wrote:
> ppc_tlb_invalidate_one() has a big switch handling many different MMU

> types.  However, most of those branches can never be reached:

> 

> It is called from 3 places: from remove_hpte() and h_protect() in

> spapr_hcall.c (which always has a 64-bit hash MMU type), and from

> helper_tlbie() in mmu_helper.c.

> 

> Calls to helper_tlbie() are generated from gen_tlbiel, gen_tlbiel and

> gen_tlbiva.  The first two are only used with the PPC_MEM_TLBIE flag,

> set only with 32-bit or 64-bit hash MMU models, and gen_tlbiva() is

> used only on 440 and 460 models with the BookE mmu model.

> 

> These means the exhaustive list of MMU types which may call

> ppc_tlb_invalidate_one() is: POWERPC_MMU_SOFT_6xx, POWERPC_MMU_601,

> POWERPC_MMU_32B, POWERPC_MMU_SOFT_74xx, POWERPC_MMU_64B,

> POWERPC_MMU_2_03,

> POWERPC_MMU_2_06, POWERPC_MMU_2_07 and POWERPC_MMU_BOOKE.

> 

> Clean up by removing logic for all other MMU types from

> ppc_tlb_invalidate_one().


I would argue to move hash64 out of it as well anyway. First what we do
in there is dumb, but the way I change it with lazy inval differs and
tlbie does provide additional information on server processors that
we would need should we chose to implemented fine grained invalidations
(such as the page size).

In the meantime:

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>


> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

> ---

>  target-ppc/mmu_helper.c | 20 ++------------------

>  1 file changed, 2 insertions(+), 18 deletions(-)

> 

> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c

> index c040b17..82ebe5d 100644

> --- a/target-ppc/mmu_helper.c

> +++ b/target-ppc/mmu_helper.c

> @@ -1971,25 +1971,10 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,

> target_ulong addr)

>              ppc6xx_tlb_invalidate_virt(env, addr, 1);

>          }

>          break;

> -    case POWERPC_MMU_SOFT_4xx:

> -    case POWERPC_MMU_SOFT_4xx_Z:

> -        ppc4xx_tlb_invalidate_virt(env, addr, env-

> >spr[SPR_40x_PID]);

> -        break;

> -    case POWERPC_MMU_REAL:

> -        cpu_abort(CPU(cpu), "No TLB for PowerPC 4xx in real

> mode\n");

> -        break;

> -    case POWERPC_MMU_MPC8xx:

> -        /* XXX: TODO */

> -        cpu_abort(CPU(cpu), "MPC8xx MMU model is not

> implemented\n");

> -        break;

>      case POWERPC_MMU_BOOKE:

>          /* XXX: TODO */

>          cpu_abort(CPU(cpu), "BookE MMU model is not implemented\n");

>          break;

> -    case POWERPC_MMU_BOOKE206:

> -        /* XXX: TODO */

> -        cpu_abort(CPU(cpu), "BookE 2.06 MMU model is not

> implemented\n");

> -        break;

>      case POWERPC_MMU_32B:

>      case POWERPC_MMU_601:

>          /* tlbie invalidate TLBs for all segments */

> @@ -2031,9 +2016,8 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,

> target_ulong addr)

>          break;

>  #endif /* defined(TARGET_PPC64) */

>      default:

> -        /* XXX: TODO */

> -        cpu_abort(CPU(cpu), "Unknown MMU model\n");

> -        break;

> +        /* Should never reach here with other MMU models */

> +        assert(0);

>      }

>  #else

>      ppc_tlb_invalidate_all(env);
David Gibson Jan. 28, 2016, 5:55 a.m. UTC | #4
On Thu, Jan 28, 2016 at 03:20:38PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2016-01-27 at 21:13 +1100, David Gibson wrote:
> > ppc_tlb_invalidate_one() has a big switch handling many different MMU
> > types.  However, most of those branches can never be reached:
> > 
> > It is called from 3 places: from remove_hpte() and h_protect() in
> > spapr_hcall.c (which always has a 64-bit hash MMU type), and from
> > helper_tlbie() in mmu_helper.c.
> > 
> > Calls to helper_tlbie() are generated from gen_tlbiel, gen_tlbiel and
> > gen_tlbiva.  The first two are only used with the PPC_MEM_TLBIE flag,
> > set only with 32-bit or 64-bit hash MMU models, and gen_tlbiva() is
> > used only on 440 and 460 models with the BookE mmu model.
> > 
> > These means the exhaustive list of MMU types which may call
> > ppc_tlb_invalidate_one() is: POWERPC_MMU_SOFT_6xx, POWERPC_MMU_601,
> > POWERPC_MMU_32B, POWERPC_MMU_SOFT_74xx, POWERPC_MMU_64B,
> > POWERPC_MMU_2_03,
> > POWERPC_MMU_2_06, POWERPC_MMU_2_07 and POWERPC_MMU_BOOKE.
> > 
> > Clean up by removing logic for all other MMU types from
> > ppc_tlb_invalidate_one().
> 
> I would argue to move hash64 out of it as well anyway. First what we do
> in there is dumb, but the way I change it with lazy inval differs and
> tlbie does provide additional information on server processors that
> we would need should we chose to implemented fine grained invalidations
> (such as the page size).

I agree, but I didn't want to postpone the current things I'm working
on while I did that.
 
> In the meantime:
> 
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Thomas Huth Jan. 28, 2016, 3:45 p.m. UTC | #5
On 27.01.2016 11:13, David Gibson wrote:
> ppc_tlb_invalidate_one() has a big switch handling many different MMU
> types.  However, most of those branches can never be reached:
> 
> It is called from 3 places: from remove_hpte() and h_protect() in
> spapr_hcall.c (which always has a 64-bit hash MMU type), and from
> helper_tlbie() in mmu_helper.c.
> 
> Calls to helper_tlbie() are generated from gen_tlbiel, gen_tlbiel and
> gen_tlbiva.  The first two are only used with the PPC_MEM_TLBIE flag,
> set only with 32-bit or 64-bit hash MMU models, and gen_tlbiva() is
> used only on 440 and 460 models with the BookE mmu model.
> 
> These means the exhaustive list of MMU types which may call
> ppc_tlb_invalidate_one() is: POWERPC_MMU_SOFT_6xx, POWERPC_MMU_601,
> POWERPC_MMU_32B, POWERPC_MMU_SOFT_74xx, POWERPC_MMU_64B, POWERPC_MMU_2_03,
> POWERPC_MMU_2_06, POWERPC_MMU_2_07 and POWERPC_MMU_BOOKE.
> 
> Clean up by removing logic for all other MMU types from
> ppc_tlb_invalidate_one().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
...
> @@ -2031,9 +2016,8 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
>          break;
>  #endif /* defined(TARGET_PPC64) */
>      default:
> -        /* XXX: TODO */
> -        cpu_abort(CPU(cpu), "Unknown MMU model\n");
> -        break;
> +        /* Should never reach here with other MMU models */
> +        assert(0);
>      }

May I suggest to simply use "abort()" instead of "assert(0)" ?

 Thomas
David Gibson Jan. 29, 2016, 2:31 a.m. UTC | #6
On Thu, Jan 28, 2016 at 04:45:21PM +0100, Thomas Huth wrote:
> On 27.01.2016 11:13, David Gibson wrote:
> > ppc_tlb_invalidate_one() has a big switch handling many different MMU
> > types.  However, most of those branches can never be reached:
> > 
> > It is called from 3 places: from remove_hpte() and h_protect() in
> > spapr_hcall.c (which always has a 64-bit hash MMU type), and from
> > helper_tlbie() in mmu_helper.c.
> > 
> > Calls to helper_tlbie() are generated from gen_tlbiel, gen_tlbiel and
> > gen_tlbiva.  The first two are only used with the PPC_MEM_TLBIE flag,
> > set only with 32-bit or 64-bit hash MMU models, and gen_tlbiva() is
> > used only on 440 and 460 models with the BookE mmu model.
> > 
> > These means the exhaustive list of MMU types which may call
> > ppc_tlb_invalidate_one() is: POWERPC_MMU_SOFT_6xx, POWERPC_MMU_601,
> > POWERPC_MMU_32B, POWERPC_MMU_SOFT_74xx, POWERPC_MMU_64B, POWERPC_MMU_2_03,
> > POWERPC_MMU_2_06, POWERPC_MMU_2_07 and POWERPC_MMU_BOOKE.
> > 
> > Clean up by removing logic for all other MMU types from
> > ppc_tlb_invalidate_one().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> ...
> > @@ -2031,9 +2016,8 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
> >          break;
> >  #endif /* defined(TARGET_PPC64) */
> >      default:
> > -        /* XXX: TODO */
> > -        cpu_abort(CPU(cpu), "Unknown MMU model\n");
> > -        break;
> > +        /* Should never reach here with other MMU models */
> > +        assert(0);
> >      }
> 
> May I suggest to simply use "abort()" instead of "assert(0)" ?

I actually prefer assert(0), because it documents that this is really
a "can't happen" rather than just "we can't cope".  It also means it
can be elided with -DNDEBUG.
diff mbox

Patch

diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index c040b17..82ebe5d 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -1971,25 +1971,10 @@  void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
             ppc6xx_tlb_invalidate_virt(env, addr, 1);
         }
         break;
-    case POWERPC_MMU_SOFT_4xx:
-    case POWERPC_MMU_SOFT_4xx_Z:
-        ppc4xx_tlb_invalidate_virt(env, addr, env->spr[SPR_40x_PID]);
-        break;
-    case POWERPC_MMU_REAL:
-        cpu_abort(CPU(cpu), "No TLB for PowerPC 4xx in real mode\n");
-        break;
-    case POWERPC_MMU_MPC8xx:
-        /* XXX: TODO */
-        cpu_abort(CPU(cpu), "MPC8xx MMU model is not implemented\n");
-        break;
     case POWERPC_MMU_BOOKE:
         /* XXX: TODO */
         cpu_abort(CPU(cpu), "BookE MMU model is not implemented\n");
         break;
-    case POWERPC_MMU_BOOKE206:
-        /* XXX: TODO */
-        cpu_abort(CPU(cpu), "BookE 2.06 MMU model is not implemented\n");
-        break;
     case POWERPC_MMU_32B:
     case POWERPC_MMU_601:
         /* tlbie invalidate TLBs for all segments */
@@ -2031,9 +2016,8 @@  void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
         break;
 #endif /* defined(TARGET_PPC64) */
     default:
-        /* XXX: TODO */
-        cpu_abort(CPU(cpu), "Unknown MMU model\n");
-        break;
+        /* Should never reach here with other MMU models */
+        assert(0);
     }
 #else
     ppc_tlb_invalidate_all(env);