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 <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.
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.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
