Skip to contentSkip to conversation

reworking cause_ipi and adding global doorbell support

RFC PATCH In review Nicholas Piggin 2 participants 11 messages Mar 12, 2017 – Mar 14, 2017

Subsystems (3)
PowerPC Linux for power macintosh Linux for PowerPC embedded PPC85XX
lore
Share & tools
lore

Nicholas Piggin

author reviewer
#0 lore raw

cover reworking cause_ipi and adding global doorbell support

Hi,

Just after the previous two fixes, I would like to propose changing the way we do doorbell vs interrupt controller IPIs, and add support for global doorbells supported by POWER9 in HV mode.

After this, the platform code knows about doorbells and interrupt controller IPIs, rather than they know about each other.

Thanks,
Nick

Nicholas Piggin (2):
powerpc/64s: change the doorbell IPI calling convention powerpc/64s: use global doorbell on POWER9 in HV mode

 arch/powerpc/include/asm/dbell.h       | 38 ++++++++++++++++++++++-----
 arch/powerpc/include/asm/smp.h         |  4 +--
 arch/powerpc/include/asm/xics.h        |  2 +-
 arch/powerpc/kernel/dbell.c            | 47 ++++++++++++++++++++++------------
 arch/powerpc/kernel/smp.c              | 27 ++++++++++---------
 arch/powerpc/platforms/85xx/smp.c      |  9 +------
 arch/powerpc/platforms/powermac/smp.c  |  2 +-
 arch/powerpc/platforms/powernv/smp.c   | 32 +++++++++++++++++------
 arch/powerpc/platforms/pseries/smp.c   | 28 ++++++++------------
 arch/powerpc/sysdev/xics/icp-hv.c      |  2 +-
 arch/powerpc/sysdev/xics/icp-native.c  | 12 +--------
 arch/powerpc/sysdev/xics/icp-opal.c    |  2 +-
 arch/powerpc/sysdev/xics/xics-common.c |  3 ---
 13 files changed, 118 insertions(+), 90 deletions(-)
Signature · 1 line
2.11.0
author reviewer
#1 lore raw

PATCH 1/2 powerpc/64s: change the doorbell IPI calling convention

Change the doorbell calls to know about their msgsnd addressing, rather than have them set a per-cpu target data tag at boot that gets sent to the cause_ipi functions. The data is only used for doorbell IPI functions, so it's backward to arrange it like this.

Have the platform code understand doorbell IPIs, rather than the interrupt controller code understand them.

---
 arch/powerpc/include/asm/dbell.h       | 11 ++++++--
 arch/powerpc/include/asm/smp.h         |  3 +--
 arch/powerpc/include/asm/xics.h        |  2 +-
 arch/powerpc/kernel/dbell.c            | 46 +++++++++++++++++++++++-----------
 arch/powerpc/kernel/smp.c              | 14 +++--------
 arch/powerpc/platforms/85xx/smp.c      |  9 +------
 arch/powerpc/platforms/powermac/smp.c  |  2 +-
 arch/powerpc/platforms/powernv/smp.c   | 16 ++++++------
 arch/powerpc/platforms/pseries/smp.c   | 24 ++++--------------
 arch/powerpc/sysdev/xics/icp-hv.c      |  2 +-
 arch/powerpc/sysdev/xics/icp-native.c  | 12 +--------
 arch/powerpc/sysdev/xics/icp-opal.c    |  2 +-
 arch/powerpc/sysdev/xics/xics-common.c |  3 ---
 13 files changed, 64 insertions(+), 82 deletions(-)
arch/powerpc/include/asm/dbell.h +9 -2
index b9d3b52e2a2c..78b8ba575ec2 100644
@@ -55,9 +55,16 @@ static inline void _ppc_msgsnd(u32 msg)
 
 #endif /* CONFIG_PPC_BOOK3S */
 
-extern void doorbell_cause_ipi(int cpu, unsigned long data);
+extern void global_doorbell_cause_ipi(int cpu);
+extern void core_doorbell_cause_ipi(int cpu);
+extern int __try_core_doorbell_cause_ipi(int cpu);
+static inline int try_core_doorbell_cause_ipi(int cpu)
+{
+	if (!cpu_has_feature(CPU_FTR_DBELL))
+		return 0;
+	return __try_core_doorbell_cause_ipi(cpu);
+}
 extern void doorbell_exception(struct pt_regs *regs);
