All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] powerpc/64s: fix for CPU hotplug vs mm_cpumask bug
@ 2020-11-20  2:57 ` Nicholas Piggin
  0 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-11-20  2:57 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Nicholas Piggin, Aneesh Kumar K.V, Anton Vorontsov,
	Peter Zijlstra, Thomas Gleixner, linux-kernel

This fixes a race in powerpc mm_cpumask code, I hope the core kernel
patch looks okay and we could take it through the powerpc tree with
an ack from someone (Peter or Thomas, perhaps?)

Thanks,
Nick

Nicholas Piggin (2):
  kernel/cpu: add arch override for clear_tasks_mm_cpumask() mm handling
  powerpc/64s: Trim offlined CPUs from mm_cpumasks

 arch/powerpc/include/asm/book3s/64/mmu.h     | 12 ++++++++++++
 arch/powerpc/mm/book3s64/mmu_context.c       | 20 ++++++++++++++++++++
 arch/powerpc/platforms/powermac/smp.c        |  2 ++
 arch/powerpc/platforms/powernv/smp.c         |  3 +++
 arch/powerpc/platforms/pseries/hotplug-cpu.c |  3 +++
 kernel/cpu.c                                 |  6 +++++-
 6 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 0/2] powerpc/64s: fix for CPU hotplug vs mm_cpumask bug
@ 2020-11-20  2:57 ` Nicholas Piggin
  0 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-11-20  2:57 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Peter Zijlstra, Aneesh Kumar K.V, linux-kernel, Nicholas Piggin,
	Anton Vorontsov, Thomas Gleixner

This fixes a race in powerpc mm_cpumask code, I hope the core kernel
patch looks okay and we could take it through the powerpc tree with
an ack from someone (Peter or Thomas, perhaps?)

Thanks,
Nick

Nicholas Piggin (2):
  kernel/cpu: add arch override for clear_tasks_mm_cpumask() mm handling
  powerpc/64s: Trim offlined CPUs from mm_cpumasks

 arch/powerpc/include/asm/book3s/64/mmu.h     | 12 ++++++++++++
 arch/powerpc/mm/book3s64/mmu_context.c       | 20 ++++++++++++++++++++
 arch/powerpc/platforms/powermac/smp.c        |  2 ++
 arch/powerpc/platforms/powernv/smp.c         |  3 +++
 arch/powerpc/platforms/pseries/hotplug-cpu.c |  3 +++
 kernel/cpu.c                                 |  6 +++++-
 6 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/2] kernel/cpu: add arch override for clear_tasks_mm_cpumask() mm handling
  2020-11-20  2:57 ` Nicholas Piggin
@ 2020-11-20  2:57   ` Nicholas Piggin
  -1 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-11-20  2:57 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Nicholas Piggin, Aneesh Kumar K.V, Anton Vorontsov,
	Peter Zijlstra, Thomas Gleixner, linux-kernel

powerpc keeps a counter in the mm which counts bits set in mm_cpumask as
well as other things. This means it can't use generic code to clear bits
out of the mask and doesn't adjust the arch specific counter.

