thr3ads.net - Xen devel - [PATCH 0/3 v5] arm: introduce psci_smp

If this information is useful, please help other people find it:
Share via:

Stefano Stabellini

2013-Apr-02 16:16 UTC

[PATCH 0/3 v5] arm: introduce psci_smp_ops and use them on Xen

Hi all,
this is the fifth version of the patch series to move virt_smp_ops out of
mach-virt and make it a generic set of reusable PSCI based smp_ops.
I added a patch to introduce smp_init to this series.
In the last patch I am using smp_init to switch to psci_smp_ops if Xen
is detected on the platform.


Jon Medhurst (1):
      ARM: Enable selection of SMP operations at boot time

Stefano Stabellini (2):
      arm: introduce psci_smp_ops
      xen/arm: introduce xen_early_init, use PSCI on xen

 arch/arm/Kconfig                      |    1 +
 arch/arm/include/asm/mach/arch.h      |    3 +
 arch/arm/include/asm/psci.h           |   29 ++++++++++++++
 arch/arm/include/asm/xen/hypervisor.h |    6 +++
 arch/arm/kernel/Makefile              |    5 ++-
 arch/arm/kernel/psci.c                |    7 +--
 arch/arm/kernel/psci_smp.c            |   67 +++++++++++++++++++++++++++++++++
 arch/arm/kernel/setup.c               |   12 +++++-
 arch/arm/mach-virt/Makefile           |    1 -
 arch/arm/mach-virt/platsmp.c          |   58 ----------------------------
 arch/arm/mach-virt/virt.c             |    3 -
 arch/arm/xen/enlighten.c              |   27 ++++++++++---
 12 files changed, 144 insertions(+), 75 deletions(-)


A branch based on "xen/arm: move to mach-virt and support SMP" and
3.9-rc3 is available here:

git://xenbits.xen.org/people/sstabellini/linux-pvhvm.git 3.9-rc3-smp-5


Cheers,

Stefano

Stefano Stabellini

2013-Apr-02 16:16 UTC

head link

[PATCH v5 1/3] arm: introduce psci_smp_ops

Rename virt_smp_ops to psci_smp_ops and move them to arch/arm/kernel/psci_smp.c.
Remove mach-virt/platsmp.c, now unused.
Compile psci_smp if CONFIG_ARM_PSCI and CONFIG_SMP.

Add a cpu_die smp_op based on psci_ops.cpu_off.

Initialize PSCI before setting smp_ops in setup_arch.
Use psci_smp_ops if the platform doesn''t provide its own smp_ops.


Changes in v5:
- document psci_operations;
- psci_init returns NULL.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Nicolas Pitre <nico@linaro.org>
Acked-by: Rob Herring <rob.herring@calxeda.com>

CC: will.deacon@arm.com
CC: arnd@arndb.de
CC: marc.zyngier@arm.com
CC: linux@arm.linux.org.uk
CC: nico@linaro.org
---
 arch/arm/include/asm/psci.h  |   29 ++++++++++++++++++
 arch/arm/kernel/Makefile     |    5 ++-
 arch/arm/kernel/psci.c       |    7 ++--
 arch/arm/kernel/psci_smp.c   |   67 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/setup.c      |    7 ++++-
 arch/arm/mach-virt/Makefile  |    1 -
 arch/arm/mach-virt/platsmp.c |   58 ------------------------------------
 arch/arm/mach-virt/virt.c    |    3 --
 8 files changed, 109 insertions(+), 68 deletions(-)
 create mode 100644 arch/arm/kernel/psci_smp.c
 delete mode 100644 arch/arm/mach-virt/platsmp.c

diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
index ce0dbe7..fd94ce2 100644
--- a/arch/arm/include/asm/psci.h
+++ b/arch/arm/include/asm/psci.h
@@ -23,6 +23,26 @@ struct psci_power_state {
 	u8	affinity_level;
 };
 
