[PATCH 0/3] powerpc/pseries: IPI doorbell improvements

Thanks for the review, I think I incorporated all your comments, I
also did add KVM detection which avoids introducing a performance
regression.

Thanks,
Nick

Nicholas Piggin (3):
  powerpc: inline doorbell sending functions
  powerpc/pseries: Use doorbells even if XIVE is available
  powerpc/pseries: Add KVM guest doorbell restrictions

 arch/powerpc/include/asm/dbell.h          | 59 +++++++++++++++++++--
 arch/powerpc/include/asm/firmware.h       |  2 +
 arch/powerpc/include/asm/kvm_para.h       | 26 ++--------
 arch/powerpc/kernel/dbell.c               | 55 --------------------
 arch/powerpc/platforms/pseries/firmware.c | 14 +++++
 arch/powerpc/platforms/pseries/smp.c      | 62 ++++++++++++++++-------
 6 files changed, 119 insertions(+), 99 deletions(-)

-- 
2.23.0

These are only called in one place for a given platform, so inline them
for performance.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/dbell.h | 59 ++++++++++++++++++++++++++++++--
 arch/powerpc/kernel/dbell.c      | 55 -----------------------------
 2 files changed, 56 insertions(+), 58 deletions(-)

diff --git a/arch/powerpc/include/asm/dbell.h b/arch/powerpc/include/asm/dbell.h
index 4ce6808deed3..5c9625e5070b 100644
--- a/arch/powerpc/include/asm/dbell.h
+++ b/arch/powerpc/include/asm/dbell.h

@@ -13,6 +13,7 @@
 
 #include <asm/ppc-opcode.h>
 #include <asm/feature-fixups.h>
+#include <asm/kvm_ppc.h>
 
 #define PPC_DBELL_MSG_BRDCAST	(0x04000000)
 #define PPC_DBELL_TYPE(x)	(((x) & 0xf) << (63-36))

@@ -87,9 +88,6 @@ static inline void ppc_msgsync(void)
 
 #endif /* CONFIG_PPC_BOOK3S */
 
-extern void doorbell_global_ipi(int cpu);
-extern void doorbell_core_ipi(int cpu);
-extern int doorbell_try_core_ipi(int cpu);
 extern void doorbell_exception(struct pt_regs *regs);
 
 static inline void ppc_msgsnd(enum ppc_dbell type, u32 flags, u32 tag)

@@ -100,4 +98,59 @@ static inline void ppc_msgsnd(enum ppc_dbell type, u32 flags, u32 tag)
 	_ppc_msgsnd(msg);
 }
 
+/*
+ * Doorbells must only be used if CPU_FTR_DBELL is available.
+ * msgsnd is used in HV, and msgsndp is used in !HV.
+ *
+ * These should be used by platform code that is aware of restrictions.
+ * Other arch code should use ->cause_ipi.
+ *
+ * doorbell_global_ipi() sends a dbell to any target CPU.
+ * Must be used only by architectures that address msgsnd target
+ * by PIR/get_hard_smp_processor_id.
+ */
+static inline void doorbell_global_ipi(int cpu)
+{
+	u32 tag = get_hard_smp_processor_id(cpu);
+
+	kvmppc_set_host_ipi(cpu);
+	/* Order previous accesses vs. msgsnd, which is treated as a store */
+	ppc_msgsnd_sync();
+	ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, tag);
+}
+
+/*
+ * doorbell_core_ipi() sends a dbell to a target CPU in the same core.
+ * Must be used only by architectures that address msgsnd target
+ * by TIR/cpu_thread_in_core.
+ */
+static inline void doorbell_core_ipi(int cpu)
+{
+	u32 tag = cpu_thread_in_core(cpu);
+
+	kvmppc_set_host_ipi(cpu);
+	/* Order previous accesses vs. msgsnd, which is treated as a store */
+	ppc_msgsnd_sync();
+	ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, tag);
+}
+
+/*
+ * Attempt to cause a core doorbell if destination is on the same core.
+ * Returns 1 on success, 0 on failure.
+ */
+static inline int doorbell_try_core_ipi(int cpu)
+{
+	int this_cpu = get_cpu();
+	int ret = 0;
+
+	if (cpumask_test_cpu(cpu, cpu_sibling_mask(this_cpu))) {
+		doorbell_core_ipi(cpu);
+		ret = 1;
+	}
+
+	put_cpu();
+
+	return ret;
+}
+
 #endif /* _ASM_POWERPC_DBELL_H */