Add an arch override allowing powerpc to use clear_tasks_mm_cpumask().

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 kernel/cpu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6ff2578ecf17..2b8d7a5db383 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -815,6 +815,10 @@ void __init cpuhp_threads_init(void)
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
+#ifndef arch_clear_mm_cpumask_cpu
+#define arch_clear_mm_cpumask_cpu(cpu, mm) cpumask_clear_cpu(cpu, mm_cpumask(mm))
+#endif
+
 /**
  * clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU
  * @cpu: a CPU id
@@ -850,7 +854,7 @@ void clear_tasks_mm_cpumask(int cpu)
 		t = find_lock_task_mm(p);
 		if (!t)
 			continue;
-		cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
+		arch_clear_mm_cpumask_cpu(cpu, t->mm);
 		task_unlock(t);
 	}
 	rcu_read_unlock();
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 1/2] kernel/cpu: add arch override for clear_tasks_mm_cpumask() mm handling
@ 2020-11-20  2:57   ` Nicholas Piggin
  0 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-11-20  2:57 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Peter Zijlstra, Aneesh Kumar K.V, linux-kernel, Nicholas Piggin,
	Anton Vorontsov, Thomas Gleixner

powerpc keeps a counter in the mm which counts bits set in mm_cpumask as
well as other things. This means it can't use generic code to clear bits
out of the mask and doesn't adjust the arch specific counter.

Add an arch override allowing powerpc to use clear_tasks_mm_cpumask().

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 kernel/cpu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6ff2578ecf17..2b8d7a5db383 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -815,6 +815,10 @@ void __init cpuhp_threads_init(void)
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
+#ifndef arch_clear_mm_cpumask_cpu
+#define arch_clear_mm_cpumask_cpu(cpu, mm) cpumask_clear_cpu(cpu, mm_cpumask(mm))
+#endif
+
 /**
  * clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU
  * @cpu: a CPU id
@@ -850,7 +854,7 @@ void clear_tasks_mm_cpumask(int cpu)
 		t = find_lock_task_mm(p);
 		if (!t)
 			continue;
-		cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
+		arch_clear_mm_cpumask_cpu(cpu, t->mm);
 		task_unlock(t);
 	}
 	rcu_read_unlock();
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
  2020-11-20  2:57 ` Nicholas Piggin
@ 2020-11-20  2:57   ` Nicholas Piggin
  -1 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-11-20  2:57 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Nicholas Piggin, Aneesh Kumar K.V, Anton Vorontsov,
	Peter Zijlstra, Thomas Gleixner, linux-kernel

When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
to manage its TLBs.

However the exit_flush_lazy_tlbs() function expects that after
returning, all CPUs (except self) have flushed TLBs for that mm, in
which case TLBIEL can be used for this flush. This breaks for offline
CPUs because they don't get the IPI to flush their TLB. This can lead
to stale translations.

Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
before going offline.

These offlined CPU bits stuck in the cpumask also prevents the cpumask
from being trimmed back to local mode, which means continual broadcast
IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
situation too.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/book3s/64/mmu.h     | 12 ++++++++++++
 arch/powerpc/mm/book3s64/mmu_context.c       | 20 ++++++++++++++++++++
 arch/powerpc/platforms/powermac/smp.c        |  2 ++
 arch/powerpc/platforms/powernv/smp.c         |  3 +++
 arch/powerpc/platforms/pseries/hotplug-cpu.c |  3 +++
 5 files changed, 40 insertions(+)

diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/include/asm/book3s/64/mmu.h
index e0b52940e43c..750918451dd2 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu.h
@@ -242,6 +242,18 @@ extern void radix_init_pseries(void);
 static inline void radix_init_pseries(void) { };
 #endif
 
+#ifdef CONFIG_HOTPLUG_CPU
+#define arch_clear_mm_cpumask_cpu(cpu, mm)				\
+	do {								\
+		if (cpumask_test_cpu(cpu, mm_cpumask(mm))) {		\
+			atomic_dec(&(mm)->context.active_cpus);		\
+			cpumask_clear_cpu(cpu, mm_cpumask(mm));		\
+		}							\
+	} while (0)
+
+void cleanup_cpu_mmu_context(void);
+#endif
+
 static inline int get_user_context(mm_context_t *ctx, unsigned long ea)
 {
 	int index = ea >> MAX_EA_BITS_PER_CONTEXT;
diff --git a/arch/powerpc/mm/book3s64/mmu_context.c b/arch/powerpc/mm/book3s64/mmu_context.c
index 1c54821de7bf..0c8557220ae2 100644
--- a/arch/powerpc/mm/book3s64/mmu_context.c
+++ b/arch/powerpc/mm/book3s64/mmu_context.c
@@ -17,6 +17,7 @@
 #include <linux/export.h>
 #include <linux/gfp.h>
 #include <linux/slab.h>
+#include <linux/cpu.h>
 
 #include <asm/mmu_context.h>
 #include <asm/pgalloc.h>
@@ -307,3 +308,22 @@ void radix__switch_mmu_context(struct mm_struct *prev, struct mm_struct *next)
 	isync();
 }
 #endif
+
+/**
+ * cleanup_cpu_mmu_context - Clean up MMU details for this CPU (newly offlined)
+ *
+ * This clears the CPU from mm_cpumask for all processes, and then flushes the
+ * local TLB to ensure TLB coherency in case the CPU is onlined again.
+ *
+ * KVM guest translations are not necessarily flushed here. If KVM started
+ * using mm_cpumask or the Linux APIs which do, this would have to be resolved.
+ */
+#ifdef CONFIG_HOTPLUG_CPU
+void cleanup_cpu_mmu_context(void)
+{
+	int cpu = smp_processor_id();
+
+	clear_tasks_mm_cpumask(cpu);
+	tlbiel_all();
+}
+#endif
diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index 74ebe664b016..adae2a6712e1 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
 
 	mpic_cpu_set_priority(0xf);
 
+	cleanup_cpu_mmu_context();
+
 	return 0;
 }
 
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index 54c4ba45c7ce..cbb67813cd5d 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -143,6 +143,9 @@ static int pnv_smp_cpu_disable(void)
 		xive_smp_disable_cpu();
 	else
 		xics_migrate_irqs_away();
+
+	cleanup_cpu_mmu_context();
+
 	return 0;
 }
 
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index f2837e33bf5d..a02012f1b04a 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -90,6 +90,9 @@ static int pseries_cpu_disable(void)
 		xive_smp_disable_cpu();
 	else
 		xics_migrate_irqs_away();
+
+	cleanup_cpu_mmu_context();
+
 	return 0;
 }
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
@ 2020-11-20  2:57   ` Nicholas Piggin
  0 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-11-20  2:57 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Peter Zijlstra, Aneesh Kumar K.V, linux-kernel, Nicholas Piggin,
	Anton Vorontsov, Thomas Gleixner

When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
to manage its TLBs.

However the exit_flush_lazy_tlbs() function expects that after
returning, all CPUs (except self) have flushed TLBs for that mm, in
which case TLBIEL can be used for this flush. This breaks for offline
CPUs because they don't get the IPI to flush their TLB. This can lead
to stale translations.

Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
before going offline.

These offlined CPU bits stuck in the cpumask also prevents the cpumask
from being trimmed back to local mode, which means continual broadcast
IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
situation too.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/book3s/64/mmu.h     | 12 ++++++++++++
 arch/powerpc/mm/book3s64/mmu_context.c       | 20 ++++++++++++++++++++
 arch/powerpc/platforms/powermac/smp.c        |  2 ++
 arch/powerpc/platforms/powernv/smp.c         |  3 +++
 arch/powerpc/platforms/pseries/hotplug-cpu.c |  3 +++
 5 files changed, 40 insertions(+)

diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/include/asm/book3s/64/mmu.h
index e0b52940e43c..750918451dd2 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu.h
@@ -242,6 +242,18 @@ extern void radix_init_pseries(void);
 static inline void radix_init_pseries(void) { };
 #endif
 
+#ifdef CONFIG_HOTPLUG_CPU
+#define arch_clear_mm_cpumask_cpu(cpu, mm)				\
+	do {								\
+		if (cpumask_test_cpu(cpu, mm_cpumask(mm))) {		\
+			atomic_dec(&(mm)->context.active_cpus);		\
+			cpumask_clear_cpu(cpu, mm_cpumask(mm));		\
+		}							\
+	} while (0)
+
+void cleanup_cpu_mmu_context(void);
+#endif
+
 static inline int get_user_context(mm_context_t *ctx, unsigned long ea)
 {
 	int index = ea >> MAX_EA_BITS_PER_CONTEXT;
diff --git a/arch/powerpc/mm/book3s64/mmu_context.c b/arch/powerpc/mm/book3s64/mmu_context.c
index 1c54821de7bf..0c8557220ae2 100644
--- a/arch/powerpc/mm/book3s64/mmu_context.c
+++ b/arch/powerpc/mm/book3s64/mmu_context.c
@@ -17,6 +17,7 @@
 #include <linux/export.h>
 #include <linux/gfp.h>
 #include <linux/slab.h>
+#include <linux/cpu.h>
 
 #include <asm/mmu_context.h>
 #include <asm/pgalloc.h>
@@ -307,3 +308,22 @@ void radix__switch_mmu_context(struct mm_struct *prev, struct mm_struct *next)
 	isync();
 }
 #endif
+
+/**
+ * cleanup_cpu_mmu_context - Clean up MMU details for this CPU (newly offlined)
+ *
+ * This clears the CPU from mm_cpumask for all processes, and then flushes the
+ * local TLB to ensure TLB coherency in case the CPU is onlined again.
+ *
+ * KVM guest translations are not necessarily flushed here. If KVM started
+ * using mm_cpumask or the Linux APIs which do, this would have to be resolved.
+ */
+#ifdef CONFIG_HOTPLUG_CPU
+void cleanup_cpu_mmu_context(void)
+{
+	int cpu = smp_processor_id();
+
+	clear_tasks_mm_cpumask(cpu);
+	tlbiel_all();
+}
+#endif
diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index 74ebe664b016..adae2a6712e1 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
 
 	mpic_cpu_set_priority(0xf);
 
+	cleanup_cpu_mmu_context();
+
 	return 0;
 }
 
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index 54c4ba45c7ce..cbb67813cd5d 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -143,6 +143,9 @@ static int pnv_smp_cpu_disable(void)
 		xive_smp_disable_cpu();
 	else
 		xics_migrate_irqs_away();
+
+	cleanup_cpu_mmu_context();
+
 	return 0;
 }
 
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index f2837e33bf5d..a02012f1b04a 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -90,6 +90,9 @@ static int pseries_cpu_disable(void)
 		xive_smp_disable_cpu();
 	else
 		xics_migrate_irqs_away();
+
+	cleanup_cpu_mmu_context();
+
 	return 0;
 }
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] kernel/cpu: add arch override for clear_tasks_mm_cpumask() mm handling
  2020-11-20  2:57   ` Nicholas Piggin