+/*
+ * cpu_suspend   Suspend the execution on a CPU
+ * @state        we don''t currently describe affinity levels, so just
pass 0.
+ * @entry_point  the first instruction to be executed on return
+ * returns 0  success, < 0 on failure
+ *
+ * cpu_off       Power down a CPU
+ * @state        we don''t currently describe affinity levels, so just
pass 0.
+ * no return on successful call
+ *
+ * cpu_on        Power up a CPU
+ * @cpuid        cpuid of target CPU, as from MPIDR
+ * @entry_point  the first instruction to be executed on return
+ * returns 0  success, < 0 on failure
+ *
+ * migrate       Migrate the context to a different CPU
+ * @cpuid        cpuid of target CPU, as from MPIDR
+ * returns 0  success, < 0 on failure
+ *
+ */
 struct psci_operations {
 	int (*cpu_suspend)(struct psci_power_state state,
 			   unsigned long entry_point);
@@ -32,5 +52,14 @@ struct psci_operations {
 };
 
 extern struct psci_operations psci_ops;
+extern struct smp_operations psci_smp_ops;
+
+#ifdef CONFIG_ARM_PSCI
+void psci_init(void);
+bool psci_smp_available(void);
+#else
+static inline void psci_init(void) { return -ENODEV; }
+static inline bool psci_smp_available(void) { return false; }
+#endif
 
 #endif /* __ASM_ARM_PSCI_H */
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 5f3338e..dd9d90a 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -82,6 +82,9 @@ obj-$(CONFIG_DEBUG_LL)	+= debug.o
 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
 
 obj-$(CONFIG_ARM_VIRT_EXT)	+= hyp-stub.o
-obj-$(CONFIG_ARM_PSCI)		+= psci.o
+ifeq ($(CONFIG_ARM_PSCI),y)
+obj-y				+= psci.o
+obj-$(CONFIG_SMP)		+= psci_smp.o
+endif
 
 extra-y := $(head-y) vmlinux.lds
diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index 3653164..4693188 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -158,7 +158,7 @@ static const struct of_device_id psci_of_match[] __initconst
= {
 	{},
 };
 
-static int __init psci_init(void)
+void __init psci_init(void)
 {
 	struct device_node *np;
 	const char *method;
@@ -166,7 +166,7 @@ static int __init psci_init(void)
 
 	np = of_find_matching_node(NULL, psci_of_match);
 	if (!np)
-		return 0;
+		return;
 
 	pr_info("probing function IDs from device-tree\n");
 
@@ -206,6 +206,5 @@ static int __init psci_init(void)
 
 out_put_node:
 	of_node_put(np);
-	return 0;
+	return;
 }
-early_initcall(psci_init);
diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
new file mode 100644
index 0000000..66b0f77
--- /dev/null
+++ b/arch/arm/kernel/psci_smp.c
@@ -0,0 +1,67 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Copyright (C) 2012 ARM Limited
+ *
+ * Author: Will Deacon <will.deacon@arm.com>
+ */
+
+#include <linux/init.h>
+#include <linux/irqchip/arm-gic.h>
+#include <linux/smp.h>
+#include <linux/of.h>
+
+#include <asm/psci.h>
+#include <asm/smp_plat.h>
+
+extern void secondary_startup(void);
+
+static int __cpuinit psci_boot_secondary(unsigned int cpu,
+					 struct task_struct *idle)
+{
+	if (psci_ops.cpu_on)
+		return psci_ops.cpu_on(cpu_logical_map(cpu),
+				       __pa(secondary_startup));
+	return -ENODEV;
+}
+
+static void __cpuinit psci_secondary_init(unsigned int cpu)
+{
+	gic_secondary_init(0);
+}
+
+#ifdef CONFIG_HOTPLUG_CPU
+void __ref psci_cpu_die(unsigned int cpu)
+{
+       const struct psci_power_state ps = {
+               .type = PSCI_POWER_STATE_TYPE_POWER_DOWN,
+       };
+
+       if (psci_ops.cpu_off)
+               psci_ops.cpu_off(ps);
+
+       /* We should never return */
+       panic("psci: cpu %d failed to shutdown\n", cpu);
+}
+#else
+#define psci_cpu_die NULL
+#endif
+
+bool __init psci_smp_available(void)
+{
+	/* is cpu_on available at least? */
+	return (psci_ops.cpu_on != NULL);
+}
+
+struct smp_operations __initdata psci_smp_ops = {
+	.smp_secondary_init	= psci_secondary_init,
+	.smp_boot_secondary	= psci_boot_secondary,
+	.cpu_die		= psci_cpu_die,
+};
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 3f6cbb2..341efaa 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -36,6 +36,7 @@
 #include <asm/cputype.h>
 #include <asm/elf.h>
 #include <asm/procinfo.h>
+#include <asm/psci.h>
 #include <asm/sections.h>
 #include <asm/setup.h>
 #include <asm/smp_plat.h>
@@ -766,9 +767,13 @@ void __init setup_arch(char **cmdline_p)
 	unflatten_device_tree();
 
 	arm_dt_init_cpu_maps();
+	psci_init();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
-		smp_set_ops(mdesc->smp);
+		if (mdesc->smp)
+			smp_set_ops(mdesc->smp);
+		else if (psci_smp_available())
+			smp_set_ops(&psci_smp_ops);
 		smp_init_cpus();
 	}
 #endif