diff --git a/arch/powerpc/kernel/dbell.c b/arch/powerpc/kernel/dbell.c
index f17ff1200eaa..52680cf07c9d 100644
--- a/arch/powerpc/kernel/dbell.c
+++ b/arch/powerpc/kernel/dbell.c

@@ -18,61 +18,6 @@
 
 #ifdef CONFIG_SMP
 
-/*
- * Doorbells must only be used if CPU_FTR_DBELL is available.
- * msgsnd is used in HV, and msgsndp is used in !HV.
- *
- * These should be used by platform code that is aware of restrictions.
- * Other arch code should use ->cause_ipi.
- *
- * doorbell_global_ipi() sends a dbell to any target CPU.
- * Must be used only by architectures that address msgsnd target
- * by PIR/get_hard_smp_processor_id.
- */
-void doorbell_global_ipi(int cpu)
-{
-	u32 tag = get_hard_smp_processor_id(cpu);
-
-	kvmppc_set_host_ipi(cpu);
-	/* Order previous accesses vs. msgsnd, which is treated as a store */
-	ppc_msgsnd_sync();
-	ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, tag);
-}
-
-/*
- * doorbell_core_ipi() sends a dbell to a target CPU in the same core.
- * Must be used only by architectures that address msgsnd target
- * by TIR/cpu_thread_in_core.
- */
-void doorbell_core_ipi(int cpu)
-{
-	u32 tag = cpu_thread_in_core(cpu);
-
-	kvmppc_set_host_ipi(cpu);
-	/* Order previous accesses vs. msgsnd, which is treated as a store */
-	ppc_msgsnd_sync();
-	ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, tag);
-}
-
-/*
- * Attempt to cause a core doorbell if destination is on the same core.
- * Returns 1 on success, 0 on failure.
- */
-int doorbell_try_core_ipi(int cpu)
-{
-	int this_cpu = get_cpu();
-	int ret = 0;
-
-	if (cpumask_test_cpu(cpu, cpu_sibling_mask(this_cpu))) {
-		doorbell_core_ipi(cpu);
-		ret = 1;
-	}
-
-	put_cpu();
-
-	return ret;
-}
-
 void doorbell_exception(struct pt_regs *regs)
 {
 	struct pt_regs *old_regs = set_irq_regs(regs);

-- 
2.23.0

KVM supports msgsndp in guests by trapping and emulating the
instruction, so it was decided to always use XIVE for IPIs if it is
available. However on PowerVM systems, msgsndp can be used and gives
better performance. On large systems, high XIVE interrupt rates can
have sub-linear scaling, and using msgsndp can reduce the load on
the interrupt controller.

So switch to using core local doorbells even if XIVE is available.
This reduces performance for KVM guests with an SMT topology by
about 50% for ping-pong context switching between SMT vCPUs. An
option vector (or dt-cpu-ftrs) could be defined to disable msgsndp
to get KVM performance back.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/pseries/smp.c | 54 ++++++++++++++++++----------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
index 6891710833be..67e6ad5076ce 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c

@@ -188,13 +188,16 @@ static int pseries_smp_prepare_cpu(int cpu)
 	return 0;
 }
 
-static void smp_pseries_cause_ipi(int cpu)
+/* Cause IPI as setup by the interrupt controller (xics or xive) */
+static void (*ic_cause_ipi)(int cpu) __ro_after_init;
+
+/* Use msgsndp doorbells target is a sibling, else use interrupt controller */
+static void dbell_or_ic_cause_ipi(int cpu)
 {
-	/* POWER9 should not use this handler */
 	if (doorbell_try_core_ipi(cpu))
 		return;
 
-	icp_ops->cause_ipi(cpu);
+	ic_cause_ipi(cpu);
 }
 
 static int pseries_cause_nmi_ipi(int cpu)

@@ -218,26 +221,41 @@ static int pseries_cause_nmi_ipi(int cpu)
 	return 0;
 }
 