@ 2020-11-20 10:06     ` Peter Zijlstra
  -1 siblings, 0 replies; 18+ messages in thread
From: Peter Zijlstra @ 2020-11-20 10:06 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: linuxppc-dev, Aneesh Kumar K.V, Anton Vorontsov, Thomas Gleixner,
	linux-kernel

On Fri, Nov 20, 2020 at 12:57:56PM +1000, Nicholas Piggin wrote:
> powerpc keeps a counter in the mm which counts bits set in mm_cpumask as
> well as other things. This means it can't use generic code to clear bits
> out of the mask and doesn't adjust the arch specific counter.
> 
> Add an arch override allowing powerpc to use clear_tasks_mm_cpumask().
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Seems reasonable enough..

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  kernel/cpu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 6ff2578ecf17..2b8d7a5db383 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -815,6 +815,10 @@ void __init cpuhp_threads_init(void)
>  }
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> +#ifndef arch_clear_mm_cpumask_cpu
> +#define arch_clear_mm_cpumask_cpu(cpu, mm) cpumask_clear_cpu(cpu, mm_cpumask(mm))
> +#endif
> +
>  /**
>   * clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU
>   * @cpu: a CPU id
> @@ -850,7 +854,7 @@ void clear_tasks_mm_cpumask(int cpu)
>  		t = find_lock_task_mm(p);
>  		if (!t)
>  			continue;
> -		cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
> +		arch_clear_mm_cpumask_cpu(cpu, t->mm);
>  		task_unlock(t);
>  	}
>  	rcu_read_unlock();
> -- 
> 2.23.0
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] kernel/cpu: add arch override for clear_tasks_mm_cpumask() mm handling
@ 2020-11-20 10:06     ` Peter Zijlstra
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Zijlstra @ 2020-11-20 10:06 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: Aneesh Kumar K.V, Thomas Gleixner, linuxppc-dev, linux-kernel,
	Anton Vorontsov

