On Tue, 2014-23-09 at 03:53:54 UTC, Mahesh Salgaonkar wrote:
> From: Mahesh Salgaonkar <mah...@linux.vnet.ibm.com>
>
> The flush_tlb hook in cpu_spec was introduced as a generic function hook
> to invalidate TLBs. But the current implementation of flush_tlb hook
> takes IS (invalidation selector) as an argument which is architecture
> dependent. Hence, It is not right to have a generic routine where caller
> has to pass non-generic argument.
>
> This patch fixes this and makes flush_tlb hook as high level API.
>
> The old code used to call flush_tlb hook with IS=0 (single page) resulting
> partial invalidation of TLBs which is not right. This fix now makes
> sure that whole TLB is invalidated to be able to successfully recover from
> TLB and ERAT errors.
Which old code? You mean the MCE code I think. That's a bug fix, so it should
be a separate patch.
> diff --git a/arch/powerpc/include/asm/cputable.h
> b/arch/powerpc/include/asm/cputable.h
> index daa5af9..ae3e74f 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -100,7 +100,7 @@ struct cpu_spec {
> /*
> * Processor specific routine to flush tlbs.
> */
> - void (*flush_tlb)(unsigned long inval_selector);
> + void (*flush_tlb)(unsigned int action);
>
> };
>
> diff --git a/arch/powerpc/include/asm/mmu-hash64.h
> b/arch/powerpc/include/asm/mmu-hash64.h
> index d765144..068ac8b 100644
> --- a/arch/powerpc/include/asm/mmu-hash64.h
> +++ b/arch/powerpc/include/asm/mmu-hash64.h
> @@ -112,6 +112,11 @@
> #define TLBIEL_INVAL_SET_SHIFT 12
>
> #define POWER7_TLB_SETS 128 /* # sets in POWER7 TLB */
> +#define POWER8_TLB_SETS 512 /* # sets in POWER8 TLB */
> +
> +/* TLB flush actions. Used as argument to cpu_spec.flush_tlb() hook */
> +#define FLUSH_TLB_ALL 0 /* invalidate all TLBs */
> +#define FLUSH_TLB_LPID 1 /* invalidate TLBs for current
> LPID */
Now that these are generic actions then they should go in cputable.h with the
flush hook.
> diff --git a/arch/powerpc/kernel/cpu_setup_power.S
> b/arch/powerpc/kernel/cpu_setup_power.S
> index 4673353..9c9b741 100644
> --- a/arch/powerpc/kernel/cpu_setup_power.S
> +++ b/arch/powerpc/kernel/cpu_setup_power.S
> @@ -137,15 +137,11 @@ __init_HFSCR:
> /*
> * Clear the TLB using the specified IS form of tlbiel instruction
> * (invalidate by congruence class). P7 has 128 CCs., P8 has 512.
> - *
> - * r3 = IS field
> */
> __init_tlb_power7:
> - li r3,0xc00 /* IS field = 0b11 */
> -_GLOBAL(__flush_tlb_power7)
> li r6,128
> mtctr r6
> - mr r7,r3 /* IS field */
> + li r7,0xc00 /* IS field = 0b11 */
> ptesync
> 2: tlbiel r7
> addi r7,r7,0x1000
So the current version is:
_GLOBAL(__flush_tlb_power7)
li r6,128
mtctr r6
mr r7,r3 /* IS field */
ptesync
2: tlbiel r7
addi r7,r7,0x1000
bdnz 2b
ptesync
1: blr
ie. a loop preceeded and followed by ptesync.
Your new version is:
> +static void _flush_tlb(uint32_t tlb_set, unsigned long inval_selector)
> +{
> + unsigned long i, rb;
> +
> + rb = inval_selector;
> + for (i = 0; i < tlb_set; i++) {
> + asm volatile("tlbiel %0" : : "r" (rb));
> + rb += 1 << TLBIEL_INVAL_SET_SHIFT;
> + }
> +}
ie. no ptesyncs at all.
But there's no mention of that in the changelog. You need to explain why it is
OK to drop the ptesyncs.
> +/*
> + * Generic routine to flush TLB on power7. This routine is used as
> + * flush_tlb hook in cpu_spec for Power7 processor.
> + *
> + * action => FLUSH_TLB_ALL: Invalidate all TLBs.
> + * FLUSH_TLB_LPID: Invalidate TLB for current LPID.
> + */
> +void __flush_tlb_power7(unsigned int action)
> +{
> + switch (action) {
> + case FLUSH_TLB_ALL:
> + _flush_tlb(POWER7_TLB_SETS, TLBIEL_INVAL_SET);
> + break;
> + case FLUSH_TLB_LPID:
> + _flush_tlb(POWER7_TLB_SETS, TLBIEL_INVAL_SET_LPID);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * Generic routine to flush TLB on power8. This routine is used as
> + * flush_tlb hook in cpu_spec for power8 processor.
> + *
> + * action => FLUSH_TLB_ALL: Invalidate all TLBs.
> + * FLUSH_TLB_LPID: Invalidate TLB for current LPID.
> + */
> +void __flush_tlb_power8(unsigned int action)
> +{
> + switch (action) {
> + case FLUSH_TLB_ALL:
> + _flush_tlb(POWER8_TLB_SETS, TLBIEL_INVAL_SET);
> + break;
> + case FLUSH_TLB_LPID:
> + _flush_tlb(POWER8_TLB_SETS, TLBIEL_INVAL_SET_LPID);
> + break;
> + default:
> + break;
> + }
> +}
How about this:
void flush_tlb_206(unsigned num_sets, unsigned int action)
{
unsigned long rb;
int i;
switch (action) {
case FLUSH_TLB_ALL:
rb = TLBIEL_INVAL_SET;
break;
case FLUSH_TLB_LPID:
rb = TLBIEL_INVAL_SET_LPID;
break;
default:
BUG();
}
for (i = 0; i < num_sets; i++) {
asm volatile("tlbiel %0" : : "r" (rb));
rb += 1 << TLBIEL_INVAL_SET_SHIFT;
}
}
void flush_tlb_power8(unsigned int action)
{
flush_tlb_206(POWER8_TLB_SETS, action);
}
void flush_tlb_power7(unsigned int action)
{
flush_tlb_206(POWER7_TLB_SETS, action);
}
cheers
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
