[PATCH] powerpc/book3s: Fix flush_tlb cpu_spec hook to take a generic argument.

[PATCH] powerpc/book3s: Fix flush_tlb cpu_spec hook to take a generic argument.

[Mahesh J Salgaonkar]

From: Mahesh Salgaonkar <mahesh@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.

Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/cputable.h   |    2 +
 arch/powerpc/include/asm/mmu-hash64.h |    5 +++
 arch/powerpc/kernel/cpu_setup_power.S |   10 +-----
 arch/powerpc/kernel/cputable.c        |    4 +-
 arch/powerpc/kernel/mce_power.c       |   57 ++++++++++++++++++++++++++++++++-
 arch/powerpc/kvm/book3s_hv_ras.c      |    4 +-
 6 files changed, 67 insertions(+), 15 deletions(-)

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 */
 
 #ifndef __ASSEMBLY__
 
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
@@ -154,11 +150,9 @@ _GLOBAL(__flush_tlb_power7)
 1:	blr
 
 __init_tlb_power8:
-	li	r3,0xc00	/* IS field = 0b11 */
-_GLOBAL(__flush_tlb_power8)
 	li	r6,512
 	mtctr	r6
-	mr	r7,r3		/* IS field */
+	li	r7,0xc00	/* IS field = 0b11 */
 	ptesync
 2:	tlbiel	r7
 	addi	r7,r7,0x1000
diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index 9b6dcaa..5cfffeb 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c
@@ -71,8 +71,8 @@ extern void __restore_cpu_power7(void);
 extern void __setup_cpu_power8(unsigned long offset, struct cpu_spec* spec);
 extern void __restore_cpu_power8(void);
 extern void __restore_cpu_a2(void);
-extern void __flush_tlb_power7(unsigned long inval_selector);
-extern void __flush_tlb_power8(unsigned long inval_selector);
+extern void __flush_tlb_power7(unsigned int action);
+extern void __flush_tlb_power8(unsigned int action);
 extern long __machine_check_early_realmode_p7(struct pt_regs *regs);
 extern long __machine_check_early_realmode_p8(struct pt_regs *regs);
 #endif /* CONFIG_PPC64 */
diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
index aa9aff3..ffe22aa 100644
--- a/arch/powerpc/kernel/mce_power.c
+++ b/arch/powerpc/kernel/mce_power.c
@@ -28,6 +28,59 @@
 #include <asm/mce.h>
 #include <asm/machdep.h>
 
+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;
+	}
+}
+
+/*
+ * 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;
+	}
+}
+
 /* flush SLBs and reload */
 static void flush_and_reload_slb(void)
 {
@@ -79,7 +132,7 @@ static long mce_handle_derror(uint64_t dsisr, uint64_t slb_error_bits)
 	}
 	if (dsisr & P7_DSISR_MC_TLB_MULTIHIT_MFTLB) {
 		if (cur_cpu_spec && cur_cpu_spec->flush_tlb)
-			cur_cpu_spec->flush_tlb(TLBIEL_INVAL_PAGE);
+			cur_cpu_spec->flush_tlb(FLUSH_TLB_ALL);
 		/* reset error bits */
 		dsisr &= ~P7_DSISR_MC_TLB_MULTIHIT_MFTLB;
 	}
@@ -110,7 +163,7 @@ static long mce_handle_common_ierror(uint64_t srr1)
 		break;
 	case P7_SRR1_MC_IFETCH_TLB_MULTIHIT:
 		if (cur_cpu_spec && cur_cpu_spec->flush_tlb) {
-			cur_cpu_spec->flush_tlb(TLBIEL_INVAL_PAGE);
+			cur_cpu_spec->flush_tlb(FLUSH_TLB_ALL);
 			handled = 1;
 		}
 		break;
diff --git a/arch/powerpc/kvm/book3s_hv_ras.c b/arch/powerpc/kvm/book3s_hv_ras.c
index d562c8e..8246090 100644
--- a/arch/powerpc/kvm/book3s_hv_ras.c
+++ b/arch/powerpc/kvm/book3s_hv_ras.c
@@ -84,7 +84,7 @@ static long kvmppc_realmode_mc_power7(struct kvm_vcpu *vcpu)
 		}
 		if (dsisr & DSISR_MC_TLB_MULTI) {
 			if (cur_cpu_spec && cur_cpu_spec->flush_tlb)
-				cur_cpu_spec->flush_tlb(TLBIEL_INVAL_SET_LPID);
+				cur_cpu_spec->flush_tlb(FLUSH_TLB_LPID);
 			dsisr &= ~DSISR_MC_TLB_MULTI;
 		}
 		/* Any other errors we don't understand? */
@@ -102,7 +102,7 @@ static long kvmppc_realmode_mc_power7(struct kvm_vcpu *vcpu)
 		break;
 	case SRR1_MC_IFETCH_TLBMULTI:
 		if (cur_cpu_spec && cur_cpu_spec->flush_tlb)