On Fri, Nov 20, 2020 at 12:57:56PM +1000, Nicholas Piggin wrote:
> powerpc keeps a counter in the mm which counts bits set in mm_cpumask as
> well as other things. This means it can't use generic code to clear bits
> out of the mask and doesn't adjust the arch specific counter.
> 
> Add an arch override allowing powerpc to use clear_tasks_mm_cpumask().
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Seems reasonable enough..

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  kernel/cpu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 6ff2578ecf17..2b8d7a5db383 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -815,6 +815,10 @@ void __init cpuhp_threads_init(void)
>  }
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> +#ifndef arch_clear_mm_cpumask_cpu
> +#define arch_clear_mm_cpumask_cpu(cpu, mm) cpumask_clear_cpu(cpu, mm_cpumask(mm))
> +#endif
> +
>  /**
>   * clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU
>   * @cpu: a CPU id
> @@ -850,7 +854,7 @@ void clear_tasks_mm_cpumask(int cpu)
>  		t = find_lock_task_mm(p);
>  		if (!t)
>  			continue;
> -		cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
> +		arch_clear_mm_cpumask_cpu(cpu, t->mm);
>  		task_unlock(t);
>  	}
>  	rcu_read_unlock();
> -- 
> 2.23.0
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
  2020-11-20  2:57   ` Nicholas Piggin
@ 2020-12-10  9:06     ` Geert Uytterhoeven
  -1 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2020-12-10  9:06 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: linuxppc-dev, Aneesh Kumar K.V, Anton Vorontsov, Peter Zijlstra,
	Thomas Gleixner, Linux Kernel Mailing List

Hi Nicholas,

On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
> to manage its TLBs.
>
> However the exit_flush_lazy_tlbs() function expects that after
> returning, all CPUs (except self) have flushed TLBs for that mm, in
> which case TLBIEL can be used for this flush. This breaks for offline
> CPUs because they don't get the IPI to flush their TLB. This can lead
> to stale translations.
>
> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
> before going offline.
>
> These offlined CPU bits stuck in the cpumask also prevents the cpumask
> from being trimmed back to local mode, which means continual broadcast
> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
> situation too.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Thanks for your patch!

> --- a/arch/powerpc/platforms/powermac/smp.c
> +++ b/arch/powerpc/platforms/powermac/smp.c
> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>
>         mpic_cpu_set_priority(0xf);
>
> +       cleanup_cpu_mmu_context();
> +

I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?

arch/powerpc/platforms/powermac/smp.c: error: implicit
declaration of function 'cleanup_cpu_mmu_context'
[-Werror=implicit-function-declaration]:  => 914:2

http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/


>         return 0;
>  }
>
> diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> index 54c4ba45c7ce..cbb67813cd5d 100644
> --- a/arch/powerpc/platforms/powernv/smp.c
> +++ b/arch/powerpc/platforms/powernv/smp.c
> @@ -143,6 +143,9 @@ static int pnv_smp_cpu_disable(void)
>                 xive_smp_disable_cpu();
>         else
>                 xics_migrate_irqs_away();
> +
> +       cleanup_cpu_mmu_context();
> +
>         return 0;
>  }
>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index f2837e33bf5d..a02012f1b04a 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -90,6 +90,9 @@ static int pseries_cpu_disable(void)
>                 xive_smp_disable_cpu();
>         else
>                 xics_migrate_irqs_away();
> +
> +       cleanup_cpu_mmu_context();
> +
>         return 0;
>  }
>
> --
> 2.23.0
>


--
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
@ 2020-12-10  9:06     ` Geert Uytterhoeven
  0 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2020-12-10  9:06 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: Peter Zijlstra, Aneesh Kumar K.V, Linux Kernel Mailing List,
	Anton Vorontsov, Thomas Gleixner, linuxppc-dev

Hi Nicholas,

On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
> to manage its TLBs.
>
> However the exit_flush_lazy_tlbs() function expects that after
> returning, all CPUs (except self) have flushed TLBs for that mm, in
> which case TLBIEL can be used for this flush. This breaks for offline
> CPUs because they don't get the IPI to flush their TLB. This can lead
> to stale translations.
>
> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
> before going offline.
>
> These offlined CPU bits stuck in the cpumask also prevents the cpumask
> from being trimmed back to local mode, which means continual broadcast
> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
> situation too.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Thanks for your patch!

> --- a/arch/powerpc/platforms/powermac/smp.c
> +++ b/arch/powerpc/platforms/powermac/smp.c
> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>
>         mpic_cpu_set_priority(0xf);
>
> +       cleanup_cpu_mmu_context();
> +

I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?

arch/powerpc/platforms/powermac/smp.c: error: implicit
declaration of function 'cleanup_cpu_mmu_context'
[-Werror=implicit-function-declaration]:  => 914:2

http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/


>         return 0;
>  }
>
> diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> index 54c4ba45c7ce..cbb67813cd5d 100644
> --- a/arch/powerpc/platforms/powernv/smp.c
> +++ b/arch/powerpc/platforms/powernv/smp.c
> @@ -143,6 +143,9 @@ static int pnv_smp_cpu_disable(void)
>                 xive_smp_disable_cpu();
>         else
>                 xics_migrate_irqs_away();
> +
> +       cleanup_cpu_mmu_context();
> +
>         return 0;
>  }
>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index f2837e33bf5d..a02012f1b04a 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -90,6 +90,9 @@ static int pseries_cpu_disable(void)
>                 xive_smp_disable_cpu();
>         else
>                 xics_migrate_irqs_away();
> +
> +       cleanup_cpu_mmu_context();
> +
>         return 0;
>  }
>
> --
> 2.23.0
>


--
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
  2020-12-10  9:06     ` Geert Uytterhoeven
@ 2020-12-14  4:15       ` Nicholas Piggin
  -1 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-12-14  4:15 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Aneesh Kumar K.V, Anton Vorontsov, Peter Zijlstra,
	Linux Kernel Mailing List, linuxppc-dev, Thomas Gleixner

Excerpts from Geert Uytterhoeven's message of December 10, 2020 7:06 pm:
> Hi Nicholas,
> 
> On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
>> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
>> to manage its TLBs.
>>
>> However the exit_flush_lazy_tlbs() function expects that after
>> returning, all CPUs (except self) have flushed TLBs for that mm, in
>> which case TLBIEL can be used for this flush. This breaks for offline
>> CPUs because they don't get the IPI to flush their TLB. This can lead
>> to stale translations.
>>
>> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
>> before going offline.
>>
>> These offlined CPU bits stuck in the cpumask also prevents the cpumask
>> from being trimmed back to local mode, which means continual broadcast
>> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
>> situation too.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> Thanks for your patch!
> 
>> --- a/arch/powerpc/platforms/powermac/smp.c
>> +++ b/arch/powerpc/platforms/powermac/smp.c
>> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>>
>>         mpic_cpu_set_priority(0xf);
>>
>> +       cleanup_cpu_mmu_context();
>> +
> 
> I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?
> 
> arch/powerpc/platforms/powermac/smp.c: error: implicit
> declaration of function 'cleanup_cpu_mmu_context'
> [-Werror=implicit-function-declaration]:  => 914:2
> 
> http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/

Hey, yeah it does thanks for catching it. This patch fixes it for me

---
From a9b5ec92ffac975e81c6d7db6ff2b1486b2723f7 Mon Sep 17 00:00:00 2001
From: Nicholas Piggin <npiggin@gmail.com>
Date: Mon, 14 Dec 2020 13:52:39 +1000
Subject: [PATCH] powerpc/32s: Fix cleanup_cpu_mmu_context() compile bug

32s has no tlbiel_all() defined, so just disable the cleanup with a
comment.

Fixes: 01b0f0eae081 ("powerpc/64s: Trim offlined CPUs from mm_cpumasks")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powermac/smp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index adae2a6712e1..66ef5f8f4445 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -911,7 +911,16 @@ static int smp_core99_cpu_disable(void)
 
 	mpic_cpu_set_priority(0xf);
 
+	/*
+	 * Would be nice for consistency if all platforms clear mm_cpumask and
+	 * flush TLBs on unplug, but the TLB invalidation bug described in
+	 * commit 01b0f0eae081 ("powerpc/64s: Trim offlined CPUs from
+	 * mm_cpumasks") only applies to 64s and for now we only have the TLB
+	 * flush code for that platform.
+	 */
+#ifdef CONFIG_PPC64
 	cleanup_cpu_mmu_context();