diff --git a/arch/arm/mach-virt/Makefile b/arch/arm/mach-virt/Makefile
index 042afc1..7ddbfa6 100644
--- a/arch/arm/mach-virt/Makefile
+++ b/arch/arm/mach-virt/Makefile
@@ -3,4 +3,3 @@
 #
 
 obj-y					:= virt.o
-obj-$(CONFIG_SMP)			+= platsmp.o
diff --git a/arch/arm/mach-virt/platsmp.c b/arch/arm/mach-virt/platsmp.c
deleted file mode 100644
index 8badaab..0000000
--- a/arch/arm/mach-virt/platsmp.c
+++ /dev/null
@@ -1,58 +0,0 @@
-/*
- * Dummy Virtual Machine - does what it says on the tin.
- *
- * Copyright (C) 2012 ARM Ltd
- * Author: Will Deacon <will.deacon@arm.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#include <linux/init.h>
-#include <linux/smp.h>
-#include <linux/of.h>
-
-#include <linux/irqchip/arm-gic.h>
-
-#include <asm/psci.h>
-#include <asm/smp_plat.h>
-
-extern void secondary_startup(void);
-
-static void __init virt_smp_init_cpus(void)
-{
-}
-
-static void __init virt_smp_prepare_cpus(unsigned int max_cpus)
-{
-}
-
-static int __cpuinit virt_boot_secondary(unsigned int cpu,
-					 struct task_struct *idle)
-{
-	if (psci_ops.cpu_on)
-		return psci_ops.cpu_on(cpu_logical_map(cpu),
-				       __pa(secondary_startup));
-	return -ENODEV;
-}
-
-static void __cpuinit virt_secondary_init(unsigned int cpu)
-{
-	gic_secondary_init(0);
-}
-
-struct smp_operations __initdata virt_smp_ops = {
-	.smp_init_cpus		= virt_smp_init_cpus,
-	.smp_prepare_cpus	= virt_smp_prepare_cpus,
-	.smp_secondary_init	= virt_secondary_init,
-	.smp_boot_secondary	= virt_boot_secondary,
-};
diff --git a/arch/arm/mach-virt/virt.c b/arch/arm/mach-virt/virt.c
index 528c05e..c417752 100644
--- a/arch/arm/mach-virt/virt.c
+++ b/arch/arm/mach-virt/virt.c
@@ -44,12 +44,9 @@ static const char *virt_dt_match[] = {
 	NULL
 };
 
-extern struct smp_operations virt_smp_ops;
-
 DT_MACHINE_START(VIRT, "Dummy Virtual Machine")
 	.init_irq	= irqchip_init,
 	.init_time	= virt_timer_init,
 	.init_machine	= virt_init,
-	.smp		= smp_ops(virt_smp_ops),
 	.dt_compat	= virt_dt_match,
 MACHINE_END
-- 
1.7.2.5

Stefano Stabellini

2013-Apr-02 16:16 UTC

head link

[PATCH v5 2/3] ARM: Enable selection of SMP operations at boot time

From: Jon Medhurst <tixy@linaro.org>

Add a new ''smp_init'' hook to machine_desc so platforms can
specify a
function to be used to setup smp ops instead of having a statically
defined value.

Signed-off-by: Jon Medhurst <tixy@linaro.org>
Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/include/asm/mach/arch.h |    3 +++
 arch/arm/kernel/setup.c          |    4 +++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 308ad7d..c01bf53 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -16,8 +16,10 @@ struct pt_regs;
 struct smp_operations;
 #ifdef CONFIG_SMP
 #define smp_ops(ops) (&(ops))
+#define smp_init_ops(ops) (&(ops))
 #else
 #define smp_ops(ops) (struct smp_operations *)NULL
+#define smp_init_ops(ops) (void (*)(void))NULL
 #endif
 
 struct machine_desc {
@@ -41,6 +43,7 @@ struct machine_desc {
 	unsigned char		reserve_lp2 :1;	/* never has lp2	*/
 	char			restart_mode;	/* default restart mode	*/
 	struct smp_operations	*smp;		/* SMP operations	*/