-			cur_cpu_spec->flush_tlb(TLBIEL_INVAL_SET_LPID);
+			cur_cpu_spec->flush_tlb(FLUSH_TLB_LPID);
 		break;
 	default:
 		handled = 0;

Re: powerpc/book3s: Fix flush_tlb cpu_spec hook to take a generic argument.

[Michael Ellerman]

On Tue, 2014-23-09 at 03:53:54 UTC, Mahesh Salgaonkar wrote:
> From: Mahesh Salgaonkar <mahesh@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.

Hi Mahesh,

It seems that the only detail we are abstracting here is that power7 has 128
TLB sets and power8 has 512. Is that right?

It seems like a lot of code to deal with that one detail.

I guess the other thing it does is cur_cpu_spec->flush_tlb() is only
implemented on power7 and power8, ie. not power6. But it seems that the two
callers can only ever run on power7/power8 anway.

So couldn't we just do:
 - add the number of tlb_sets to cpu_spec
 - use that in flush_tlb()

cheers

Re: powerpc/book3s: Fix flush_tlb cpu_spec hook to take a generic argument.

[Benjamin Herrenschmidt]

On Fri, 2014-10-03 at 17:36 +1000, Michael Ellerman wrote:
> It seems that the only detail we are abstracting here is that power7 has 128
> TLB sets and power8 has 512. Is that right?

Today ...

> It seems like a lot of code to deal with that one detail.
> 
> I guess the other thing it does is cur_cpu_spec->flush_tlb() is only
> implemented on power7 and power8, ie. not power6. But it seems that the two
> callers can only ever run on power7/power8 anway.
> 
> So couldn't we just do:
>  - add the number of tlb_sets to cpu_spec
>  - use that in flush_tlb()

Provided the form of tlbiel we use remains ... things are different from
embedded for example. I've asked Mahesh to add this hook with the idea
somewhat to migrate existing/older code to use it as well, and on older
chips or embedded the TLB flush is quite different.

Cheers,
Ben.

Re: powerpc/book3s: Fix flush_tlb cpu_spec hook to take a generic argument.

[Michael Ellerman]

On Tue, 2014-23-09 at 03:53:54 UTC, Mahesh Salgaonkar wrote:
> From: Mahesh Salgaonkar <mahesh@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

Re: powerpc/book3s: Fix flush_tlb cpu_spec hook to take a generic argument.

[Mahesh Jagannath Salgaonkar]

On 11/29/2014 04:08 AM, Michael Ellerman wrote:
> On Tue, 2014-23-09 at 03:53:54 UTC, Mahesh Salgaonkar wrote:
>> From: Mahesh Salgaonkar <mahesh@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.

Yes. MCE code. Since this patch re-factors the code that takes IS as
direct argument, having a separate fix patch does not make any sense and
would get overwritten by this patch anyway.

> 
>> 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.

Sure, I will move them.

> 
>> 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.

You are right. I should put ptesyncs in _flush_tlb(). Will make this
change in v2. Thanks for catching this.

> 
>> +/*
>> + * 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);
> }
> 

Agree. Will roll out v2 with above suggested changes.

Thanks,
-Mahesh.

Re: powerpc/book3s: Fix flush_tlb cpu_spec hook to take a generic argument.

[Michael Ellerman]

On Tue, 2014-12-02 at 14:31 +0530, Mahesh Jagannath Salgaonkar wrote:
> On 11/29/2014 04:08 AM, Michael Ellerman wrote:
> > On Tue, 2014-23-09 at 03:53:54 UTC, Mahesh Salgaonkar wrote:
> >> From: Mahesh Salgaonkar <mahesh@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.
> 
> Yes. MCE code. Since this patch re-factors the code that takes IS as
> direct argument, having a separate fix patch does not make any sense and
> would get overwritten by this patch anyway.

That's irrelevant.

The fix will go to stable, the refactor will not.

Please do the MCE fix as a separate, preceeding patch.

cheers

Re: powerpc/book3s: Fix flush_tlb cpu_spec hook to take a generic argument.

[Mahesh Jagannath Salgaonkar]

On 12/04/2014 03:05 PM, Michael Ellerman wrote:
> On Tue, 2014-12-02 at 14:31 +0530, Mahesh Jagannath Salgaonkar wrote:
>> On 11/29/2014 04:08 AM, Michael Ellerman wrote:
>>> On Tue, 2014-23-09 at 03:53:54 UTC, Mahesh Salgaonkar wrote:
>>>> From: Mahesh Salgaonkar <mahesh@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.
>>
>> Yes. MCE code. Since this patch re-factors the code that takes IS as
>> direct argument, having a separate fix patch does not make any sense and
>> would get overwritten by this patch anyway.
> 
> That's irrelevant.
> 
> The fix will go to stable, the refactor will not.
> 
> Please do the MCE fix as a separate, preceeding patch.

Done. Sent out a separate fix patch for stable
https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-December/123310.html

Thanks,
-Mahesh.