+#endif
 
 	return 0;
 }
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
@ 2020-12-14  4:15       ` Nicholas Piggin
  0 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-12-14  4:15 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Peter Zijlstra, Aneesh Kumar K.V, Linux Kernel Mailing List,
	Anton Vorontsov, Thomas Gleixner, linuxppc-dev

Excerpts from Geert Uytterhoeven's message of December 10, 2020 7:06 pm:
> Hi Nicholas,
> 
> On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
>> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
>> to manage its TLBs.
>>
>> However the exit_flush_lazy_tlbs() function expects that after
>> returning, all CPUs (except self) have flushed TLBs for that mm, in
>> which case TLBIEL can be used for this flush. This breaks for offline
>> CPUs because they don't get the IPI to flush their TLB. This can lead
>> to stale translations.
>>
>> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
>> before going offline.
>>
>> These offlined CPU bits stuck in the cpumask also prevents the cpumask
>> from being trimmed back to local mode, which means continual broadcast
>> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
>> situation too.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> Thanks for your patch!
> 
>> --- a/arch/powerpc/platforms/powermac/smp.c
>> +++ b/arch/powerpc/platforms/powermac/smp.c
>> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>>
>>         mpic_cpu_set_priority(0xf);
>>
>> +       cleanup_cpu_mmu_context();
>> +
> 
> I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?
> 
> arch/powerpc/platforms/powermac/smp.c: error: implicit
> declaration of function 'cleanup_cpu_mmu_context'
> [-Werror=implicit-function-declaration]:  => 914:2
> 
> http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/

Hey, yeah it does thanks for catching it. This patch fixes it for me

---
From a9b5ec92ffac975e81c6d7db6ff2b1486b2723f7 Mon Sep 17 00:00:00 2001
From: Nicholas Piggin <npiggin@gmail.com>
Date: Mon, 14 Dec 2020 13:52:39 +1000
Subject: [PATCH] powerpc/32s: Fix cleanup_cpu_mmu_context() compile bug

32s has no tlbiel_all() defined, so just disable the cleanup with a
comment.

Fixes: 01b0f0eae081 ("powerpc/64s: Trim offlined CPUs from mm_cpumasks")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powermac/smp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index adae2a6712e1..66ef5f8f4445 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -911,7 +911,16 @@ static int smp_core99_cpu_disable(void)
 
 	mpic_cpu_set_priority(0xf);
 
+	/*
+	 * Would be nice for consistency if all platforms clear mm_cpumask and
+	 * flush TLBs on unplug, but the TLB invalidation bug described in
+	 * commit 01b0f0eae081 ("powerpc/64s: Trim offlined CPUs from
+	 * mm_cpumasks") only applies to 64s and for now we only have the TLB
+	 * flush code for that platform.
+	 */
+#ifdef CONFIG_PPC64
 	cleanup_cpu_mmu_context();
+#endif
 
 	return 0;
 }
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
  2020-12-14  4:15       ` Nicholas Piggin