-extern void doorbell_setup_this_cpu(void);
 
 static inline void ppc_msgsnd(enum ppc_dbell type, u32 flags, u32 tag)
 {
arch/powerpc/include/asm/smp.h +1 -2
index 32db16d2e7ad..0ada12e61fd7 100644
@@ -40,7 +40,7 @@ extern int cpu_to_chip_id(int cpu);
 struct smp_ops_t {
 	void  (*message_pass)(int cpu, int msg);
 #ifdef CONFIG_PPC_SMP_MUXED_IPI
-	void  (*cause_ipi)(int cpu, unsigned long data);
+	void  (*cause_ipi)(int cpu);
 #endif
 	void  (*probe)(void);
 	int   (*kick_cpu)(int nr);

@@ -125,7 +125,6 @@ extern int smp_request_message_ipi(int virq, int message);
 extern const char *smp_ipi_name[];
 
 /* for irq controllers with only a single ipi */
-extern void smp_muxed_ipi_set_data(int cpu, unsigned long data);
 extern void smp_muxed_ipi_message_pass(int cpu, int msg);
 extern void smp_muxed_ipi_set_message(int cpu, int msg);
 extern irqreturn_t smp_ipi_demux(void);
arch/powerpc/include/asm/xics.h +1 -1
index e0b9e576905a..7ce2c3ac2964 100644
@@ -57,7 +57,7 @@ struct icp_ops {
 	void (*teardown_cpu)(void);
 	void (*flush_ipi)(void);
 #ifdef CONFIG_SMP
-	void (*cause_ipi)(int cpu, unsigned long data);
+	void (*cause_ipi)(int cpu);
 	irq_handler_t ipi_action;
 #endif
 };
arch/powerpc/kernel/dbell.c +31 -15

59 lines not on this page · raw message

arch/powerpc/kernel/smp.c +3 -11
index 46f89e66a273..bb882fa1f762 100644
@@ -39,6 +39,7 @@
 #include <asm/irq.h>
 #include <asm/hw_irq.h>
 #include <asm/kvm_ppc.h>
+#include <asm/dbell.h>
 #include <asm/page.h>
 #include <asm/pgtable.h>
 #include <asm/prom.h>

@@ -211,17 +212,9 @@ int smp_request_message_ipi(int virq, int msg)
 #ifdef CONFIG_PPC_SMP_MUXED_IPI
 struct cpu_messages {
 	long messages;			/* current messages */
-	unsigned long data;		/* data for cause ipi */
 };
 static DEFINE_PER_CPU_SHARED_ALIGNED(struct cpu_messages, ipi_message);
 
-void smp_muxed_ipi_set_data(int cpu, unsigned long data)
-{
-	struct cpu_messages *info = &per_cpu(ipi_message, cpu);
-
-	info->data = data;
-}
-
 void smp_muxed_ipi_set_message(int cpu, int msg)
 {
 	struct cpu_messages *info = &per_cpu(ipi_message, cpu);

@@ -236,14 +229,13 @@ void smp_muxed_ipi_set_message(int cpu, int msg)
 
 void smp_muxed_ipi_message_pass(int cpu, int msg)
 {
-	struct cpu_messages *info = &per_cpu(ipi_message, cpu);
-
 	smp_muxed_ipi_set_message(cpu, msg);
+
 	/*
 	 * cause_ipi functions are required to include a full barrier
 	 * before doing whatever causes the IPI.
 	 */
-	smp_ops->cause_ipi(cpu, info->data);
+	smp_ops->cause_ipi(cpu);
 }
 
 #ifdef __BIG_ENDIAN__
arch/powerpc/platforms/85xx/smp.c +1 -8
index 078097a0b09d..5cfc24708021 100644
@@ -461,16 +461,9 @@ static void mpc85xx_smp_machine_kexec(struct kimage *image)
 }
 #endif /* CONFIG_KEXEC_CORE */
 
-static void smp_85xx_basic_setup(int cpu_nr)
-{
-	if (cpu_has_feature(CPU_FTR_DBELL))
-		doorbell_setup_this_cpu();
-}
-
 static void smp_85xx_setup_cpu(int cpu_nr)
 {
 	mpic_setup_this_cpu();
-	smp_85xx_basic_setup(cpu_nr);
 }
 
 void __init mpc85xx_smp_init(void)

@@ -492,7 +485,7 @@ void __init mpc85xx_smp_init(void)
 		 * smp_muxed_ipi_message_pass
 		 */
 		smp_85xx_ops.message_pass = NULL;
-		smp_85xx_ops.cause_ipi = doorbell_cause_ipi;
+		smp_85xx_ops.cause_ipi = global_doorbell_cause_ipi;
 		smp_85xx_ops.probe = NULL;
 	}
arch/powerpc/platforms/powermac/smp.c +1 -1
index 746ca7321b03..a3207cea360e 100644
@@ -172,7 +172,7 @@ static irqreturn_t psurge_ipi_intr(int irq, void *d)
 	return IRQ_HANDLED;
 }
 