+	void			(*smp_init)(void);
 	void			(*fixup)(struct tag *, char **,
 					 struct meminfo *);
 	void			(*reserve)(void);/* reserve mem blocks	*/
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 341efaa..7e193df 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -770,7 +770,9 @@ void __init setup_arch(char **cmdline_p)
 	psci_init();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
-		if (mdesc->smp)
+		if (mdesc->smp_init)
+			(*mdesc->smp_init)();
+		else if (mdesc->smp)
 			smp_set_ops(mdesc->smp);
 		else if (psci_smp_available())
 			smp_set_ops(&psci_smp_ops);
-- 
1.7.2.5

Stefano Stabellini

2013-Apr-02 16:16 UTC

head link

[PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen

Split xen_guest_init in two functions, one of them (xen_early_init) is
going to be called very early from setup_arch.

Change machine_desc->smp_init to xen_smp_init if Xen is present on the
platform. xen_smp_init just sets smp_ops to psci_smp_ops.

Introduce a dependency for ARM_PSCI in XEN.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/Kconfig                      |    1 +
 arch/arm/include/asm/xen/hypervisor.h |    6 ++++++
 arch/arm/kernel/setup.c               |    3 +++
 arch/arm/xen/enlighten.c              |   27 ++++++++++++++++++++-------
 4 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 2c3bdce..66658d3 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1891,6 +1891,7 @@ config XEN
 	bool "Xen guest support on ARM (EXPERIMENTAL)"
 	depends on ARM && AEABI && OF
 	depends on CPU_V7 && !CPU_V6
+	depends on ARM_PSCI
 	depends on !GENERIC_ATOMIC64
 	help
 	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
diff --git a/arch/arm/include/asm/xen/hypervisor.h
b/arch/arm/include/asm/xen/hypervisor.h
index d7ab99a..17b3ea2 100644
--- a/arch/arm/include/asm/xen/hypervisor.h
+++ b/arch/arm/include/asm/xen/hypervisor.h
@@ -16,4 +16,10 @@ static inline enum paravirt_lazy_mode
paravirt_get_lazy_mode(void)
 	return PARAVIRT_LAZY_NONE;
 }
 
+#ifdef CONFIG_XEN
+void xen_early_init(void);
+#else
+static inline void xen_early_init(void) { return; }
+#endif
+
 #endif /* _ASM_ARM_XEN_HYPERVISOR_H */
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 7e193df..67d911f 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -55,6 +55,8 @@
 #include <asm/unwind.h>
 #include <asm/memblock.h>
 #include <asm/virt.h>
+#include <asm/xen/hypervisor.h>
+#include <xen/xen.h>
 
 #include "atags.h"
 #include "tcm.h"
@@ -768,6 +770,7 @@ void __init setup_arch(char **cmdline_p)
 
 	arm_dt_init_cpu_maps();
 	psci_init();
+	xen_early_init();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
 		if (mdesc->smp_init)
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index b002822..ee09357 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -11,6 +11,8 @@
 #include <xen/xenbus.h>
 #include <xen/page.h>
 #include <xen/xen-ops.h>