@ 2020-12-14 10:43         ` Michael Ellerman
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2020-12-14 10:43 UTC (permalink / raw)
  To: Nicholas Piggin, Geert Uytterhoeven
  Cc: Aneesh Kumar K.V, Anton Vorontsov, Peter Zijlstra,
	Linux Kernel Mailing List, linuxppc-dev, Thomas Gleixner

Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Geert Uytterhoeven's message of December 10, 2020 7:06 pm:
>> Hi Nicholas,
>> 
>> On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>>>
>>> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
>>> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
>>> to manage its TLBs.
>>>
>>> However the exit_flush_lazy_tlbs() function expects that after
>>> returning, all CPUs (except self) have flushed TLBs for that mm, in
>>> which case TLBIEL can be used for this flush. This breaks for offline
>>> CPUs because they don't get the IPI to flush their TLB. This can lead
>>> to stale translations.
>>>
>>> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
>>> before going offline.
>>>
>>> These offlined CPU bits stuck in the cpumask also prevents the cpumask
>>> from being trimmed back to local mode, which means continual broadcast
>>> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
>>> situation too.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> 
>> Thanks for your patch!
>> 
>>> --- a/arch/powerpc/platforms/powermac/smp.c
>>> +++ b/arch/powerpc/platforms/powermac/smp.c
>>> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>>>
>>>         mpic_cpu_set_priority(0xf);
>>>
>>> +       cleanup_cpu_mmu_context();
>>> +
>> 
>> I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?
>> 
>> arch/powerpc/platforms/powermac/smp.c: error: implicit
>> declaration of function 'cleanup_cpu_mmu_context'
>> [-Werror=implicit-function-declaration]:  => 914:2
>> 
>> http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/
>
> Hey, yeah it does thanks for catching it. This patch fixes it for me
>
> ---
> From a9b5ec92ffac975e81c6d7db6ff2b1486b2723f7 Mon Sep 17 00:00:00 2001
> From: Nicholas Piggin <npiggin@gmail.com>
> Date: Mon, 14 Dec 2020 13:52:39 +1000
> Subject: [PATCH] powerpc/32s: Fix cleanup_cpu_mmu_context() compile bug
>
> 32s has no tlbiel_all() defined, so just disable the cleanup with a
> comment.

Or what about just:

diff --git a/arch/powerpc/include/asm/book3s/32/mmu-hash.h b/arch/powerpc/include/asm/book3s/32/mmu-hash.h
index 331187661236..685c589e723f 100644
--- a/arch/powerpc/include/asm/book3s/32/mmu-hash.h
+++ b/arch/powerpc/include/asm/book3s/32/mmu-hash.h
@@ -94,6 +94,7 @@ typedef struct {
 } mm_context_t;

 void update_bats(void);
+static inline void cleanup_cpu_mmu_context(void) { };

 /* patch sites */
 extern s32 patch__hash_page_A0, patch__hash_page_A1, patch__hash_page_A2;


cheers


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
@ 2020-12-14 10:43         ` Michael Ellerman
  0 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2020-12-14 10:43 UTC (permalink / raw)
  To: Nicholas Piggin, Geert Uytterhoeven
  Cc: Peter Zijlstra, Aneesh Kumar K.V, Linux Kernel Mailing List,
	Anton Vorontsov, Thomas Gleixner, linuxppc-dev

Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Geert Uytterhoeven's message of December 10, 2020 7:06 pm:
>> Hi Nicholas,
>> 
>> On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>>>
>>> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
>>> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
>>> to manage its TLBs.
>>>
>>> However the exit_flush_lazy_tlbs() function expects that after
>>> returning, all CPUs (except self) have flushed TLBs for that mm, in
>>> which case TLBIEL can be used for this flush. This breaks for offline
>>> CPUs because they don't get the IPI to flush their TLB. This can lead
>>> to stale translations.
>>>
>>> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
>>> before going offline.
>>>
>>> These offlined CPU bits stuck in the cpumask also prevents the cpumask
>>> from being trimmed back to local mode, which means continual broadcast
>>> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
>>> situation too.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> 
>> Thanks for your patch!
>> 
>>> --- a/arch/powerpc/platforms/powermac/smp.c
>>> +++ b/arch/powerpc/platforms/powermac/smp.c
>>> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>>>
>>>         mpic_cpu_set_priority(0xf);
>>>
>>> +       cleanup_cpu_mmu_context();
>>> +
>> 
>> I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?
>> 
>> arch/powerpc/platforms/powermac/smp.c: error: implicit
>> declaration of function 'cleanup_cpu_mmu_context'
>> [-Werror=implicit-function-declaration]:  => 914:2
>> 
>> http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/
>
> Hey, yeah it does thanks for catching it. This patch fixes it for me
>
> ---
> From a9b5ec92ffac975e81c6d7db6ff2b1486b2723f7 Mon Sep 17 00:00:00 2001
> From: Nicholas Piggin <npiggin@gmail.com>
> Date: Mon, 14 Dec 2020 13:52:39 +1000
> Subject: [PATCH] powerpc/32s: Fix cleanup_cpu_mmu_context() compile bug
>
> 32s has no tlbiel_all() defined, so just disable the cleanup with a
> comment.

Or what about just:

diff --git a/arch/powerpc/include/asm/book3s/32/mmu-hash.h b/arch/powerpc/include/asm/book3s/32/mmu-hash.h
index 331187661236..685c589e723f 100644
--- a/arch/powerpc/include/asm/book3s/32/mmu-hash.h
+++ b/arch/powerpc/include/asm/book3s/32/mmu-hash.h
@@ -94,6 +94,7 @@ typedef struct {
 } mm_context_t;

 void update_bats(void);
+static inline void cleanup_cpu_mmu_context(void) { };

 /* patch sites */
 extern s32 patch__hash_page_A0, patch__hash_page_A1, patch__hash_page_A2;


cheers


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
  2020-12-14 10:43         ` Michael Ellerman
@ 2020-12-14 11:04           ` Nicholas Piggin
  -1 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-12-14 11:04 UTC (permalink / raw)
  To: Geert Uytterhoeven, Michael Ellerman
  Cc: Aneesh Kumar K.V, Anton Vorontsov, Peter Zijlstra,
	Linux Kernel Mailing List, linuxppc-dev, Thomas Gleixner

Excerpts from Michael Ellerman's message of December 14, 2020 8:43 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Excerpts from Geert Uytterhoeven's message of December 10, 2020 7:06 pm:
>>> Hi Nicholas,
>>> 
>>> On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>>>>
>>>> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
>>>> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
>>>> to manage its TLBs.
>>>>
>>>> However the exit_flush_lazy_tlbs() function expects that after
>>>> returning, all CPUs (except self) have flushed TLBs for that mm, in
>>>> which case TLBIEL can be used for this flush. This breaks for offline
>>>> CPUs because they don't get the IPI to flush their TLB. This can lead
>>>> to stale translations.
>>>>
>>>> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
>>>> before going offline.
>>>>
>>>> These offlined CPU bits stuck in the cpumask also prevents the cpumask
>>>> from being trimmed back to local mode, which means continual broadcast
>>>> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
>>>> situation too.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> 
>>> Thanks for your patch!
>>> 
>>>> --- a/arch/powerpc/platforms/powermac/smp.c
>>>> +++ b/arch/powerpc/platforms/powermac/smp.c
>>>> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>>>>
>>>>         mpic_cpu_set_priority(0xf);
>>>>
>>>> +       cleanup_cpu_mmu_context();
>>>> +
>>> 
>>> I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?
>>> 
>>> arch/powerpc/platforms/powermac/smp.c: error: implicit
>>> declaration of function 'cleanup_cpu_mmu_context'
>>> [-Werror=implicit-function-declaration]:  => 914:2
>>> 
>>> http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/
>>
>> Hey, yeah it does thanks for catching it. This patch fixes it for me
>>
>> ---
>> From a9b5ec92ffac975e81c6d7db6ff2b1486b2723f7 Mon Sep 17 00:00:00 2001
>> From: Nicholas Piggin <npiggin@gmail.com>
>> Date: Mon, 14 Dec 2020 13:52:39 +1000
>> Subject: [PATCH] powerpc/32s: Fix cleanup_cpu_mmu_context() compile bug
>>
>> 32s has no tlbiel_all() defined, so just disable the cleanup with a
>> comment.
> 
> Or what about just:

That works, I kind of wanted it in there explicit that we don't
clean up on 32s. I don't mind if you prefer this though.

Thanks,
Nick

> 
> diff --git a/arch/powerpc/include/asm/book3s/32/mmu-hash.h b/arch/powerpc/include/asm/book3s/32/mmu-hash.h
> index 331187661236..685c589e723f 100644
> --- a/arch/powerpc/include/asm/book3s/32/mmu-hash.h
> +++ b/arch/powerpc/include/asm/book3s/32/mmu-hash.h
> @@ -94,6 +94,7 @@ typedef struct {
>  } mm_context_t;
> 
>  void update_bats(void);
> +static inline void cleanup_cpu_mmu_context(void) { };
> 
>  /* patch sites */
>  extern s32 patch__hash_page_A0, patch__hash_page_A1, patch__hash_page_A2;
> 
> 
> cheers
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
@ 2020-12-14 11:04           ` Nicholas Piggin
  0 siblings, 0 replies; 18+ messages in thread
From: Nicholas Piggin @ 2020-12-14 11:04 UTC (permalink / raw)
  To: Geert Uytterhoeven, Michael Ellerman
  Cc: Peter Zijlstra, Aneesh Kumar K.V, Linux Kernel Mailing List,
	Anton Vorontsov, Thomas Gleixner, linuxppc-dev

Excerpts from Michael Ellerman's message of December 14, 2020 8:43 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Excerpts from Geert Uytterhoeven's message of December 10, 2020 7:06 pm:
>>> Hi Nicholas,
>>> 
>>> On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>>>>
>>>> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
>>>> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
>>>> to manage its TLBs.
>>>>
>>>> However the exit_flush_lazy_tlbs() function expects that after
>>>> returning, all CPUs (except self) have flushed TLBs for that mm, in
>>>> which case TLBIEL can be used for this flush. This breaks for offline
>>>> CPUs because they don't get the IPI to flush their TLB. This can lead
>>>> to stale translations.
>>>>
>>>> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
>>>> before going offline.
>>>>
>>>> These offlined CPU bits stuck in the cpumask also prevents the cpumask
>>>> from being trimmed back to local mode, which means continual broadcast
>>>> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
>>>> situation too.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> 
>>> Thanks for your patch!
>>> 
>>>> --- a/arch/powerpc/platforms/powermac/smp.c
>>>> +++ b/arch/powerpc/platforms/powermac/smp.c
>>>> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>>>>
>>>>         mpic_cpu_set_priority(0xf);
>>>>
>>>> +       cleanup_cpu_mmu_context();
>>>> +
>>> 
>>> I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?
>>> 
>>> arch/powerpc/platforms/powermac/smp.c: error: implicit
>>> declaration of function 'cleanup_cpu_mmu_context'
>>> [-Werror=implicit-function-declaration]:  => 914:2
>>> 
>>> http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/
>>
>> Hey, yeah it does thanks for catching it. This patch fixes it for me
>>
>> ---
>> From a9b5ec92ffac975e81c6d7db6ff2b1486b2723f7 Mon Sep 17 00:00:00 2001
>> From: Nicholas Piggin <npiggin@gmail.com>
>> Date: Mon, 14 Dec 2020 13:52:39 +1000
>> Subject: [PATCH] powerpc/32s: Fix cleanup_cpu_mmu_context() compile bug
>>
>> 32s has no tlbiel_all() defined, so just disable the cleanup with a
>> comment.
> 
> Or what about just:

That works, I kind of wanted it in there explicit that we don't
clean up on 32s. I don't mind if you prefer this though.

Thanks,
Nick

> 
> diff --git a/arch/powerpc/include/asm/book3s/32/mmu-hash.h b/arch/powerpc/include/asm/book3s/32/mmu-hash.h
> index 331187661236..685c589e723f 100644
> --- a/arch/powerpc/include/asm/book3s/32/mmu-hash.h
> +++ b/arch/powerpc/include/asm/book3s/32/mmu-hash.h
> @@ -94,6 +94,7 @@ typedef struct {
>  } mm_context_t;
> 
>  void update_bats(void);
> +static inline void cleanup_cpu_mmu_context(void) { };
> 
>  /* patch sites */
>  extern s32 patch__hash_page_A0, patch__hash_page_A1, patch__hash_page_A2;
> 
> 
> cheers
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
  2020-12-14 11:04           ` Nicholas Piggin