-static void smp_psurge_cause_ipi(int cpu, unsigned long data)
+static void smp_psurge_cause_ipi(int cpu)
 {
 	psurge_set_ipi(cpu);
 }
arch/powerpc/platforms/powernv/smp.c +9 -7
index 8b67e1eefb5c..e796c97914c8 100644
@@ -49,11 +49,6 @@ static void pnv_smp_setup_cpu(int cpu)
 {
 	if (cpu != boot_cpuid)
 		xics_setup_cpu();
-
-#ifdef CONFIG_PPC_DOORBELL
-	if (cpu_has_feature(CPU_FTR_DBELL))
-		doorbell_setup_this_cpu();
-#endif
 }
 
 static int pnv_smp_kick_cpu(int nr)

@@ -252,9 +247,16 @@ static int pnv_cpu_bootable(unsigned int nr)
 	return smp_generic_cpu_bootable(nr);
 }
 
+static void pnv_cause_ipi(int cpu)
+{
+	if (try_core_doorbell_cause_ipi(cpu))
+		return;
+	icp_ops->cause_ipi(cpu);
+}
+
 static struct smp_ops_t pnv_smp_ops = {
-	.message_pass	= smp_muxed_ipi_message_pass,
-	.cause_ipi	= NULL,	/* Filled at runtime by xics_smp_probe() */
+	.message_pass	= NULL, /* Use smp_muxed_ipi_message_pass */
+	.cause_ipi	= pnv_cause_ipi,
 	.probe		= xics_smp_probe,
 	.kick_cpu	= pnv_smp_kick_cpu,
 	.setup_cpu	= pnv_smp_setup_cpu,
arch/powerpc/platforms/pseries/smp.c +5 -19
index f6f83aeccaaa..a26b3158555e 100644
@@ -55,11 +55,6 @@
  */
 static cpumask_var_t of_spin_mask;
 
-/*
- * If we multiplex IPI mechanisms, store the appropriate XICS IPI mechanism here
- */
-static void  (*xics_cause_ipi)(int cpu, unsigned long data);
-
 /* Query where a cpu is now.  Return codes #defined in plpar_wrappers.h */
 int smp_query_cpu_stopped(unsigned int pcpu)
 {

@@ -143,8 +138,6 @@ static void smp_setup_cpu(int cpu)
 {
 	if (cpu != boot_cpuid)
 		xics_setup_cpu();
-	if (cpu_has_feature(CPU_FTR_DBELL))
-		doorbell_setup_this_cpu();
 
 	if (firmware_has_feature(FW_FEATURE_SPLPAR))
 		vpa_init(cpu);

@@ -187,28 +180,21 @@ static int smp_pSeries_kick_cpu(int nr)
 	return 0;
 }
 
-/* Only used on systems that support multiple IPI mechanisms */
-static void pSeries_cause_ipi_mux(int cpu, unsigned long data)
+static void smp_pseries_cause_ipi(int cpu)
 {
-	if (cpumask_test_cpu(cpu, cpu_sibling_mask(smp_processor_id())))
-		doorbell_cause_ipi(cpu, data);
-	else
-		xics_cause_ipi(cpu, data);
+	if (try_core_doorbell_cause_ipi(cpu))
+		return;
+	icp_ops->cause_ipi(cpu);
 }
 
 static __init void pSeries_smp_probe(void)
 {
 	xics_smp_probe();
-
-	if (cpu_has_feature(CPU_FTR_DBELL)) {
-		xics_cause_ipi = smp_ops->cause_ipi;
-		smp_ops->cause_ipi = pSeries_cause_ipi_mux;
-	}
 }
 
 static struct smp_ops_t pseries_smp_ops = {
 	.message_pass	= NULL,	/* Use smp_muxed_ipi_message_pass */
-	.cause_ipi	= NULL,	/* Filled at runtime by pSeries_smp_probe() */
+	.cause_ipi	= smp_pseries_cause_ipi,
 	.probe		= pSeries_smp_probe,
 	.kick_cpu	= smp_pSeries_kick_cpu,
 	.setup_cpu	= smp_setup_cpu,
arch/powerpc/sysdev/xics/icp-hv.c +1 -1
index e7fa26c4ff73..bbc839a98c41 100644
@@ -138,7 +138,7 @@ static void icp_hv_set_cpu_priority(unsigned char cppr)
 
 #ifdef CONFIG_SMP
 
-static void icp_hv_cause_ipi(int cpu, unsigned long data)
+static void icp_hv_cause_ipi(int cpu)
 {
 	icp_hv_set_qirr(cpu, IPI_PRIORITY);
 }
arch/powerpc/sysdev/xics/icp-native.c +1 -11
index 8a6a043e239b..f9e2f8b2e3eb 100644
@@ -143,19 +143,9 @@ static unsigned int icp_native_get_irq(void)
 
 #ifdef CONFIG_SMP
 
-static void icp_native_cause_ipi(int cpu, unsigned long data)
+static void icp_native_cause_ipi(int cpu)
 {
 	kvmppc_set_host_ipi(cpu, 1);
-#ifdef CONFIG_PPC_DOORBELL
-	if (cpu_has_feature(CPU_FTR_DBELL)) {
-		if (cpumask_test_cpu(cpu, cpu_sibling_mask(get_cpu()))) {
-			doorbell_cause_ipi(cpu, data);
-			put_cpu();
-			return;
-		}
-		put_cpu();
-	}
-#endif
 	icp_native_set_qirr(cpu, IPI_PRIORITY);
 }
arch/powerpc/sysdev/xics/icp-opal.c +1 -1
index b53f80f0b4d8..c71d2ea42627 100644
@@ -126,7 +126,7 @@ static void icp_opal_eoi(struct irq_data *d)
 
 #ifdef CONFIG_SMP
 
-static void icp_opal_cause_ipi(int cpu, unsigned long data)
+static void icp_opal_cause_ipi(int cpu)
 {
 	int hw_cpu = get_hard_smp_processor_id(cpu);
arch/powerpc/sysdev/xics/xics-common.c +0 -3
index 23efe4e42172..2eb53c12ee6e 100644
@@ -143,9 +143,6 @@ static void xics_request_ipi(void)
 
 void __init xics_smp_probe(void)
 {
-	/* Setup cause_ipi callback  based on which ICP is used */
-	smp_ops->cause_ipi = icp_ops->cause_ipi;
-
 	/* Register all the IPIs */
 	xics_request_ipi();
 }
Signature · 1 line
2.11.0
author reviewer
#2 lore raw

PATCH 2/2 powerpc/64s: use global doorbell on POWER9 in HV mode

In reply to Nicholas Piggin

Callers of global_doorbell and core_doorbell must now be aware of their capabilities and which addressing mode their msgsnd instruction uses, and use the correct one.

---
 arch/powerpc/include/asm/dbell.h     | 41 ++++++++++++++++++++++++++----------
 arch/powerpc/include/asm/smp.h       |  1 +
 arch/powerpc/kernel/dbell.c          | 11 +++++-----
 arch/powerpc/kernel/smp.c            | 13 +++++++++---
 arch/powerpc/platforms/powernv/smp.c | 18 ++++++++++++++--
 arch/powerpc/platforms/pseries/smp.c |  8 ++++++-
 6 files changed, 69 insertions(+), 23 deletions(-)
arch/powerpc/include/asm/dbell.h +30 -11
index 78b8ba575ec2..0ce54014632b 100644
@@ -38,10 +38,24 @@ enum ppc_dbell {
 
 static inline void _ppc_msgsnd(u32 msg)
 {
-	if (cpu_has_feature(CPU_FTR_HVMODE))
-		__asm__ __volatile__ (PPC_MSGSND(%0) : : "r" (msg));
-	else
-		__asm__ __volatile__ (PPC_MSGSNDP(%0) : : "r" (msg));
+	__asm__ __volatile__ (ASM_FTR_IFSET(PPC_MSGSND(%1), PPC_MSGSNDP(%1), %0)
+				: : "i" (CPU_FTR_HVMODE), "r" (msg));
+}
+
+/* sync before sending message */
+static inline void ppc_msgsnd_sync(void)
+{
+	/* Could use lwsync for msgsndp? (does not order non-cacheable) */
+	__asm__ __volatile__ ("sync" : : : "memory");
+}
+
+/* sync after taking message interrupt */
+static inline void ppc_msgsync(void)
+{
+	/* sync is not required when taking messages from the same core */
+	__asm__ __volatile__ (ASM_FTR_IFSET(PPC_MSGSYNC, "", %0)
+				: : "i" (CPU_FTR_ARCH_300|CPU_FTR_HVMODE)
+				: "memory");
 }
 
 #else /* CONFIG_PPC_BOOK3S */

@@ -55,15 +69,20 @@ static inline void _ppc_msgsnd(u32 msg)
 
 #endif /* CONFIG_PPC_BOOK3S */
 
+/*
+ * Doorbells must only be used if CPU_FTR_DBELL is available.
+ *
+ * Global doorbell IPIs must only be used on CPUs that address the msgsnd
+ * (or msgsndp if in HVMODE) with PIR (hard_smp_processor_id)
+ */
 extern void global_doorbell_cause_ipi(int cpu);
+
+/*
+ * Core doorbell IPIs must only be used on CPUs that address the msgsnd
+ * (or msgsndp if in HVMODE) with TIR (cpu_thread_in_core)
+ */
 extern void core_doorbell_cause_ipi(int cpu);
-extern int __try_core_doorbell_cause_ipi(int cpu);
-static inline int try_core_doorbell_cause_ipi(int cpu)
-{
-	if (!cpu_has_feature(CPU_FTR_DBELL))
-		return 0;
-	return __try_core_doorbell_cause_ipi(cpu);
-}
+extern int try_core_doorbell_cause_ipi(int cpu);
 extern void doorbell_exception(struct pt_regs *regs);
 
 static inline void ppc_msgsnd(enum ppc_dbell type, u32 flags, u32 tag)
arch/powerpc/include/asm/smp.h +1 -0
index 0ada12e61fd7..0ee8a6cb1d87 100644
@@ -128,6 +128,7 @@ extern const char *smp_ipi_name[];
 extern void smp_muxed_ipi_message_pass(int cpu, int msg);
 extern void smp_muxed_ipi_set_message(int cpu, int msg);
 extern irqreturn_t smp_ipi_demux(void);
+extern irqreturn_t smp_ipi_demux_relaxed(void);
 
 void smp_init_pSeries(void);
 void smp_init_cell(void);
arch/powerpc/kernel/dbell.c +5 -6
index 4df23ba7c3d3..95409bca76b0 100644
@@ -34,18 +34,15 @@ void core_doorbell_cause_ipi(int cpu)
 {
 	u32 tag;
 
-	if (cpu_has_feature(CPU_FTR_ARCH_300) && cpu_has_feature(CPU_FTR_HVMODE))
-		global_doorbell_cause_ipi(cpu);
-
 	tag = cpu_thread_in_core(cpu);
 
 	kvmppc_set_host_ipi(cpu, 1);
 	/* Order previous accesses vs. msgsnd, which is treated as a store */
-	mb();
+	ppc_msgsnd_sync();
 	ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, tag);
 }
 
-int __try_core_doorbell_cause_ipi(int cpu)
+int try_core_doorbell_cause_ipi(int cpu)
 {
 	int this_cpu = get_cpu();
 	int ret = 0;

@@ -66,12 +63,14 @@ void doorbell_exception(struct pt_regs *regs)
 
 	irq_enter();
 
+	ppc_msgsync();
+
 	may_hard_irq_enable();
 
 	kvmppc_set_host_ipi(smp_processor_id(), 0);
 	__this_cpu_inc(irq_stat.doorbell_irqs);
 
-	smp_ipi_demux();
+	smp_ipi_demux_relaxed(); /* already performed the barrier */
 
 	irq_exit();
 	set_irq_regs(old_regs);
arch/powerpc/kernel/smp.c +10 -3
index bb882fa1f762..fd2441591b81 100644
@@ -246,11 +246,18 @@ void smp_muxed_ipi_message_pass(int cpu, int msg)
 
 irqreturn_t smp_ipi_demux(void)
 {
-	struct cpu_messages *info = this_cpu_ptr(&ipi_message);
-	unsigned long all;
-
 	mb();	/* order any irq clear */
 
+	return smp_ipi_demux_relaxed();
+}
+
+/* sync-free variant. Callers should ensure synchronization */
+irqreturn_t smp_ipi_demux_relaxed(void)
+{
+	struct cpu_messages *info;
+	unsigned long all;
+
+	info = this_cpu_ptr(&ipi_message);
 	do {
 		all = xchg(&info->messages, 0);
 #if defined(CONFIG_KVM_XICS) && defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE)
arch/powerpc/platforms/powernv/smp.c +16 -2
index e796c97914c8..660212d3c2d9 100644
@@ -254,10 +254,24 @@ static void pnv_cause_ipi(int cpu)
 	icp_ops->cause_ipi(cpu);
 }
 
+static __init void pnv_smp_probe(void)
+{
+	xics_smp_probe();
+
+	if (cpu_has_feature(CPU_FTR_DBELL)) {
+		if (cpu_has_feature(CPU_FTR_ARCH_300))
+			smp_ops->cause_ipi = global_doorbell_cause_ipi;
+		else
+			smp_ops->cause_ipi = pnv_cause_ipi;
+	} else {
+		smp_ops->cause_ipi = icp_ops->cause_ipi;
+	}
+}
+
 static struct smp_ops_t pnv_smp_ops = {
 	.message_pass	= NULL, /* Use smp_muxed_ipi_message_pass */
-	.cause_ipi	= pnv_cause_ipi,
-	.probe		= xics_smp_probe,
+	.cause_ipi	= NULL,	/* Filled at runtime by pnv_smp_probe() */
+	.probe		= pnv_smp_probe,
 	.kick_cpu	= pnv_smp_kick_cpu,
 	.setup_cpu	= pnv_smp_setup_cpu,
 	.cpu_bootable	= pnv_cpu_bootable,
arch/powerpc/platforms/pseries/smp.c +7 -1
index a26b3158555e..87b5700b217b 100644
@@ -190,11 +190,17 @@ static void smp_pseries_cause_ipi(int cpu)
 static __init void pSeries_smp_probe(void)
 {
 	xics_smp_probe();
+
+	if (cpu_has_feature(CPU_FTR_DBELL))
+		smp_ops->cause_ipi = pseries_cause_ipi;
+	else
+		smp_ops->cause_ipi = icp_ops->cause_ipi;
+
 }
 
 static struct smp_ops_t pseries_smp_ops = {
 	.message_pass	= NULL,	/* Use smp_muxed_ipi_message_pass */
-	.cause_ipi	= smp_pseries_cause_ipi,
+	.cause_ipi	= NULL, /* Filled at runtime by pSeries_smp_probe */
 	.probe		= pSeries_smp_probe,
 	.kick_cpu	= smp_pSeries_kick_cpu,
 	.setup_cpu	= smp_setup_cpu,
Signature · 1 line
2.11.0
#3 lore raw
In reply to Nicholas Piggin
Quoted 8 lines · Nicholas Piggin

Hi,

Just after the previous two fixes, I would like to propose changing the way we do doorbell vs interrupt controller IPIs, and add support for global doorbells supported by POWER9 in HV mode.

After this, the platform code knows about doorbells and interrupt controller IPIs, rather than they know about each other.

A few things come to mind:

- We don't want to use doorbells under KVM. They are going to turn
into traps and be emulated, slower than using H_IPI, at least on P9. Even for core only doorbells. I'm not sure how to convey that to the guest.

- On PP9 DD1 we need a CI load instead of msgsync (a DARN instruction
would do too if it works)

- Can we get rid of the atomic ops for manipulating the IPI mux ? What
about a cache line per message and just set/clear ? If we clear in the doorbell handler before we call the respective targets, we shouldn't "lose" messages no ? As long as the actual handlers "loop" as necessary of course.

Cheers,
Ben.

Quoted 24 lines

Thanks,
Nick

Nicholas Piggin (2):
  powerpc/64s: change the doorbell IPI calling convention powerpc/64s: use global doorbell on POWER9 in HV mode

 arch/powerpc/include/asm/dbell.h       | 38 ++++++++++++++++++++++
-----
 arch/powerpc/include/asm/smp.h         |  4 +-- arch/powerpc/include/asm/xics.h        |  2 +- arch/powerpc/kernel/dbell.c            | 47 ++++++++++++++++++++++
------------
 arch/powerpc/kernel/smp.c              | 27 ++++++++++--------- arch/powerpc/platforms/85xx/smp.c      |  9 +------ arch/powerpc/platforms/powermac/smp.c  |  2 +- arch/powerpc/platforms/powernv/smp.c   | 32 +++++++++++++++++------ arch/powerpc/platforms/pseries/smp.c   | 28 ++++++++------------ arch/powerpc/sysdev/xics/icp-hv.c      |  2 +- arch/powerpc/sysdev/xics/icp-native.c  | 12 +-------- arch/powerpc/sysdev/xics/icp-opal.c    |  2 +- arch/powerpc/sysdev/xics/xics-common.c |  3 ---

 13 files changed, 118 insertions(+), 90 deletions(-)
author reviewer
#4 lore raw
Quoted 10 lines · On Tue, 14 Mar 2017 10:31:08 +1100 Benjamin Herrenschmidt…
Quoted 8 lines · Nicholas Piggin

Hi,

Just after the previous two fixes, I would like to propose changing the way we do doorbell vs interrupt controller IPIs, and add support for global doorbells supported by POWER9 in HV mode.

After this, the platform code knows about doorbells and interrupt controller IPIs, rather than they know about each other.

Quoted 6 lines

A few things come to mind:

- We don't want to use doorbells under KVM. They are going to turn
into traps and be emulated, slower than using H_IPI, at least on P9. Even for core only doorbells. I'm not sure how to convey that to the guest.

msgsndp will be okay, won't it? Guest just chooses that based on HVMODE (which pseries platform knows is core only).

Quoted 2 lines

- On PP9 DD1 we need a CI load instead of msgsync (a DARN instruction
would do too if it works)

Yes, Paul pointed this out too. I'll add an alt patch for it. Apparently also msgsync needs lwsync afterwards for DD2.

Quoted 6 lines

- Can we get rid of the atomic ops for manipulating the IPI mux ? What
about a cache line per message and just set/clear ? If we clear in the doorbell handler before we call the respective targets, we shouldn't "lose" messages no ? As long as the actual handlers "loop" as necessary of course.

Yes I think that would work. Good idea. A single cacheline with messages being independently stored bytes within it might work better, so the receiver CPU does not have to go through and load multiple cachelines to check for messages. It could load up to 8 message types with one load.

Thanks,
Nick

#5 lore raw
Quoted 15 lines · Nicholas Piggin

On Tue, 14 Mar 2017 10:31:08 +1100

Quoted 14 lines

Benjamin Herrenschmidt <[email protected]> wrote:

Quoted 8 lines · Nicholas Piggin

Hi,

Just after the previous two fixes, I would like to propose changing the way we do doorbell vs interrupt controller IPIs, and add support for global doorbells supported by POWER9 in HV mode.

After this, the platform code knows about doorbells and interrupt controller IPIs, rather than they know about each other.

A few things come to mind:

Quoted 7 lines
Quoted 4 lines

 - We don't want to use doorbells under KVM. They are going to turn into traps and be emulated, slower than using H_IPI, at least on P9. Even for core only doorbells. I'm not sure how to convey that to the guest.

msgsndp will be okay, won't it? Guest just chooses that based on HVMODE (which pseries platform knows is core only).

No. It will suck. Because KVM can run each guest thread on a different core, the HW won't work, so we have to disable it and trap the instructions & emulate them. We really don't want P9 guests to use it under KVM (it's fine under pHyp).

Quoted 5 lines
Quoted 2 lines

 - On PP9 DD1 we need a CI load instead of msgsync (a DARN instruction would do too if it works)

Yes, Paul pointed this out too. I'll add an alt patch for it. Apparently also msgsync needs lwsync afterwards for DD2.

Odd. Ok.

Quoted 10 lines
Quoted 5 lines

 - Can we get rid of the atomic ops for manipulating the IPI mux ? What about a cache line per message and just set/clear ? If we clear in the doorbell handler before we call the respective targets, we shouldn't "lose" messages no ? As long as the actual handlers "loop" as necessary of course.

Yes I think that would work. Good idea. A single cacheline with messages being independently stored bytes within it might work better, so the receiver CPU does not have to go through and load multiple cachelines to check for messages. It could load up to 8 message types with one load.

Ok. But we need to make sure we use multiple stores to not lose messages.

Ie.

- Load all
- For each byte if set
- clear byte
- then call handler

Cheers,
Ben.

author reviewer
#6 lore raw
Quoted 21 lines · On Tue, 14 Mar 2017 13:34:38 +1100 Benjamin Herrenschmidt…
Quoted 20 lines · Nicholas Piggin

On Tue, 14 Mar 2017 10:31:08 +1100

Quoted 18 lines

Benjamin Herrenschmidt <[email protected]> wrote:

Quoted 8 lines · Nicholas Piggin

Hi,

Just after the previous two fixes, I would like to propose changing the way we do doorbell vs interrupt controller IPIs, and add support for global doorbells supported by POWER9 in HV mode.

After this, the platform code knows about doorbells and interrupt controller IPIs, rather than they know about each other.

A few things come to mind:

 - We don't want to use doorbells under KVM. They are going to turn into traps and be emulated, slower than using H_IPI, at least on P9. Even for core only doorbells. I'm not sure how to convey that to the guest.

Quoted 6 lines
Quoted 2 lines

msgsndp will be okay, won't it? Guest just chooses that based on HVMODE (which pseries platform knows is core only).

No. It will suck. Because KVM can run each guest thread on a different core, the HW won't work, so we have to disable it and trap the instructions & emulate them. We really don't want P9 guests to use it under KVM (it's fine under pHyp).

Ah, gotcha.

Quoted 19 lines
Quoted 5 lines
Quoted 2 lines

 - On PP9 DD1 we need a CI load instead of msgsync (a DARN instruction would do too if it works)

Yes, Paul pointed this out too. I'll add an alt patch for it. Apparently also msgsync needs lwsync afterwards for DD2.

Odd. Ok.

Quoted 10 lines
Quoted 5 lines

 - Can we get rid of the atomic ops for manipulating the IPI mux ? What about a cache line per message and just set/clear ? If we clear in the doorbell handler before we call the respective targets, we shouldn't "lose" messages no ? As long as the actual handlers "loop" as necessary of course.

Yes I think that would work. Good idea. A single cacheline with messages being independently stored bytes within it might work better, so the receiver CPU does not have to go through and load multiple cachelines to check for messages. It could load up to 8 message types with one load.

Quoted 8 lines

Ok. But we need to make sure we use multiple stores to not lose messages.

Ie.

- Load all
- For each byte if set
- clear byte
- then call handler

Yes. I think that will be okay because we shouldn't get any load-hit-store issues. I'll do some benchmarking anyway.

Thanks,
Nick

#7 lore raw
Quoted 8 lines · Nicholas Piggin
Quoted 4 lines

  - Load all
  - For each byte if set
     - clear byte
     - then call handler

Yes. I think that will be okay because we shouldn't get any load-hit- store
issues. I'll do some benchmarking anyway.

We might need a sync still between clearing the byte and calling the handler no ? Or at least a smp_wmb() to ensure that the clear is visible before any action of the handler.

Cheers,
Ben.

author reviewer
#8 lore raw
Quoted 6 lines · On Tue, 14 Mar 2017 14:57:20 +1100 Benjamin Herrenschmidt…
Quoted 5 lines · Nicholas Piggin
Quoted 4 lines

  - Load all
  - For each byte if set
     - clear byte
     - then call handler

Quoted 7 lines
Quoted 3 lines

Yes. I think that will be okay because we shouldn't get any load-hit- store
issues. I'll do some benchmarking anyway.

We might need a sync still between clearing the byte and calling the handler no ? Or at least a smp_wmb() to ensure that the clear is visible before any action of the handler.

Yes I have exactly that (smp_wmb).

At first I checked and cleared each byte then did a single smp_wmb, but I changed my mind because most of the time the IPI will fire with only one message set, so it does not seem like it's worth the extra branches to avoid a lwsync in the rare case of 2 messages.

Thanks,
Nick

#9 lore raw
Quoted 10 lines · Nicholas Piggin
Quoted 3 lines

We might need a sync still between clearing the byte and calling the handler no ? Or at least a smp_wmb() to ensure that the clear is visible before any action of the handler.

Yes I have exactly that (smp_wmb).

At first I checked and cleared each byte then did a single smp_wmb, but I changed my mind because most of the time the IPI will fire with only one message set, so it does not seem like it's worth the extra branches to avoid a lwsync in the rare case of 2 messages.

Will lwsync provide transitivity ?

What we care about is that if the handler does something that when observed by another CPU causes that other CPU to send back an IPI, the write by that other CPU comes after our clear. I'm not sure if lwsync is enough. We might need Paul Mck for that :-)

Cheers,
Ben.

author reviewer
#10 lore raw
Quoted 12 lines · On Tue, 14 Mar 2017 15:50:01 +1100 Benjamin Herrenschmidt…
Quoted 10 lines · Nicholas Piggin
Quoted 3 lines

We might need a sync still between clearing the byte and calling the handler no ? Or at least a smp_wmb() to ensure that the clear is visible before any action of the handler.

Yes I have exactly that (smp_wmb).

At first I checked and cleared each byte then did a single smp_wmb, but I changed my mind because most of the time the IPI will fire with only one message set, so it does not seem like it's worth the extra branches to avoid a lwsync in the rare case of 2 messages.

Quoted 6 lines

Will lwsync provide transitivity ?

What we care about is that if the handler does something that when observed by another CPU causes that other CPU to send back an IPI, the write by that other CPU comes after our clear. I'm not sure if lwsync is enough. We might need Paul Mck for that :-)

Hmm. Well lwsync is cumulative for the ordering it provides.

The target CPU can pick up messages without having gone through an interrupt and msgsync, and even before sender has executed a sync.

Let's say we have the following situation where the IPI handler on CPU1 reads variables set by CPU0 and CPU2, and stores the result in other memory locations.

CPU0 CPU1 CPU2

data0=1
sync
CPU[1].msg[x]=1

                       msg=CPU[1].msg[x]
                       if (msg) /* handle x */
                         CPU[1].msg[x]=0
                         lwsync
                         ready0=data0
                         ready2=data2

                         ASSERTa(ready0==1)

                                            x=ready0
                                            if (x)
                                              ASSERTb(data0==1 after lwsync)
                                              data2=1
                                              sync
                                              CPU[1].msg[x]=1
                                              sync
                                              doorbell

                       *doorbell*
                       if (ready2==0)
                         ASSERTc(CPU[1].msg[x]==1)

ASSERTa is the simple pairwise that data from CPU0 must be visible if the message is set. In this case sync and lwsync on CPU0 and 1 should make that work for cache inhibited. I guess if we only order cacheable and require CI to add their own syncs, then CPU0 sync may be lwsync as well? Alternatively if we require CI ordering then CPU1 has to be sync.

ASSERTb is cumulative ordering where if CPU2 sees CPU1 handler result then it must also see CPU0 pre-IPI result. I think this is covered by example 2 on p820 of ISAv3 doc.

ASSERTc says that if the handler did not pick up CPU2's store after setting msg[x]=0, then it must eventually see msg[x]=1 store from CPU2. I think this should also be true because if CPU2 found ready0==1, then it would also see CPU[1].msg[x]==0 after a barrier (simple pairwise ordering).

You're right though, we should run it by Paul :)

Thanks,
Nick