+#include <asm/mach/arch.h>
+#include <asm/psci.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
 #include <linux/interrupt.h>
@@ -176,27 +178,30 @@ static int __init xen_secondary_init(unsigned int cpu)
 	return 0;
 }
 
+static void __init xen_smp_init(void)
+{
+	if (psci_smp_available())
+		smp_set_ops(&psci_smp_ops);
+}
+
 /*
  * see Documentation/devicetree/bindings/arm/xen.txt for the
  * documentation of the Xen Device Tree format.
  */
 #define GRANT_TABLE_PHYSADDR 0
-static int __init xen_guest_init(void)
+void __init xen_early_init(void)
 {
-	struct xen_add_to_physmap xatp;
-	static struct shared_info *shared_info_page = 0;
 	struct device_node *node;
 	int len;
 	const char *s = NULL;
 	const char *version = NULL;
 	const char *xen_prefix = "xen,xen-";
 	struct resource res;
-	int i;
 
 	node = of_find_compatible_node(NULL, NULL, "xen,xen");
 	if (!node) {
 		pr_debug("No Xen support\n");
-		return 0;
+		return;
 	}
 	s = of_get_property(node, "compatible", &len);
 	if (strlen(xen_prefix) + 3  < len &&
@@ -204,15 +209,23 @@ static int __init xen_guest_init(void)
 		version = s + strlen(xen_prefix);
 	if (version == NULL) {
 		pr_debug("Xen version not found\n");
-		return 0;
+		return;
 	}
 	if (of_address_to_resource(node, GRANT_TABLE_PHYSADDR, &res))
-		return 0;
+		return;
 	xen_hvm_resume_frames = res.start >> PAGE_SHIFT;
 	xen_events_irq = irq_of_parse_and_map(node, 0);
 	pr_info("Xen %s support found, events_irq=%d
gnttab_frame_pfn=%lx\n",
 			version, xen_events_irq, xen_hvm_resume_frames);
 	xen_domain_type = XEN_HVM_DOMAIN;
+	machine_desc->smp_init = xen_smp_init;
+}
+
+static int __init xen_guest_init(void)
+{
+	struct xen_add_to_physmap xatp;
+	static struct shared_info *shared_info_page = 0;
+	int i;
 
 	xen_setup_features();
 	if (xen_feature(XENFEAT_dom0))
-- 
1.7.2.5

Will Deacon

2013-Apr-02 16:46 UTC

head link

Re: [PATCH v5 1/3] arm: introduce psci_smp_ops

On Tue, Apr 02, 2013 at 05:16:12PM +0100, Stefano Stabellini
wrote:> Rename virt_smp_ops to psci_smp_ops and move them to
arch/arm/kernel/psci_smp.c.
> Remove mach-virt/platsmp.c, now unused.
> Compile psci_smp if CONFIG_ARM_PSCI and CONFIG_SMP.
> 
> Add a cpu_die smp_op based on psci_ops.cpu_off.
> 
> Initialize PSCI before setting smp_ops in setup_arch.
> Use psci_smp_ops if the platform doesn''t provide its own smp_ops.
> 
> 
> Changes in v5:
> - document psci_operations;
> - psci_init returns NULL.
> 
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Nicolas Pitre <nico@linaro.org>
> Acked-by: Rob Herring <rob.herring@calxeda.com>
> 
> CC: will.deacon@arm.com
> CC: arnd@arndb.de
> CC: marc.zyngier@arm.com
> CC: linux@arm.linux.org.uk
> CC: nico@linaro.org
> ---
>  arch/arm/include/asm/psci.h  |   29 ++++++++++++++++++
>  arch/arm/kernel/Makefile     |    5 ++-
>  arch/arm/kernel/psci.c       |    7 ++--
>  arch/arm/kernel/psci_smp.c   |   67
++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/kernel/setup.c      |    7 ++++-
>  arch/arm/mach-virt/Makefile  |    1 -
>  arch/arm/mach-virt/platsmp.c |   58 ------------------------------------
>  arch/arm/mach-virt/virt.c    |    3 --
>  8 files changed, 109 insertions(+), 68 deletions(-)
>  create mode 100644 arch/arm/kernel/psci_smp.c
>  delete mode 100644 arch/arm/mach-virt/platsmp.c
Reviewed-by: Will Deacon <will.deacon@arm.com>

Will

Nicolas Pitre

2013-Apr-02 17:34 UTC

head link

Re: [PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen

On Tue, 2 Apr 2013, Stefano Stabellini wrote:
> Split xen_guest_init in two functions, one of them (xen_early_init) is
> going to be called very early from setup_arch.
> 
> Change machine_desc->smp_init to xen_smp_init if Xen is present on the
> platform. xen_smp_init just sets smp_ops to psci_smp_ops.
> 
> Introduce a dependency for ARM_PSCI in XEN.
The Kconfig stuff should be more understandable to "normal" users 
configuring the kernel.  Hence it might make more sense for Xen to 
select PSCI rather than making it a prerequisite.

[...]> @@ -176,27 +178,30 @@ static int __init xen_secondary_init(unsigned int
cpu)
>  	return 0;
>  }
>  
> +static void __init xen_smp_init(void)
> +{
> +	if (psci_smp_available())
> +		smp_set_ops(&psci_smp_ops);
> +}
> +
I still don''t understand why you need to do this.  Why can''t
you just
rely on the next priority which is to set PSCI ops by default if 
available?  Using this hook for Xen doesn''t look justified. As it is, 
you break MCPM.

As I explained to you, MCPM will end up using PSCI as well when 
available, which I think should be sufficient for your concern.


Nicolas

Nicolas Pitre

2013-Apr-02 17:41 UTC

head link

Re: [PATCH v5 1/3] arm: introduce psci_smp_ops

On Tue, 2 Apr 2013, Stefano Stabellini wrote:
> Rename virt_smp_ops to psci_smp_ops and move them to
arch/arm/kernel/psci_smp.c.
> Remove mach-virt/platsmp.c, now unused.
> Compile psci_smp if CONFIG_ARM_PSCI and CONFIG_SMP.
> 
> Add a cpu_die smp_op based on psci_ops.cpu_off.
> 
> Initialize PSCI before setting smp_ops in setup_arch.
> Use psci_smp_ops if the platform doesn''t provide its own smp_ops.
[...]
> @@ -766,9 +767,13 @@ void __init setup_arch(char **cmdline_p)
>  	unflatten_device_tree();
>  
>  	arm_dt_init_cpu_maps();
> +	psci_init();
>  #ifdef CONFIG_SMP
>  	if (is_smp()) {
> -		smp_set_ops(mdesc->smp);
> +		if (mdesc->smp)
> +			smp_set_ops(mdesc->smp);
> +		else if (psci_smp_available())
> +			smp_set_ops(&psci_smp_ops);
Didn''t we agree to do this the other way around, i.e. 

	if (psci_smp_available())
		smp_set_ops(&psci_smp_ops);
	else if (mdesc->smp)
		smp_set_ops(mdesc->smp);

?

This way you won''t need the ->smp_init for Xen and that
won''t conflict
with MCPM.


Nicolas

Stefano Stabellini

2013-Apr-03 11:28 UTC

head link

Re: [PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen

On Tue, 2 Apr 2013, Nicolas Pitre wrote:> On Tue, 2 Apr 2013, Stefano Stabellini wrote:
> 
> > Split xen_guest_init in two functions, one of them (xen_early_init) is
> > going to be called very early from setup_arch.
> > 
> > Change machine_desc->smp_init to xen_smp_init if Xen is present on
the
> > platform. xen_smp_init just sets smp_ops to psci_smp_ops.
> > 
> > Introduce a dependency for ARM_PSCI in XEN.
> 
> The Kconfig stuff should be more understandable to "normal" users
> configuring the kernel.  Hence it might make more sense for Xen to 
> select PSCI rather than making it a prerequisite.
You are right, I''ll do that.

> [...]
> > @@ -176,27 +178,30 @@ static int __init xen_secondary_init(unsigned
int cpu)
> >  	return 0;
> >  }
> >  
> > +static void __init xen_smp_init(void)
> > +{
> > +	if (psci_smp_available())
> > +		smp_set_ops(&psci_smp_ops);
> > +}
> > +
> 
> I still don''t understand why you need to do this.  Why
can''t you just
> rely on the next priority which is to set PSCI ops by default if 
> available?  Using this hook for Xen doesn''t look justified. As it
is,
> you break MCPM.
> 
> As I explained to you, MCPM will end up using PSCI as well when 
> available, which I think should be sufficient for your concern.
The smp_init hook is not limited to MCPM or the versatile express
platform. It''s a generic hook that can be used by any platform and can
override the platform smp_ops or the psci_smp_ops depending on platform
specific configurations.

Configurations that I am pretty sure won''t be available on Xen anyway,
while I am certain that using psci_smp_ops would work.

It seems to me that relying on the fact that only versatile express and
only MCPM use smp_init, even though it might work today, it''s very
fragile and could break tomorrow without any of us noticing.

Stefano Stabellini

2013-Apr-03 11:29 UTC

head link

Re: [PATCH v5 1/3] arm: introduce psci_smp_ops

On Tue, 2 Apr 2013, Nicolas Pitre wrote:> On Tue, 2 Apr 2013, Stefano Stabellini wrote:
> 
> > Rename virt_smp_ops to psci_smp_ops and move them to
arch/arm/kernel/psci_smp.c.
> > Remove mach-virt/platsmp.c, now unused.
> > Compile psci_smp if CONFIG_ARM_PSCI and CONFIG_SMP.
> > 
> > Add a cpu_die smp_op based on psci_ops.cpu_off.
> > 
> > Initialize PSCI before setting smp_ops in setup_arch.
> > Use psci_smp_ops if the platform doesn''t provide its own
smp_ops.
> 
> [...]
> 
> > @@ -766,9 +767,13 @@ void __init setup_arch(char **cmdline_p)
> >  	unflatten_device_tree();
> >  
> >  	arm_dt_init_cpu_maps();
> > +	psci_init();
> >  #ifdef CONFIG_SMP
> >  	if (is_smp()) {
> > -		smp_set_ops(mdesc->smp);
> > +		if (mdesc->smp)
> > +			smp_set_ops(mdesc->smp);
> > +		else if (psci_smp_available())
> > +			smp_set_ops(&psci_smp_ops);
> 
> Didn''t we agree to do this the other way around, i.e. 
> 
> 	if (psci_smp_available())
> 		smp_set_ops(&psci_smp_ops);
> 	else if (mdesc->smp)
> 		smp_set_ops(mdesc->smp);
> 
> ?
Yes, we agreed about it.
However that change was in a separate patch that I have dropped from
this version of the series, that is why it is missing.

I''ll add back the change but I''ll keep it in a separate patch
to leave
the original one, now acked twice and reviewed once, untouched.

> This way you won''t need the ->smp_init for Xen and that
won''t conflict
> with MCPM.
See my other reply on the subject.

Nicolas Pitre

2013-Apr-03 14:00 UTC

head link

Re: [PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen

On Wed, 3 Apr 2013, Stefano Stabellini wrote:
> On Tue, 2 Apr 2013, Nicolas Pitre wrote:
> > On Tue, 2 Apr 2013, Stefano Stabellini wrote:
> > 
> > > @@ -176,27 +178,30 @@ static int __init
xen_secondary_init(unsigned int cpu)
> > >  	return 0;
> > >  }
> > >  
> > > +static void __init xen_smp_init(void)
> > > +{
> > > +	if (psci_smp_available())
> > > +		smp_set_ops(&psci_smp_ops);
> > > +}
> > > +
> > 
> > I still don''t understand why you need to do this.  Why
can''t you just
> > rely on the next priority which is to set PSCI ops by default if 
> > available?  Using this hook for Xen doesn''t look justified.
As it is,
> > you break MCPM.
> > 
> > As I explained to you, MCPM will end up using PSCI as well when 
> > available, which I think should be sufficient for your concern.
> 
> The smp_init hook is not limited to MCPM or the versatile express
> platform. It''s a generic hook that can be used by any platform and
can
> override the platform smp_ops or the psci_smp_ops depending on platform
> specific configurations.
> 
> Configurations that I am pretty sure won''t be available on Xen
anyway,
> while I am certain that using psci_smp_ops would work.
> 
> It seems to me that relying on the fact that only versatile express and
> only MCPM use smp_init, even though it might work today, it''s very
> fragile and could break tomorrow without any of us noticing.
I claim: this breaks MCPM today.

You claim: the alternative _could_ break with Xen tomorrow.

Could we please wait until there is an actual problem with Xen before 
being overly defensive in the code?


Nicolas

Stefano Stabellini

2013-Apr-03 14:47 UTC

head link

Re: [PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen

On Wed, 3 Apr 2013, Nicolas Pitre wrote:> On Wed, 3 Apr 2013, Stefano Stabellini wrote:
> 
> > On Tue, 2 Apr 2013, Nicolas Pitre wrote:
> > > On Tue, 2 Apr 2013, Stefano Stabellini wrote:
> > > 
> > > > @@ -176,27 +178,30 @@ static int __init
xen_secondary_init(unsigned int cpu)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static void __init xen_smp_init(void)
> > > > +{
> > > > +	if (psci_smp_available())
> > > > +		smp_set_ops(&psci_smp_ops);
> > > > +}
> > > > +
> > > 
> > > I still don''t understand why you need to do this.  Why
can''t you just
> > > rely on the next priority which is to set PSCI ops by default if 
> > > available?  Using this hook for Xen doesn''t look
justified. As it is,
> > > you break MCPM.
> > > 
> > > As I explained to you, MCPM will end up using PSCI as well when 
> > > available, which I think should be sufficient for your concern.
> > 
> > The smp_init hook is not limited to MCPM or the versatile express
> > platform. It''s a generic hook that can be used by any
platform and can
> > override the platform smp_ops or the psci_smp_ops depending on
platform
> > specific configurations.
> > 
> > Configurations that I am pretty sure won''t be available on
Xen anyway,
> > while I am certain that using psci_smp_ops would work.
> > 
> > It seems to me that relying on the fact that only versatile express
and
> > only MCPM use smp_init, even though it might work today, it''s
very
> > fragile and could break tomorrow without any of us noticing.
> 
> I claim: this breaks MCPM today.
>
> You claim: the alternative _could_ break with Xen tomorrow.
> 
> Could we please wait until there is an actual problem with Xen before 
> being overly defensive in the code?
Sorry, I realized that I should have explained myself better.

The smp_init patch together with the MCPM patch series
(http://marc.info/?l=linux-arm-kernel&m=136004188122492) breaks Xen Dom0
SMP support on versatile express *today* (ifndef CONFIG_CLUSTER_PM):
smp_init is not NULL on versatile express anymore therefore smp_init is
going to be called, causing vexpress_smp_ops to be selected even if psci
is on device tree.

On the other hand if this patch was applied, xen_smp_init would override
smp_init making sure that psci_smp_ops is used on Xen and everything
would work as expected.


So as it stands today, we need this patch for regular Xen Dom0 SMP
support.


BTW I have actually tried it on my versatile express machine to make
sure that it is correct :)

Xen devel - Apr 2013 - [PATCH 0/3 v5] arm: introduce psci_smp_ops and use them on Xen

[PATCH 0/3 v5] arm: introduce psci_smp_ops and use them on Xen

[PATCH v5 1/3] arm: introduce psci_smp_ops

[PATCH v5 2/3] ARM: Enable selection of SMP operations at boot time

[PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen

Re: [PATCH v5 1/3] arm: introduce psci_smp_ops

Re: [PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen

Re: [PATCH v5 1/3] arm: introduce psci_smp_ops

Re: [PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen

Re: [PATCH v5 1/3] arm: introduce psci_smp_ops

Re: [PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen

Re: [PATCH v5 3/3] xen/arm: introduce xen_early_init, use PSCI on xen