@ 2020-12-15 10:33             ` Michael Ellerman
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2020-12-15 10:33 UTC (permalink / raw)
  To: Nicholas Piggin, Geert Uytterhoeven
  Cc: Aneesh Kumar K.V, Anton Vorontsov, Peter Zijlstra,
	Linux Kernel Mailing List, linuxppc-dev, Thomas Gleixner

Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Michael Ellerman's message of December 14, 2020 8:43 pm:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> Excerpts from Geert Uytterhoeven's message of December 10, 2020 7:06 pm:
>>>> Hi Nicholas,
>>>> 
>>>> On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>>>>>
>>>>> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
>>>>> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
>>>>> to manage its TLBs.
>>>>>
>>>>> However the exit_flush_lazy_tlbs() function expects that after
>>>>> returning, all CPUs (except self) have flushed TLBs for that mm, in
>>>>> which case TLBIEL can be used for this flush. This breaks for offline
>>>>> CPUs because they don't get the IPI to flush their TLB. This can lead
>>>>> to stale translations.
>>>>>
>>>>> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
>>>>> before going offline.
>>>>>
>>>>> These offlined CPU bits stuck in the cpumask also prevents the cpumask
>>>>> from being trimmed back to local mode, which means continual broadcast
>>>>> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
>>>>> situation too.
>>>>>
>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> 
>>>> Thanks for your patch!
>>>> 
>>>>> --- a/arch/powerpc/platforms/powermac/smp.c
>>>>> +++ b/arch/powerpc/platforms/powermac/smp.c
>>>>> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>>>>>
>>>>>         mpic_cpu_set_priority(0xf);
>>>>>
>>>>> +       cleanup_cpu_mmu_context();
>>>>> +
>>>> 
>>>> I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?
>>>> 
>>>> arch/powerpc/platforms/powermac/smp.c: error: implicit
>>>> declaration of function 'cleanup_cpu_mmu_context'
>>>> [-Werror=implicit-function-declaration]:  => 914:2
>>>> 
>>>> http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/
>>>
>>> Hey, yeah it does thanks for catching it. This patch fixes it for me
>>>
>>> ---
>>> From a9b5ec92ffac975e81c6d7db6ff2b1486b2723f7 Mon Sep 17 00:00:00 2001
>>> From: Nicholas Piggin <npiggin@gmail.com>
>>> Date: Mon, 14 Dec 2020 13:52:39 +1000
>>> Subject: [PATCH] powerpc/32s: Fix cleanup_cpu_mmu_context() compile bug
>>>
>>> 32s has no tlbiel_all() defined, so just disable the cleanup with a
>>> comment.
>> 
>> Or what about just:
>
> That works, I kind of wanted it in there explicit that we don't
> clean up on 32s. I don't mind if you prefer this though.

OK. I'll merge my version because I can do that without needing to merge
master in order to get the broken commit into my tree.

cheers

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks
@ 2020-12-15 10:33             ` Michael Ellerman
  0 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2020-12-15 10:33 UTC (permalink / raw)
  To: Nicholas Piggin, Geert Uytterhoeven
  Cc: Peter Zijlstra, Aneesh Kumar K.V, Linux Kernel Mailing List,
	Anton Vorontsov, Thomas Gleixner, linuxppc-dev

Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Michael Ellerman's message of December 14, 2020 8:43 pm:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> Excerpts from Geert Uytterhoeven's message of December 10, 2020 7:06 pm:
>>>> Hi Nicholas,
>>>> 
>>>> On Fri, Nov 20, 2020 at 4:01 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>>>>>
>>>>> When offlining a CPU, powerpc/64s does not flush TLBs, rather it just
>>>>> leaves the CPU set in mm_cpumasks, so it continues to receive TLBIEs
>>>>> to manage its TLBs.
>>>>>
>>>>> However the exit_flush_lazy_tlbs() function expects that after
>>>>> returning, all CPUs (except self) have flushed TLBs for that mm, in
>>>>> which case TLBIEL can be used for this flush. This breaks for offline
>>>>> CPUs because they don't get the IPI to flush their TLB. This can lead
>>>>> to stale translations.
>>>>>
>>>>> Fix this by clearing the CPU from mm_cpumasks, then flushing all TLBs
>>>>> before going offline.
>>>>>
>>>>> These offlined CPU bits stuck in the cpumask also prevents the cpumask
>>>>> from being trimmed back to local mode, which means continual broadcast
>>>>> IPIs or TLBIEs are needed for TLB flushing. This patch prevents that
>>>>> situation too.
>>>>>
>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> 
>>>> Thanks for your patch!
>>>> 
>>>>> --- a/arch/powerpc/platforms/powermac/smp.c
>>>>> +++ b/arch/powerpc/platforms/powermac/smp.c
>>>>> @@ -911,6 +911,8 @@ static int smp_core99_cpu_disable(void)
>>>>>
>>>>>         mpic_cpu_set_priority(0xf);
>>>>>
>>>>> +       cleanup_cpu_mmu_context();
>>>>> +
>>>> 
>>>> I guess this change broke pmac32_defconfig+SMP in v5.10-rc7?
>>>> 
>>>> arch/powerpc/platforms/powermac/smp.c: error: implicit
>>>> declaration of function 'cleanup_cpu_mmu_context'
>>>> [-Werror=implicit-function-declaration]:  => 914:2
>>>> 
>>>> http://kisskb.ellerman.id.au/kisskb/buildresult/14423174/
>>>
>>> Hey, yeah it does thanks for catching it. This patch fixes it for me
>>>
>>> ---
>>> From a9b5ec92ffac975e81c6d7db6ff2b1486b2723f7 Mon Sep 17 00:00:00 2001
>>> From: Nicholas Piggin <npiggin@gmail.com>
>>> Date: Mon, 14 Dec 2020 13:52:39 +1000
>>> Subject: [PATCH] powerpc/32s: Fix cleanup_cpu_mmu_context() compile bug
>>>
>>> 32s has no tlbiel_all() defined, so just disable the cleanup with a
>>> comment.
>> 
>> Or what about just:
>
> That works, I kind of wanted it in there explicit that we don't
> clean up on 32s. I don't mind if you prefer this though.

OK. I'll merge my version because I can do that without needing to merge
master in order to get the broken commit into my tree.

cheers

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-12-15 10:35 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20  2:57 [PATCH 0/2] powerpc/64s: fix for CPU hotplug vs mm_cpumask bug Nicholas Piggin
2020-11-20  2:57 ` Nicholas Piggin
2020-11-20  2:57 ` [PATCH 1/2] kernel/cpu: add arch override for clear_tasks_mm_cpumask() mm handling Nicholas Piggin
2020-11-20  2:57   ` Nicholas Piggin
2020-11-20 10:06   ` Peter Zijlstra
2020-11-20 10:06     ` Peter Zijlstra
2020-11-20  2:57 ` [PATCH 2/2] powerpc/64s: Trim offlined CPUs from mm_cpumasks Nicholas Piggin
2020-11-20  2:57   ` Nicholas Piggin
2020-12-10  9:06   ` Geert Uytterhoeven
2020-12-10  9:06     ` Geert Uytterhoeven
2020-12-14  4:15     ` Nicholas Piggin
2020-12-14  4:15       ` Nicholas Piggin
2020-12-14 10:43       ` Michael Ellerman
2020-12-14 10:43         ` Michael Ellerman
2020-12-14 11:04         ` Nicholas Piggin
2020-12-14 11:04           ` Nicholas Piggin
2020-12-15 10:33           ` Michael Ellerman
2020-12-15 10:33             ` Michael Ellerman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.