| Message ID | 1453889591-30968-7-git-send-email-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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.
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);
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>
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
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 --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);
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(-)