-static __init void pSeries_smp_probe_xics(void)
-{
-	xics_smp_probe();
-
-	if (cpu_has_feature(CPU_FTR_DBELL) && !is_secure_guest())
-		smp_ops->cause_ipi = smp_pseries_cause_ipi;
-	else
-		smp_ops->cause_ipi = icp_ops->cause_ipi;
-}
-
 static __init void pSeries_smp_probe(void)
 {
 	if (xive_enabled())
-		/*
-		 * Don't use P9 doorbells when XIVE is enabled. IPIs
-		 * using MMIOs should be faster
-		 */
 		xive_smp_probe();
 	else
-		pSeries_smp_probe_xics();
+		xics_smp_probe();
+
+	/* No doorbell facility, must use the interrupt controller for IPIs */
+	if (!cpu_has_feature(CPU_FTR_DBELL))
+		return;
+
+	/* Doorbells can only be used for IPIs between SMT siblings */
+	if (!cpu_has_feature(CPU_FTR_SMT))
+		return;
+
+	/*
+	 * KVM emulates doorbells by disabling FSCR[MSGP] so msgsndp faults
+	 * to the hypervisor which then reads the instruction from guest
+	 * memory. This can't be done if the guest is secure, so don't use
+	 * doorbells in secure guests.
+	 *
+	 * Under PowerVM, FSCR[MSGP] is enabled so doorbells could be used
+	 * by secure guests if we distinguished this from KVM.
+	 */
+	if (is_secure_guest())
+		return;
+
+	/*
+	 * The guest can use doobells for SMT sibling IPIs, which stay in
+	 * the core rather than going to the interrupt controller. This
+	 * tends to be slower under KVM where doorbells are emulated, but
+	 * faster for PowerVM where they're enabled.
+	 */
+	ic_cause_ipi = smp_ops->cause_ipi;
+	smp_ops->cause_ipi = dbell_or_ic_cause_ipi;
 }
 
 static struct smp_ops_t pseries_smp_ops = {

-- 
2.23.0

KVM guests have certain restrictions and performance quirks when
using doorbells. This patch tests for KVM environment in doorbell
setup, and optimises IPI performance:

 - PowerVM guests may now use doorbells even if they are secure.

 - KVM guests no longer use doorbells if XIVE is available.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/firmware.h       |  2 ++
 arch/powerpc/include/asm/kvm_para.h       | 26 ++--------------
 arch/powerpc/platforms/pseries/firmware.c | 14 +++++++++
 arch/powerpc/platforms/pseries/smp.c      | 38 ++++++++++++++---------
 4 files changed, 42 insertions(+), 38 deletions(-)

diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h
index 6003c2e533a0..4dadb84ff2b2 100644
--- a/arch/powerpc/include/asm/firmware.h
+++ b/arch/powerpc/include/asm/firmware.h

@@ -134,7 +134,9 @@ extern unsigned int __start___fw_ftr_fixup, __stop___fw_ftr_fixup;
 
 #ifdef CONFIG_PPC_PSERIES
 void pseries_probe_fw_features(void);
+bool is_kvm_guest(void);
 #else
+static inline bool is_kvm_guest(void) { return false; }
 static inline void pseries_probe_fw_features(void) { };
 #endif
 

diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h
index 9c1f6b4b9bbf..744612054c94 100644
--- a/arch/powerpc/include/asm/kvm_para.h
+++ b/arch/powerpc/include/asm/kvm_para.h

@@ -8,35 +8,15 @@
 #ifndef __POWERPC_KVM_PARA_H__
 #define __POWERPC_KVM_PARA_H__
 
-#include <uapi/asm/kvm_para.h>
-
-#ifdef CONFIG_KVM_GUEST
-
-#include <linux/of.h>
-
-static inline int kvm_para_available(void)
-{
-	struct device_node *hyper_node;
-
-	hyper_node = of_find_node_by_path("/hypervisor");
-	if (!hyper_node)
-		return 0;
+#include <asm/firmware.h>
 
-	if (!of_device_is_compatible(hyper_node, "linux,kvm"))
-		return 0;
-
-	return 1;
-}
-
-#else
+#include <uapi/asm/kvm_para.h>
 
 static inline int kvm_para_available(void)
 {
-	return 0;
+	return IS_ENABLED(CONFIG_KVM_GUEST) && is_kvm_guest();
 }
 
-#endif
-
 static inline unsigned int kvm_arch_para_features(void)
 {
 	unsigned long r;

diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c
index 3e49cc23a97a..f58eb10011dd 100644
--- a/arch/powerpc/platforms/pseries/firmware.c
+++ b/arch/powerpc/platforms/pseries/firmware.c

@@ -184,3 +184,17 @@ void __init pseries_probe_fw_features(void)
 {
 	of_scan_flat_dt(probe_fw_features, NULL);
 }
+
+bool is_kvm_guest(void)
+{
+	struct device_node *hyper_node;
+
+	hyper_node = of_find_node_by_path("/hypervisor");
+	if (!hyper_node)
+		return 0;
+
+	if (!of_device_is_compatible(hyper_node, "linux,kvm"))
+		return 0;
+
+	return 1;
+}

diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
index 67e6ad5076ce..7af0003b40b6 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c

@@ -236,24 +236,32 @@ static __init void pSeries_smp_probe(void)
 	if (!cpu_has_feature(CPU_FTR_SMT))
 		return;
 
-	/*
-	 * KVM emulates doorbells by disabling FSCR[MSGP] so msgsndp faults
-	 * to the hypervisor which then reads the instruction from guest
-	 * memory. This can't be done if the guest is secure, so don't use
-	 * doorbells in secure guests.
-	 *
-	 * Under PowerVM, FSCR[MSGP] is enabled so doorbells could be used
-	 * by secure guests if we distinguished this from KVM.
-	 */
-	if (is_secure_guest())
-		return;
+	if (is_kvm_guest()) {
+		/*
+		 * KVM emulates doorbells by disabling FSCR[MSGP] so msgsndp
+		 * faults to the hypervisor which then reads the instruction
+		 * from guest memory, which tends to be slower than using XIVE.
+		 */
+		if (xive_enabled())
+			return;
+
+		/*
+		 * XICS hcalls aren't as fast, so we can use msgsndp (which
+		 * also helps exercise KVM emulation), however KVM can't
+		 * emulate secure guests because it can't read the instruction
+		 * out of their memory.
+		 */
+		if (is_secure_guest())
+			return;
+	}
 
 	/*
-	 * The guest can use doobells for SMT sibling IPIs, which stay in
-	 * the core rather than going to the interrupt controller. This
-	 * tends to be slower under KVM where doorbells are emulated, but
-	 * faster for PowerVM where they're enabled.
+	 * Under PowerVM, FSCR[MSGP] is enabled as guest vCPU siblings are
+	 * gang scheduled on the same physical core, so doorbells are always
+	 * faster than the interrupt controller, and they can be used by
+	 * secure guests.
 	 */
+
 	ic_cause_ipi = smp_ops->cause_ipi;
 	smp_ops->cause_ipi = dbell_or_ic_cause_ipi;
 }

-- 
2.23.0

Hi Nicholas,

I love your patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on scottwood/next v5.8-rc2 next-20200626]
[cannot apply to kvm-ppc/kvm-ppc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nicholas-Piggin/powerpc-pseries-IPI-doorbell-improvements/20200627-230544
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-c003-20200628 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All error/warnings (new ones prefixed by >>):

   In file included from arch/powerpc/kernel/asm-offsets.c:38:
   arch/powerpc/include/asm/dbell.h: In function 'doorbell_global_ipi':

quoted

arch/powerpc/include/asm/dbell.h:114:12: error: implicit declaration of function 'get_hard_smp_processor_id'; did you mean 'raw_smp_processor_id'? [-Werror=implicit-function-declaration]

     114 |  u32 tag = get_hard_smp_processor_id(cpu);
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~
         |            raw_smp_processor_id
   arch/powerpc/include/asm/dbell.h: In function 'doorbell_try_core_ipi':

quoted

arch/powerpc/include/asm/dbell.h:146:28: error: implicit declaration of function 'cpu_sibling_mask'; did you mean 'cpu_online_mask'? [-Werror=implicit-function-declaration]

     146 |  if (cpumask_test_cpu(cpu, cpu_sibling_mask(this_cpu))) {
         |                            ^~~~~~~~~~~~~~~~
         |                            cpu_online_mask

quoted

arch/powerpc/include/asm/dbell.h:146:28: warning: passing argument 2 of 'cpumask_test_cpu' makes pointer from integer without a cast [-Wint-conversion]

     146 |  if (cpumask_test_cpu(cpu, cpu_sibling_mask(this_cpu))) {
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~
         |                            |
         |                            int
   In file included from include/linux/workqueue.h:15,
                    from include/linux/rhashtable-types.h:15,
                    from include/linux/ipc.h:7,
                    from include/uapi/linux/sem.h:5,
                    from include/linux/sem.h:5,
                    from include/linux/compat.h:14,
                    from arch/powerpc/kernel/asm-offsets.c:14:
   include/linux/cpumask.h:365:67: note: expected 'const struct cpumask *' but argument is of type 'int'
     365 | static inline int cpumask_test_cpu(int cpu, const struct cpumask *cpumask)
         |                                             ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
   cc1: some warnings being treated as errors
   make[2]: *** [scripts/Makefile.build:114: arch/powerpc/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1175: prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.

vim +114 arch/powerpc/include/asm/dbell.h

   100	
   101	/*
   102	 * Doorbells must only be used if CPU_FTR_DBELL is available.
   103	 * msgsnd is used in HV, and msgsndp is used in !HV.
   104	 *
   105	 * These should be used by platform code that is aware of restrictions.
   106	 * Other arch code should use ->cause_ipi.
   107	 *
   108	 * doorbell_global_ipi() sends a dbell to any target CPU.
   109	 * Must be used only by architectures that address msgsnd target
   110	 * by PIR/get_hard_smp_processor_id.
   111	 */
   112	static inline void doorbell_global_ipi(int cpu)
   113	{
 > 114		u32 tag = get_hard_smp_processor_id(cpu);
   115	
   116		kvmppc_set_host_ipi(cpu);
   117		/* Order previous accesses vs. msgsnd, which is treated as a store */
   118		ppc_msgsnd_sync();
   119		ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, tag);
   120	}
   121	
   122	/*
   123	 * doorbell_core_ipi() sends a dbell to a target CPU in the same core.
   124	 * Must be used only by architectures that address msgsnd target
   125	 * by TIR/cpu_thread_in_core.
   126	 */
   127	static inline void doorbell_core_ipi(int cpu)
   128	{
   129		u32 tag = cpu_thread_in_core(cpu);
   130	
   131		kvmppc_set_host_ipi(cpu);
   132		/* Order previous accesses vs. msgsnd, which is treated as a store */
   133		ppc_msgsnd_sync();
   134		ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, tag);
   135	}
   136	
   137	/*
   138	 * Attempt to cause a core doorbell if destination is on the same core.
   139	 * Returns 1 on success, 0 on failure.
   140	 */
   141	static inline int doorbell_try_core_ipi(int cpu)
   142	{
   143		int this_cpu = get_cpu();
   144		int ret = 0;
   145	
 > 146		if (cpumask_test_cpu(cpu, cpu_sibling_mask(this_cpu))) {
   147			doorbell_core_ipi(cpu);
   148			ret = 1;
   149		}
   150	
   151		put_cpu();
   152	
   153		return ret;
   154	}
   155	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

kernel test robot [off-list ref] writes:

Hi Nicholas,

I love your patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on scottwood/next v5.8-rc2 next-20200626]
[cannot apply to kvm-ppc/kvm-ppc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nicholas-Piggin/powerpc-pseries-IPI-doorbell-improvements/20200627-230544
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-c003-20200628 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All error/warnings (new ones prefixed by >>):

   In file included from arch/powerpc/kernel/asm-offsets.c:38:
   arch/powerpc/include/asm/dbell.h: In function 'doorbell_global_ipi':

quoted

quoted

arch/powerpc/include/asm/dbell.h:114:12: error: implicit declaration of function 'get_hard_smp_processor_id'; did you mean 'raw_smp_processor_id'? [-Werror=implicit-function-declaration]

     114 |  u32 tag = get_hard_smp_processor_id(cpu);
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~
         |            raw_smp_processor_id
   arch/powerpc/include/asm/dbell.h: In function 'doorbell_try_core_ipi':

quoted

quoted

arch/powerpc/include/asm/dbell.h:146:28: error: implicit declaration of function 'cpu_sibling_mask'; did you mean 'cpu_online_mask'? [-Werror=implicit-function-declaration]

     146 |  if (cpumask_test_cpu(cpu, cpu_sibling_mask(this_cpu))) {
         |                            ^~~~~~~~~~~~~~~~
         |                            cpu_online_mask

quoted

quoted

arch/powerpc/include/asm/dbell.h:146:28: warning: passing argument 2 of 'cpumask_test_cpu' makes pointer from integer without a cast [-Wint-conversion]

     146 |  if (cpumask_test_cpu(cpu, cpu_sibling_mask(this_cpu))) {
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~

Seems like CONFIG_SMP=n is probably the root cause.

You could try including asm/smp.h, but good chance that will lead to
header soup.

Other option would be to wrap the whole lot in #ifdef CONFIG_SMP?

cheers

Excerpts from Michael Ellerman's message of June 30, 2020 11:31 am:

kernel test robot [off-list ref] writes:

quoted

Hi Nicholas,

I love your patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on scottwood/next v5.8-rc2 next-20200626]
[cannot apply to kvm-ppc/kvm-ppc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nicholas-Piggin/powerpc-pseries-IPI-doorbell-improvements/20200627-230544
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-c003-20200628 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0

quoted

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All error/warnings (new ones prefixed by >>):

   In file included from arch/powerpc/kernel/asm-offsets.c:38:
   arch/powerpc/include/asm/dbell.h: In function 'doorbell_global_ipi':

quoted

quoted

arch/powerpc/include/asm/dbell.h:114:12: error: implicit declaration of function 'get_hard_smp_processor_id'; did you mean 'raw_smp_processor_id'? [-Werror=implicit-function-declaration]

     114 |  u32 tag = get_hard_smp_processor_id(cpu);
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~
         |            raw_smp_processor_id
   arch/powerpc/include/asm/dbell.h: In function 'doorbell_try_core_ipi':

quoted

quoted

arch/powerpc/include/asm/dbell.h:146:28: error: implicit declaration of function 'cpu_sibling_mask'; did you mean 'cpu_online_mask'? [-Werror=implicit-function-declaration]

     146 |  if (cpumask_test_cpu(cpu, cpu_sibling_mask(this_cpu))) {
         |                            ^~~~~~~~~~~~~~~~
         |                            cpu_online_mask

quoted

quoted

arch/powerpc/include/asm/dbell.h:146:28: warning: passing argument 2 of 'cpumask_test_cpu' makes pointer from integer without a cast [-Wint-conversion]

     146 |  if (cpumask_test_cpu(cpu, cpu_sibling_mask(this_cpu))) {
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~

Seems like CONFIG_SMP=n is probably the root cause.

You could try including asm/smp.h, but good chance that will lead to
header soup.

Possibly. dbell.h shouldn't be included by much, but maybe it gets
dragged in.

Other option would be to wrap the whole lot in #ifdef CONFIG_SMP?

Yeah that might be a better idea.

I'll fix it up and repost if there's no strong objections to
the KVM detection bit.

Thanks,
Nick

On Sun, Jun 28, 2020 at 01:04:28AM +1000, Nicholas Piggin wrote:

KVM guests have certain restrictions and performance quirks when
using doorbells. This patch tests for KVM environment in doorbell
setup, and optimises IPI performance:

 - PowerVM guests may now use doorbells even if they are secure.

 - KVM guests no longer use doorbells if XIVE is available.

It seems, from the fact that you completely remove
kvm_para_available(), that you perhaps haven't tried building with
CONFIG_KVM_GUEST=y.  Somewhat confusingly, that option is not used or
needed when building for a PAPR guest (i.e. the "pseries" platform)
but is used on non-IBM platforms using the "epapr" hypervisor
interface.

If you did intend to remove support for the epapr hypervisor interface
then that should have been talked about in the commit message (and
would I expect be controversial).

So NAK on the kvm_para_available() removal.

Paul.

Excerpts from Paul Mackerras's message of June 30, 2020 12:27 pm:

On Sun, Jun 28, 2020 at 01:04:28AM +1000, Nicholas Piggin wrote:

quoted

KVM guests have certain restrictions and performance quirks when
using doorbells. This patch tests for KVM environment in doorbell
setup, and optimises IPI performance:

 - PowerVM guests may now use doorbells even if they are secure.

 - KVM guests no longer use doorbells if XIVE is available.

It seems, from the fact that you completely remove
kvm_para_available(), that you perhaps haven't tried building with
CONFIG_KVM_GUEST=y.

It's still there and builds:

static inline int kvm_para_available(void)
{
        return IS_ENABLED(CONFIG_KVM_GUEST) && is_kvm_guest();
}

but...

Somewhat confusingly, that option is not used or
needed when building for a PAPR guest (i.e. the "pseries" platform)
but is used on non-IBM platforms using the "epapr" hypervisor
interface.

... is_kvm_guest() returns false on !PSERIES now. Not intended
to break EPAPR. I'm not sure of a good way to share this between
EPAPR and PSERIES, I might just make a copy of it but I'll see.

Thanks,
Nick

On Tue, Jun 30, 2020 at 03:35:08PM +1000, Nicholas Piggin wrote:

Excerpts from Paul Mackerras's message of June 30, 2020 12:27 pm:

quoted

On Sun, Jun 28, 2020 at 01:04:28AM +1000, Nicholas Piggin wrote:

quoted

KVM guests have certain restrictions and performance quirks when
using doorbells. This patch tests for KVM environment in doorbell
setup, and optimises IPI performance:

 - PowerVM guests may now use doorbells even if they are secure.

 - KVM guests no longer use doorbells if XIVE is available.

It seems, from the fact that you completely remove
kvm_para_available(), that you perhaps haven't tried building with
CONFIG_KVM_GUEST=y.

It's still there and builds:

OK, good, I missed that.

static inline int kvm_para_available(void)
{
        return IS_ENABLED(CONFIG_KVM_GUEST) && is_kvm_guest();
}

but...

quoted

Somewhat confusingly, that option is not used or
needed when building for a PAPR guest (i.e. the "pseries" platform)
but is used on non-IBM platforms using the "epapr" hypervisor
interface.

... is_kvm_guest() returns false on !PSERIES now.

And therefore kvm_para_available() returns false on all the platforms
where the code that depends on it could actually be used.

It's not correct to assume that !PSERIES means not a KVM guest.

Not intended
to break EPAPR. I'm not sure of a good way to share this between
EPAPR and PSERIES, I might just make a copy of it but I'll see.

OK, so you're doing a new version?

Regards,
Paul.

Excerpts from Paul Mackerras's message of June 30, 2020 6:26 pm:

On Tue, Jun 30, 2020 at 03:35:08PM +1000, Nicholas Piggin wrote:

quoted

Excerpts from Paul Mackerras's message of June 30, 2020 12:27 pm:

quoted

On Sun, Jun 28, 2020 at 01:04:28AM +1000, Nicholas Piggin wrote:

quoted

KVM guests have certain restrictions and performance quirks when
using doorbells. This patch tests for KVM environment in doorbell
setup, and optimises IPI performance:

 - PowerVM guests may now use doorbells even if they are secure.

 - KVM guests no longer use doorbells if XIVE is available.

It seems, from the fact that you completely remove
kvm_para_available(), that you perhaps haven't tried building with
CONFIG_KVM_GUEST=y.

It's still there and builds:

OK, good, I missed that.

quoted

static inline int kvm_para_available(void)
{
        return IS_ENABLED(CONFIG_KVM_GUEST) && is_kvm_guest();
}

but...

quoted

Somewhat confusingly, that option is not used or
needed when building for a PAPR guest (i.e. the "pseries" platform)
but is used on non-IBM platforms using the "epapr" hypervisor
interface.

... is_kvm_guest() returns false on !PSERIES now.

And therefore kvm_para_available() returns false on all the platforms
where the code that depends on it could actually be used.

It's not correct to assume that !PSERIES means not a KVM guest.

Yep, thanks for catching it.

quoted

Not intended
to break EPAPR. I'm not sure of a good way to share this between
EPAPR and PSERIES, I might just make a copy of it but I'll see.

OK, so you're doing a new version?

Just sent.

Thanks,
Nick