Xen devel - Mar 2013 - [PATCH v3] [RFC] arm: use PSCI if available

Check for the presence of PSCI before setting smp_ops, use PSCI if it is
available.

This is useful because at least when running on Xen it''s possible to
have a
PSCI node for example on a Versatile Express or an Exynos5 machine. In these
cases the PSCI SMP calls should be the ones to be called.

Remove virt_smp_ops and platsmp.c from mach-virt because they aren''t
needed
anymore.



This patch was originally part of this series:

http://marc.info/?l=linux-arm-kernel&m=136430903110734&w=2

I am keeping it separate now since it is the only non-obvious change and
it is not Xen related.




Changes in v3:
- move the call to psci_init to setup_arch;
- export psci_smp_ops from psci.h;
- introduce psci_smp_available;
- introduce stub functions for psci_init and psci_smp_available ifndef
CONFIG_ARM_PSCI;
- only compile psci_smp functions ifdef CONFIG_SMP.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.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  |    9 ++++
 arch/arm/kernel/psci.c       |   97 ++++++++++++++++++++++++++++++++++--------
 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 -
 6 files changed, 94 insertions(+), 81 deletions(-)
 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..ddef231 100644
--- a/arch/arm/include/asm/psci.h
+++ b/arch/arm/include/asm/psci.h
@@ -32,5 +32,14 @@ struct psci_operations {
 };
 
 extern struct psci_operations psci_ops;
+extern struct smp_operations psci_smp_ops;
+
+#ifdef CONFIG_ARM_PSCI
+int psci_init(void);
+bool psci_smp_available(void);
+#else
+static inline int 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/psci.c b/arch/arm/kernel/psci.c
index 3653164..90f0839 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -16,6 +16,7 @@
 #define pr_fmt(fmt) "psci: " fmt
 
 #include <linux/init.h>
+#include <linux/irqchip/arm-gic.h>
 #include <linux/of.h>
 
 #include <asm/compiler.h>
@@ -23,8 +24,9 @@
 #include <asm/opcodes-sec.h>
 #include <asm/opcodes-virt.h>
 #include <asm/psci.h>
+#include <asm/smp_plat.h>
 
-struct psci_operations psci_ops;
+extern void secondary_startup(void);
 
 static int (*invoke_psci_fn)(u32, u32, u32, u32);
 
@@ -36,7 +38,11 @@ enum psci_function {
 	PSCI_FN_MAX,
 };
 
-static u32 psci_function_id[PSCI_FN_MAX];
+struct psci_function_desc {
+	enum psci_function func;
+	bool valid;
+};
+static struct psci_function_desc psci_function_id[PSCI_FN_MAX];
 
 #define PSCI_RET_SUCCESS		0
 #define PSCI_RET_EOPNOTSUPP		-1
@@ -116,7 +122,10 @@ static int psci_cpu_suspend(struct psci_power_state state,
 	int err;
 	u32 fn, power_state;
 
-	fn = psci_function_id[PSCI_FN_CPU_SUSPEND];
+	if (!psci_function_id[PSCI_FN_CPU_SUSPEND].valid)
+		return -ENOSYS;
+
+	fn = psci_function_id[PSCI_FN_CPU_SUSPEND].func;
 	power_state = psci_power_state_pack(state);
 	err = invoke_psci_fn(fn, power_state, entry_point, 0);
 	return psci_to_linux_errno(err);
@@ -127,7 +136,10 @@ static int psci_cpu_off(struct psci_power_state state)
 	int err;
 	u32 fn, power_state;
 
-	fn = psci_function_id[PSCI_FN_CPU_OFF];
+	if (!psci_function_id[PSCI_FN_CPU_OFF].valid)
+		return -ENOSYS;
+
+	fn = psci_function_id[PSCI_FN_CPU_OFF].func;
 	power_state = psci_power_state_pack(state);
 	err = invoke_psci_fn(fn, power_state, 0, 0);
 	return psci_to_linux_errno(err);
@@ -138,7 +150,10 @@ static int psci_cpu_on(unsigned long cpuid, unsigned long
entry_point)
 	int err;
 	u32 fn;
 
-	fn = psci_function_id[PSCI_FN_CPU_ON];
+	if (!psci_function_id[PSCI_FN_CPU_ON].valid)
+		return -ENOSYS;
+
+	fn = psci_function_id[PSCI_FN_CPU_ON].func;
 	err = invoke_psci_fn(fn, cpuid, entry_point, 0);
 	return psci_to_linux_errno(err);
 }
@@ -148,25 +163,64 @@ static int psci_migrate(unsigned long cpuid)
 	int err;
 	u32 fn;
 
-	fn = psci_function_id[PSCI_FN_MIGRATE];
+	if (!psci_function_id[PSCI_FN_MIGRATE].valid)
+		return -ENOSYS;
+
+	fn = psci_function_id[PSCI_FN_MIGRATE].func;
 	err = invoke_psci_fn(fn, cpuid, 0, 0);
 	return psci_to_linux_errno(err);
 }
 
+struct psci_operations psci_ops = {
+	.cpu_suspend = psci_cpu_suspend,
+	.cpu_off     = psci_cpu_off,
+	.cpu_on      = psci_cpu_on,
+	.migrate     = psci_migrate,
+};
+
+#ifdef CONFIG_SMP
+static void __init psci_smp_init_cpus(void)
+{
+}
+
+static void __init psci_smp_prepare_cpus(unsigned int max_cpus)
+{
+}
+
+static int __cpuinit psci_boot_secondary(unsigned int cpu,
+					 struct task_struct *idle)
+{
+	return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup));
+}
+
+static void __cpuinit psci_secondary_init(unsigned int cpu)
+{
+	gic_secondary_init(0);
+}
+
+struct smp_operations __initdata psci_smp_ops = {
+	.smp_init_cpus		= psci_smp_init_cpus,
+	.smp_prepare_cpus	= psci_smp_prepare_cpus,
+	.smp_secondary_init	= psci_secondary_init,
+	.smp_boot_secondary	= psci_boot_secondary,
+};
+#endif
+
 static const struct of_device_id psci_of_match[] __initconst = {
 	{ .compatible = "arm,psci",	},
 	{},
 };
 
-static int __init psci_init(void)
+int __init psci_init(void)
 {
 	struct device_node *np;
 	const char *method;
 	u32 id;
+	int rc = -EINVAL;
 
 	np = of_find_matching_node(NULL, psci_of_match);
 	if (!np)
-		return 0;
+		return -ENODEV;
 
 	pr_info("probing function IDs from device-tree\n");
 
@@ -185,27 +239,34 @@ static int __init psci_init(void)
 	}
 
 	if (!of_property_read_u32(np, "cpu_suspend", &id)) {
-		psci_function_id[PSCI_FN_CPU_SUSPEND] = id;
-		psci_ops.cpu_suspend = psci_cpu_suspend;
+		psci_function_id[PSCI_FN_CPU_SUSPEND].func = id;
+		psci_function_id[PSCI_FN_CPU_SUSPEND].valid = true;
 	}
 
 	if (!of_property_read_u32(np, "cpu_off", &id)) {
-		psci_function_id[PSCI_FN_CPU_OFF] = id;
-		psci_ops.cpu_off = psci_cpu_off;
+		psci_function_id[PSCI_FN_CPU_OFF].func = id;
+		psci_function_id[PSCI_FN_CPU_OFF].valid = true;
 	}
 
 	if (!of_property_read_u32(np, "cpu_on", &id)) {
-		psci_function_id[PSCI_FN_CPU_ON] = id;
-		psci_ops.cpu_on = psci_cpu_on;
+		psci_function_id[PSCI_FN_CPU_ON].func = id;
+		psci_function_id[PSCI_FN_CPU_ON].valid = true;
 	}
 
 	if (!of_property_read_u32(np, "migrate", &id)) {
-		psci_function_id[PSCI_FN_MIGRATE] = id;
-		psci_ops.migrate = psci_migrate;
+		psci_function_id[PSCI_FN_MIGRATE].func = id;
+		psci_function_id[PSCI_FN_MIGRATE].valid = true;
 	}
 
+	rc = 0;
+
 out_put_node:
 	of_node_put(np);
-	return 0;
+	return rc;
+}
+
+bool __init psci_smp_available(void)
+{
+	/* is cpu_on available at least? */
+	return psci_function_id[PSCI_FN_CPU_ON].valid;
 }
-early_initcall(psci_init);
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 3f6cbb2..c7e50dd 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 (psci_smp_available())
+			smp_set_ops(&psci_smp_ops);
+		else
+			smp_set_ops(mdesc->smp);
 		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

On 27/03/13 12:50, Stefano Stabellini wrote:
> Check for the presence of PSCI before setting smp_ops, use PSCI if it is
> available.
> 
> This is useful because at least when running on Xen it''s possible
to have a
> PSCI node for example on a Versatile Express or an Exynos5 machine. In
these
> cases the PSCI SMP calls should be the ones to be called.
> 
> Remove virt_smp_ops and platsmp.c from mach-virt because they
aren''t needed
> anymore.
> 
> 
> 
> This patch was originally part of this series:
> 
> http://marc.info/?l=linux-arm-kernel&m=136430903110734&w=2
> 
> I am keeping it separate now since it is the only non-obvious change and
> it is not Xen related.
> 
> 
> 
> 
> Changes in v3:
> - move the call to psci_init to setup_arch;
> - export psci_smp_ops from psci.h;
> - introduce psci_smp_available;
> - introduce stub functions for psci_init and psci_smp_available ifndef
> CONFIG_ARM_PSCI;
> - only compile psci_smp functions ifdef CONFIG_SMP.
> 
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.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  |    9 ++++
>  arch/arm/kernel/psci.c       |   97
++++++++++++++++++++++++++++++++++--------
>  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 -
>  6 files changed, 94 insertions(+), 81 deletions(-)
>  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..ddef231 100644
> --- a/arch/arm/include/asm/psci.h
> +++ b/arch/arm/include/asm/psci.h
> @@ -32,5 +32,14 @@ struct psci_operations {
>  };
>  
>  extern struct psci_operations psci_ops;
> +extern struct smp_operations psci_smp_ops;
> +
> +#ifdef CONFIG_ARM_PSCI
> +int psci_init(void);
> +bool psci_smp_available(void);
> +#else
> +static inline int 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/psci.c b/arch/arm/kernel/psci.c
> index 3653164..90f0839 100644
> --- a/arch/arm/kernel/psci.c
> +++ b/arch/arm/kernel/psci.c
> @@ -16,6 +16,7 @@
>  #define pr_fmt(fmt) "psci: " fmt
>  
>  #include <linux/init.h>
> +#include <linux/irqchip/arm-gic.h>
>  #include <linux/of.h>
>  
>  #include <asm/compiler.h>
> @@ -23,8 +24,9 @@
>  #include <asm/opcodes-sec.h>
>  #include <asm/opcodes-virt.h>
>  #include <asm/psci.h>
> +#include <asm/smp_plat.h>
>  
> -struct psci_operations psci_ops;
> +extern void secondary_startup(void);
>  
>  static int (*invoke_psci_fn)(u32, u32, u32, u32);
>  
> @@ -36,7 +38,11 @@ enum psci_function {
>  	PSCI_FN_MAX,
>  };
>  
> -static u32 psci_function_id[PSCI_FN_MAX];
> +struct psci_function_desc {
> +	enum psci_function func;
> +	bool valid;
> +};
> +static struct psci_function_desc psci_function_id[PSCI_FN_MAX];
>  
>  #define PSCI_RET_SUCCESS		0
>  #define PSCI_RET_EOPNOTSUPP		-1
> @@ -116,7 +122,10 @@ static int psci_cpu_suspend(struct psci_power_state
state,
>  	int err;
>  	u32 fn, power_state;
>  
> -	fn = psci_function_id[PSCI_FN_CPU_SUSPEND];
> +	if (!psci_function_id[PSCI_FN_CPU_SUSPEND].valid)
> +		return -ENOSYS;
> +
> +	fn = psci_function_id[PSCI_FN_CPU_SUSPEND].func;
>  	power_state = psci_power_state_pack(state);
>  	err = invoke_psci_fn(fn, power_state, entry_point, 0);
>  	return psci_to_linux_errno(err);
> @@ -127,7 +136,10 @@ static int psci_cpu_off(struct psci_power_state state)
>  	int err;
>  	u32 fn, power_state;
>  
> -	fn = psci_function_id[PSCI_FN_CPU_OFF];
> +	if (!psci_function_id[PSCI_FN_CPU_OFF].valid)
> +		return -ENOSYS;
> +
> +	fn = psci_function_id[PSCI_FN_CPU_OFF].func;
>  	power_state = psci_power_state_pack(state);
>  	err = invoke_psci_fn(fn, power_state, 0, 0);
>  	return psci_to_linux_errno(err);
> @@ -138,7 +150,10 @@ static int psci_cpu_on(unsigned long cpuid, unsigned
long entry_point)
>  	int err;
>  	u32 fn;
>  
> -	fn = psci_function_id[PSCI_FN_CPU_ON];
> +	if (!psci_function_id[PSCI_FN_CPU_ON].valid)
> +		return -ENOSYS;
> +
> +	fn = psci_function_id[PSCI_FN_CPU_ON].func;
>  	err = invoke_psci_fn(fn, cpuid, entry_point, 0);
>  	return psci_to_linux_errno(err);
>  }
> @@ -148,25 +163,64 @@ static int psci_migrate(unsigned long cpuid)
>  	int err;
>  	u32 fn;
>  
> -	fn = psci_function_id[PSCI_FN_MIGRATE];
> +	if (!psci_function_id[PSCI_FN_MIGRATE].valid)
> +		return -ENOSYS;
> +
> +	fn = psci_function_id[PSCI_FN_MIGRATE].func;
>  	err = invoke_psci_fn(fn, cpuid, 0, 0);
>  	return psci_to_linux_errno(err);
>  }
>  
> +struct psci_operations psci_ops = {
> +	.cpu_suspend = psci_cpu_suspend,
> +	.cpu_off     = psci_cpu_off,
> +	.cpu_on      = psci_cpu_on,
> +	.migrate     = psci_migrate,
> +};
> +
> +#ifdef CONFIG_SMP
> +static void __init psci_smp_init_cpus(void)
> +{
> +}
> +
> +static void __init psci_smp_prepare_cpus(unsigned int max_cpus)
> +{
> +}
> +
> +static int __cpuinit psci_boot_secondary(unsigned int cpu,
> +					 struct task_struct *idle)
> +{
> +	return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup));
> +}
> +
> +static void __cpuinit psci_secondary_init(unsigned int cpu)
> +{
> +	gic_secondary_init(0);
> +}
So here we end-up with a dependency between SMP, PSCI, and GIC.
I really can''t see that being a good idea, even I like the general
direction of this series.

	M.
-- 
Jazz is not dead. It just smells funny...

Hi Stefano,

On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini
wrote:
> Check for the presence of PSCI before setting smp_ops, use PSCI if it is
> available.
> 
> This is useful because at least when running on Xen it''s possible
to have a
> PSCI node for example on a Versatile Express or an Exynos5 machine. In
these
> cases the PSCI SMP calls should be the ones to be called.
> 
> Remove virt_smp_ops and platsmp.c from mach-virt because they
aren''t needed
> anymore.
[...]
> +struct psci_operations psci_ops = {
> +	.cpu_suspend = psci_cpu_suspend,
> +	.cpu_off     = psci_cpu_off,
> +	.cpu_on      = psci_cpu_on,
> +	.migrate     = psci_migrate,
> +};
> +
> +#ifdef CONFIG_SMP
> +static void __init psci_smp_init_cpus(void)
> +{
> +}
> +
> +static void __init psci_smp_prepare_cpus(unsigned int max_cpus)
> +{
> +}
> +
> +static int __cpuinit psci_boot_secondary(unsigned int cpu,
> +					 struct task_struct *idle)
> +{
> +	return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup));
> +}
> +
> +static void __cpuinit psci_secondary_init(unsigned int cpu)
> +{
> +	gic_secondary_init(0);
> +}
> +
> +struct smp_operations __initdata psci_smp_ops = {
> +	.smp_init_cpus		= psci_smp_init_cpus,
> +	.smp_prepare_cpus	= psci_smp_prepare_cpus,
> +	.smp_secondary_init	= psci_secondary_init,
> +	.smp_boot_secondary	= psci_boot_secondary,
> +};
> +#endif
As I said before, I don''t agree with bolting these two interfaces
together
like this and, as it stands, I''m afraid I have to NAK this patch.

A potential alternative is to have a set of virt_smp_ops, which have
wrappers around the psci functions, but that requires agreement from Xen and
KVM to implement the same PSCI interface, which feels unfair to me.

I see what you''re trying to do, but I can''t go along with it.
Sorry.

Will

On 03/27/2013 07:50 AM, Stefano Stabellini wrote:
> Check for the presence of PSCI before setting smp_ops, use PSCI if it is
> available.
> 
> This is useful because at least when running on Xen it''s possible
to have a
> PSCI node for example on a Versatile Express or an Exynos5 machine. In
these
> cases the PSCI SMP calls should be the ones to be called.
I have a similar patch in my tree. I thought I had sent it out, but
looks like I didn''t. I didn''t make smp_ops default to this or
change
mach-virt, so we should go with yours.

[...]
> @@ -36,7 +38,11 @@ enum psci_function {
>  	PSCI_FN_MAX,
>  };
>  
> -static u32 psci_function_id[PSCI_FN_MAX];
> +struct psci_function_desc {
> +	enum psci_function func;
> +	bool valid;
Why can''t you use a NULL function ptr or a 0 psci_function_id to
indicate not valid?
> +};
> +static struct psci_function_desc psci_function_id[PSCI_FN_MAX];
>  
>  #define PSCI_RET_SUCCESS		0
>  #define PSCI_RET_EOPNOTSUPP		-1
> @@ -116,7 +122,10 @@ static int psci_cpu_suspend(struct psci_power_state
state,
>  	int err;
>  	u32 fn, power_state;
>  
> -	fn = psci_function_id[PSCI_FN_CPU_SUSPEND];
> +	if (!psci_function_id[PSCI_FN_CPU_SUSPEND].valid)
> +		return -ENOSYS;
> +
> +	fn = psci_function_id[PSCI_FN_CPU_SUSPEND].func;
>  	power_state = psci_power_state_pack(state);
>  	err = invoke_psci_fn(fn, power_state, entry_point, 0);
>  	return psci_to_linux_errno(err);
> @@ -127,7 +136,10 @@ static int psci_cpu_off(struct psci_power_state state)
>  	int err;
>  	u32 fn, power_state;
>  
> -	fn = psci_function_id[PSCI_FN_CPU_OFF];
> +	if (!psci_function_id[PSCI_FN_CPU_OFF].valid)
> +		return -ENOSYS;
> +
> +	fn = psci_function_id[PSCI_FN_CPU_OFF].func;
>  	power_state = psci_power_state_pack(state);
>  	err = invoke_psci_fn(fn, power_state, 0, 0);
>  	return psci_to_linux_errno(err);
> @@ -138,7 +150,10 @@ static int psci_cpu_on(unsigned long cpuid, unsigned
long entry_point)
>  	int err;
>  	u32 fn;
>  
> -	fn = psci_function_id[PSCI_FN_CPU_ON];
> +	if (!psci_function_id[PSCI_FN_CPU_ON].valid)
> +		return -ENOSYS;
> +
> +	fn = psci_function_id[PSCI_FN_CPU_ON].func;
>  	err = invoke_psci_fn(fn, cpuid, entry_point, 0);
>  	return psci_to_linux_errno(err);
>  }
> @@ -148,25 +163,64 @@ static int psci_migrate(unsigned long cpuid)
>  	int err;
>  	u32 fn;
>  
> -	fn = psci_function_id[PSCI_FN_MIGRATE];
> +	if (!psci_function_id[PSCI_FN_MIGRATE].valid)
> +		return -ENOSYS;
> +
> +	fn = psci_function_id[PSCI_FN_MIGRATE].func;
>  	err = invoke_psci_fn(fn, cpuid, 0, 0);
>  	return psci_to_linux_errno(err);
>  }
>  
> +struct psci_operations psci_ops = {
> +	.cpu_suspend = psci_cpu_suspend,
> +	.cpu_off     = psci_cpu_off,
> +	.cpu_on      = psci_cpu_on,
> +	.migrate     = psci_migrate,
> +};
> +
> +#ifdef CONFIG_SMP
> +static void __init psci_smp_init_cpus(void)
> +{
> +}
> +
> +static void __init psci_smp_prepare_cpus(unsigned int max_cpus)
> +{
> +}
You can leave these 2 functions as NULL.
> +
> +static int __cpuinit psci_boot_secondary(unsigned int cpu,
> +					 struct task_struct *idle)
> +{
> +	return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup));
> +}
> +
> +static void __cpuinit psci_secondary_init(unsigned int cpu)
> +{
> +	gic_secondary_init(0);
> +}
> +
> +struct smp_operations __initdata psci_smp_ops = {
> +	.smp_init_cpus		= psci_smp_init_cpus,
> +	.smp_prepare_cpus	= psci_smp_prepare_cpus,
> +	.smp_secondary_init	= psci_secondary_init,
> +	.smp_boot_secondary	= psci_boot_secondary,
You should also include hotplug. Here''s what I had:

#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

On 03/27/2013 08:35 AM, Marc Zyngier wrote:
> On 27/03/13 12:50, Stefano Stabellini wrote:
>> Check for the presence of PSCI before setting smp_ops, use PSCI if it
is
>> available.
>>
>> This is useful because at least when running on Xen it''s
possible to have a
>> PSCI node for example on a Versatile Express or an Exynos5 machine. In
these
>> cases the PSCI SMP calls should be the ones to be called.
>>
>> Remove virt_smp_ops and platsmp.c from mach-virt because they
aren''t needed
>> anymore.
[...]
>> +static void __cpuinit psci_secondary_init(unsigned int cpu)
>> +{
>> +	gic_secondary_init(0);
>> +}
> 
> So here we end-up with a dependency between SMP, PSCI, and GIC.
> I really can''t see that being a good idea, even I like the general
> direction of this series.
> 
The GIC dependency should be removed with Catalin''s series to use
notifiers for GIC cpu interface init.

Rob

On Wed, 27 Mar 2013, Will Deacon wrote:
> Hi Stefano,
> 
> On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini wrote:
> > Check for the presence of PSCI before setting smp_ops, use PSCI if it
is
> > available.
> > 
> > This is useful because at least when running on Xen it''s
possible to have a
> > PSCI node for example on a Versatile Express or an Exynos5 machine. In
these
> > cases the PSCI SMP calls should be the ones to be called.
> > 
> > Remove virt_smp_ops and platsmp.c from mach-virt because they
aren''t needed
> > anymore.
> 
> [...]
> 
> > +struct psci_operations psci_ops = {
> > +	.cpu_suspend = psci_cpu_suspend,
> > +	.cpu_off     = psci_cpu_off,
> > +	.cpu_on      = psci_cpu_on,
> > +	.migrate     = psci_migrate,
> > +};
> > +
> > +#ifdef CONFIG_SMP
> > +static void __init psci_smp_init_cpus(void)
> > +{
> > +}
> > +
> > +static void __init psci_smp_prepare_cpus(unsigned int max_cpus)
> > +{
> > +}
> > +
> > +static int __cpuinit psci_boot_secondary(unsigned int cpu,
> > +					 struct task_struct *idle)
> > +{
> > +	return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup));
> > +}
> > +
> > +static void __cpuinit psci_secondary_init(unsigned int cpu)
> > +{
> > +	gic_secondary_init(0);
> > +}
> > +
> > +struct smp_operations __initdata psci_smp_ops = {
> > +	.smp_init_cpus		= psci_smp_init_cpus,
> > +	.smp_prepare_cpus	= psci_smp_prepare_cpus,
> > +	.smp_secondary_init	= psci_secondary_init,
> > +	.smp_boot_secondary	= psci_boot_secondary,
> > +};
> > +#endif
> 
> As I said before, I don''t agree with bolting these two interfaces
together
> like this and, as it stands, I''m afraid I have to NAK this patch.
> 
> A potential alternative is to have a set of virt_smp_ops, which have
> wrappers around the psci functions, but that requires agreement from Xen
and
> KVM to implement the same PSCI interface, which feels unfair to me.
> 
> I see what you''re trying to do, but I can''t go along with
it. Sorry.
 
OK, let''s see if I can make this acceptable to you.


Would you agree on a patch that moves virt_smp_ops out of mach-virt and
renames them to psci_smp_ops (maybe to arch/arm/kernel/psci_smp_ops.c)?

Would you agree on initializing psci from setup_arch, right after the
call to arm_dt_init_cpu_maps()?

Finally the most controversial point: would you agree on using
psci_smp_ops by default if they are available?
If not, would you at least agree on letting Xen overwrite the default
machine smp_ops?
We need one or the other for dom0 support.

On 03/27/2013 08:38 AM, Will Deacon wrote:
> Hi Stefano,
> 
> On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini wrote:
>> Check for the presence of PSCI before setting smp_ops, use PSCI if it
is
>> available.
>>
>> This is useful because at least when running on Xen it''s
possible to have a
>> PSCI node for example on a Versatile Express or an Exynos5 machine. In
these
>> cases the PSCI SMP calls should be the ones to be called.
>>
>> Remove virt_smp_ops and platsmp.c from mach-virt because they
aren''t needed
>> anymore.
> 
> [...]
> 
>> +struct psci_operations psci_ops = {
>> +	.cpu_suspend = psci_cpu_suspend,
>> +	.cpu_off     = psci_cpu_off,
>> +	.cpu_on      = psci_cpu_on,
>> +	.migrate     = psci_migrate,
>> +};
>> +
>> +#ifdef CONFIG_SMP
>> +static void __init psci_smp_init_cpus(void)
>> +{
>> +}
>> +
>> +static void __init psci_smp_prepare_cpus(unsigned int max_cpus)
>> +{
>> +}
>> +
>> +static int __cpuinit psci_boot_secondary(unsigned int cpu,
>> +					 struct task_struct *idle)
>> +{
>> +	return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup));
>> +}
>> +
>> +static void __cpuinit psci_secondary_init(unsigned int cpu)
>> +{
>> +	gic_secondary_init(0);
>> +}
>> +
>> +struct smp_operations __initdata psci_smp_ops = {
>> +	.smp_init_cpus		= psci_smp_init_cpus,
>> +	.smp_prepare_cpus	= psci_smp_prepare_cpus,
>> +	.smp_secondary_init	= psci_secondary_init,
>> +	.smp_boot_secondary	= psci_boot_secondary,
>> +};
>> +#endif
> 
> As I said before, I don''t agree with bolting these two interfaces
together
> like this and, as it stands, I''m afraid I have to NAK this patch.
> 
> A potential alternative is to have a set of virt_smp_ops, which have
> wrappers around the psci functions, but that requires agreement from Xen
and
> KVM to implement the same PSCI interface, which feels unfair to me.
I need the same smp ops for highbank. By using mach-virt Xen is using
the same interface as KVM. This patch does not change that, but rather
allows other platforms to use the same smp ops as well.

Isn''t the whole point of PSCI to have a common interface? No one is
making Xen use PSCI at all. It is a choice and since they are making
that choice, why would the PSCI interface be different?

Rob

On 03/27/2013 11:23 AM, Stefano Stabellini wrote:
> On Wed, 27 Mar 2013, Will Deacon wrote:
>> Hi Stefano,
>>
>> On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini wrote:
>>> Check for the presence of PSCI before setting smp_ops, use PSCI if
it is
>>> available.
>>>
>>> This is useful because at least when running on Xen it''s
possible to have a
>>> PSCI node for example on a Versatile Express or an Exynos5 machine.
In these
>>> cases the PSCI SMP calls should be the ones to be called.
>>>
>>> Remove virt_smp_ops and platsmp.c from mach-virt because they
aren''t needed
>>> anymore.
>>
>> [...]
>>
>>> +struct psci_operations psci_ops = {
>>> +	.cpu_suspend = psci_cpu_suspend,
>>> +	.cpu_off     = psci_cpu_off,
>>> +	.cpu_on      = psci_cpu_on,
>>> +	.migrate     = psci_migrate,
>>> +};
>>> +
>>> +#ifdef CONFIG_SMP
>>> +static void __init psci_smp_init_cpus(void)
>>> +{
>>> +}
>>> +
>>> +static void __init psci_smp_prepare_cpus(unsigned int max_cpus)
>>> +{
>>> +}
>>> +
>>> +static int __cpuinit psci_boot_secondary(unsigned int cpu,
>>> +					 struct task_struct *idle)
>>> +{
>>> +	return psci_cpu_on(cpu_logical_map(cpu),
__pa(secondary_startup));
>>> +}
>>> +
>>> +static void __cpuinit psci_secondary_init(unsigned int cpu)
>>> +{
>>> +	gic_secondary_init(0);
>>> +}
>>> +
>>> +struct smp_operations __initdata psci_smp_ops = {
>>> +	.smp_init_cpus		= psci_smp_init_cpus,
>>> +	.smp_prepare_cpus	= psci_smp_prepare_cpus,
>>> +	.smp_secondary_init	= psci_secondary_init,
>>> +	.smp_boot_secondary	= psci_boot_secondary,
>>> +};
>>> +#endif
>>
>> As I said before, I don''t agree with bolting these two
interfaces together
>> like this and, as it stands, I''m afraid I have to NAK this
patch.
>>
>> A potential alternative is to have a set of virt_smp_ops, which have
>> wrappers around the psci functions, but that requires agreement from
Xen and
>> KVM to implement the same PSCI interface, which feels unfair to me.
>>
>> I see what you''re trying to do, but I can''t go along
with it. Sorry.
>  
> OK, let''s see if I can make this acceptable to you.
> 
> 
> Would you agree on a patch that moves virt_smp_ops out of mach-virt and
> renames them to psci_smp_ops (maybe to arch/arm/kernel/psci_smp_ops.c)?
> 
> Would you agree on initializing psci from setup_arch, right after the
> call to arm_dt_init_cpu_maps()?
> 
> Finally the most controversial point: would you agree on using
> psci_smp_ops by default if they are available?
> If not, would you at least agree on letting Xen overwrite the default
> machine smp_ops?
> We need one or the other for dom0 support.
It should not be *always* use PSCI smp ops if available, but use them
only if the platform does not define its own smp ops.

Rob

On Wed, Mar 27, 2013 at 04:33:58PM +0000, Rob Herring
wrote:
> On 03/27/2013 08:38 AM, Will Deacon wrote:
> > On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini wrote:
> >> +struct smp_operations __initdata psci_smp_ops = {
> >> +	.smp_init_cpus		= psci_smp_init_cpus,
> >> +	.smp_prepare_cpus	= psci_smp_prepare_cpus,
> >> +	.smp_secondary_init	= psci_secondary_init,
> >> +	.smp_boot_secondary	= psci_boot_secondary,
> >> +};
> >> +#endif
> > 
> > As I said before, I don''t agree with bolting these two
interfaces together
> > like this and, as it stands, I''m afraid I have to NAK this
patch.
> > 
> > A potential alternative is to have a set of virt_smp_ops, which have
> > wrappers around the psci functions, but that requires agreement from
Xen and
> > KVM to implement the same PSCI interface, which feels unfair to me.
> 
> I need the same smp ops for highbank. By using mach-virt Xen is using
> the same interface as KVM. This patch does not change that, but rather
> allows other platforms to use the same smp ops as well.
> 
> Isn''t the whole point of PSCI to have a common interface? No one
is
> making Xen use PSCI at all. It is a choice and since they are making
> that choice, why would the PSCI interface be different?
The channel is common, sure, but I wouldn''t expect the semantics of
each
call to be identical between firmware implementations (going back to my
previous examples of CPU IDs and implementation-defined state parameters).

If a platform happens to have an id-mapping from smp_operations to psci,
then I still think there should be an indirection in there so that we have
the flexibility to change the smp_operations if we wish and not give
platforms the false impression that these two things are equivalent.

Will

On Wed, 27 Mar 2013, Rob Herring wrote:
> On 03/27/2013 11:23 AM, Stefano Stabellini wrote:
> > Would you agree on a patch that moves virt_smp_ops out of mach-virt
and
> > renames them to psci_smp_ops (maybe to
arch/arm/kernel/psci_smp_ops.c)?
> > 
> > Would you agree on initializing psci from setup_arch, right after the
> > call to arm_dt_init_cpu_maps()?
> > 
> > Finally the most controversial point: would you agree on using
> > psci_smp_ops by default if they are available?
> > If not, would you at least agree on letting Xen overwrite the default
> > machine smp_ops?
> > We need one or the other for dom0 support.
> 
> It should not be *always* use PSCI smp ops if available, but use them
> only if the platform does not define its own smp ops.
Well, that is the one additional problem that we have on Xen.

On x86 Xen replaces a lot of core native function calls with its own
implementations (see paravirt_ops).
On ARM we only need *one* set of calls: the smp_ops calls.

So if we don''t want to give priority to PSCI over the platform smp_ops,
then we need a simple workaround just for Xen in common code like the
one appended below.
Not pretty, but at least small:

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 3f6cbb2..08cf7e0 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -43,6 +43,8 @@
 #include <asm/cacheflush.h>
 #include <asm/cachetype.h>
 #include <asm/tlbflush.h>
+#include <xen/xen.h>
+#include <asm/xen/hypervisor.h>
 
 #include <asm/prom.h>
 #include <asm/mach/arch.h>
@@ -766,9 +768,13 @@ void __init setup_arch(char **cmdline_p)
 	unflatten_device_tree();
 
 	arm_dt_init_cpu_maps();
+	xen_early_init();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
-		smp_set_ops(mdesc->smp);
+		if (xen_domain())
+			smp_set_ops(&xen_smp_ops);
+		else
+			smp_set_ops(mdesc->smp);
 		smp_init_cpus();
 	}
 #endif

On Wed, Mar 27, 2013 at 04:23:15PM +0000, Stefano Stabellini
wrote:
> OK, let''s see if I can make this acceptable to you.
> 
> 
> Would you agree on a patch that moves virt_smp_ops out of mach-virt and
> renames them to psci_smp_ops (maybe to arch/arm/kernel/psci_smp_ops.c)?
Moving the code out of psci.c is certainly a good first step, yes.
> Would you agree on initializing psci from setup_arch, right after the
> call to arm_dt_init_cpu_maps()?
Hmmm. An early_initcall runs before SMP is up, so why do you need this
earlier than that? Is it because you don''t want to set the SMP ops
later on?
> Finally the most controversial point: would you agree on using
> psci_smp_ops by default if they are available?
> If not, would you at least agree on letting Xen overwrite the default
> machine smp_ops?
> We need one or the other for dom0 support.
Again, I think there needs to be a dummy layer between the smp_ops and PSCI,
rather than assigning the things directly if we''re going to use this as
a
default implementation. I still question whether default PSCI operations make
any sense though... I understand that you''re currently saying `yes, Xen
can
use the same firmware interface as KVM'' but will that always be true?
What
happens when we want to run virtual machines on multi-cluster platforms, for
example? Will KVM and Xen make sure that CPU affinities are described in the
same way? What if one or the other decides to pass side-band information in
the power_state parameters?

In all of these cases, we''d have to split the code back up, so I
don''t see
any long-term value in consolidating everything just because it might be
possible today. The real problem you''re trying to solve seems to stem
from
patching the smp_ops in your dom0 kernel. Can you elaborate a bit more on
what''s going on here please? How would having PSCI default smp_ops help
you?

Cheers,

Will

On Wed, 27 Mar 2013, Stefano Stabellini wrote:
> On Wed, 27 Mar 2013, Rob Herring wrote:
> > On 03/27/2013 11:23 AM, Stefano Stabellini wrote:
> > > Would you agree on a patch that moves virt_smp_ops out of
mach-virt and
> > > renames them to psci_smp_ops (maybe to
arch/arm/kernel/psci_smp_ops.c)?
> > > 
> > > Would you agree on initializing psci from setup_arch, right after
the
> > > call to arm_dt_init_cpu_maps()?
> > > 
> > > Finally the most controversial point: would you agree on using
> > > psci_smp_ops by default if they are available?
> > > If not, would you at least agree on letting Xen overwrite the
default
> > > machine smp_ops?
> > > We need one or the other for dom0 support.
> > 
> > It should not be *always* use PSCI smp ops if available, but use them
> > only if the platform does not define its own smp ops.
> 
> Well, that is the one additional problem that we have on Xen.
> 
> On x86 Xen replaces a lot of core native function calls with its own
> implementations (see paravirt_ops).
> On ARM we only need *one* set of calls: the smp_ops calls.
> 
> So if we don''t want to give priority to PSCI over the platform
smp_ops,
> then we need a simple workaround just for Xen in common code like the
> one appended below.
> Not pretty, but at least small:
[...]

What about the patch below that I''m carying in my MCPM branch 
which has been posted here already:

From: Jon Medhurst <tixy@linaro.org>
Date: Thu, 13 Dec 2012 13:23:13 +0000
Subject: [PATCH] ARM: Enable selection of SMP operations at boot time

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>

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 308ad7d6f9..c01bf53b85 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 3f6cbb2e3e..41edca8582 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -768,7 +768,10 @@ void __init setup_arch(char **cmdline_p)
 	arm_dt_init_cpu_maps();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
-		smp_set_ops(mdesc->smp);
+		if(mdesc->smp_init)
+			(*mdesc->smp_init)();
+		else
+			smp_set_ops(mdesc->smp);
 		smp_init_cpus();
 	}
 #endif

On 03/27/2013 12:10 PM, Stefano Stabellini wrote:
> On Wed, 27 Mar 2013, Rob Herring wrote:
>> On 03/27/2013 11:23 AM, Stefano Stabellini wrote:
>>> Would you agree on a patch that moves virt_smp_ops out of mach-virt
and
>>> renames them to psci_smp_ops (maybe to
arch/arm/kernel/psci_smp_ops.c)?
>>>
>>> Would you agree on initializing psci from setup_arch, right after
the
>>> call to arm_dt_init_cpu_maps()?
>>>
>>> Finally the most controversial point: would you agree on using
>>> psci_smp_ops by default if they are available?
>>> If not, would you at least agree on letting Xen overwrite the
default
>>> machine smp_ops?
>>> We need one or the other for dom0 support.
>>
>> It should not be *always* use PSCI smp ops if available, but use them
>> only if the platform does not define its own smp ops.
> 
> Well, that is the one additional problem that we have on Xen.
> 
> On x86 Xen replaces a lot of core native function calls with its own
> implementations (see paravirt_ops).
> On ARM we only need *one* set of calls: the smp_ops calls.
> 
> So if we don''t want to give priority to PSCI over the platform
smp_ops,
> then we need a simple workaround just for Xen in common code like the
> one appended below.
> Not pretty, but at least small:
> 
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 3f6cbb2..08cf7e0 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -43,6 +43,8 @@
>  #include <asm/cacheflush.h>
>  #include <asm/cachetype.h>
>  #include <asm/tlbflush.h>
> +#include <xen/xen.h>
> +#include <asm/xen/hypervisor.h>
>  
>  #include <asm/prom.h>
>  #include <asm/mach/arch.h>
> @@ -766,9 +768,13 @@ void __init setup_arch(char **cmdline_p)
>  	unflatten_device_tree();
>  
>  	arm_dt_init_cpu_maps();
> +	xen_early_init();
>  #ifdef CONFIG_SMP
>  	if (is_smp()) {
> -		smp_set_ops(mdesc->smp);
> +		if (xen_domain())
> +			smp_set_ops(&xen_smp_ops);
> +		else
> +			smp_set_ops(mdesc->smp);
No, I was thinking in the case of Xen and mach-virt, you would not set
mdesc->smp. So you would have something like this:

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


Rob

On Wednesday 27 March 2013, Will Deacon wrote:
> The channel is common, sure, but I wouldn''t expect the semantics
of each
> call to be identical between firmware implementations (going back to my
> previous examples of CPU IDs and implementation-defined state parameters).
> 
> If a platform happens to have an id-mapping from smp_operations to psci,
> then I still think there should be an indirection in there so that we have
> the flexibility to change the smp_operations if we wish and not give
> platforms the false impression that these two things are equivalent.
I think the only reasonably implementation for psci is if we can assume
that each callback with a specific property name has a well-defined behavior,
and we should mandate that every platform that implements the callbacks
we need for SMP actually implements them according to the spec.

What would be the point of a standard psci interface if the specific
implementation are not required to follow the same semantics?

	Arnd

On Wednesday 27 March 2013, Rob Herring wrote:
> No, I was thinking in the case of Xen and mach-virt, you would not set
> mdesc->smp. So you would have something like this:
> 
> if (mdesc->smp)
>         smp_set_ops(mdesc->smp);
> else
>         smp_set_ops(&psci_smp_ops);
The case that Stefano is interested in if obviously other platforms
that can either run as Dom0 under Xen with psci_smp_ops or natively
with their own smp_ops. A similar case would be a platform that
may implement psci using smc when run in secure mode but provide
its own smp_ops when run natively.

In both cases, it would be simpler to use psci if available but
fall back to mdesc->smp otherwise.

	Arnd

On Wed, Mar 27, 2013 at 05:50:51PM +0000, Arnd Bergmann
wrote:
> On Wednesday 27 March 2013, Will Deacon wrote:
> > The channel is common, sure, but I wouldn''t expect the
semantics of each
> > call to be identical between firmware implementations (going back to
my
> > previous examples of CPU IDs and implementation-defined state
parameters).
> > 
> > If a platform happens to have an id-mapping from smp_operations to
psci,
> > then I still think there should be an indirection in there so that we
have
> > the flexibility to change the smp_operations if we wish and not give
> > platforms the false impression that these two things are equivalent.
> 
> I think the only reasonably implementation for psci is if we can assume
> that each callback with a specific property name has a well-defined
behavior,
> and we should mandate that every platform that implements the callbacks
> we need for SMP actually implements them according to the spec.
> 
> What would be the point of a standard psci interface if the specific
> implementation are not required to follow the same semantics?
The interface *is* standard. The functions have well-defined headers and can
be called in the same way between implementations. The difference is in the
semantics of the parameters. For example:

  int cpu_off(u32 power_state);

If you look at the power_state parameter, it''s actually a struct (see
struct
psci_power_state) with a u16 id field. The current specification describes
that field as `This is platform specific, the number is understood by the
firmware, and used to program the power controller.''.

So unless we get everybody to agree on the definition of that field, we
can''t blindly plug the interfaces together. Furthermore, there are
other
parameters like this and, as new functions are specified, I would expect
them to grow.

Will

On Wed, 27 Mar 2013, Arnd Bergmann wrote:
> On Wednesday 27 March 2013, Rob Herring wrote:
> > No, I was thinking in the case of Xen and mach-virt, you would not set
> > mdesc->smp. So you would have something like this:
> > 
> > if (mdesc->smp)
> >         smp_set_ops(mdesc->smp);
> > else
> >         smp_set_ops(&psci_smp_ops);
> 
> The case that Stefano is interested in if obviously other platforms
> that can either run as Dom0 under Xen with psci_smp_ops or natively
> with their own smp_ops. A similar case would be a platform that
> may implement psci using smc when run in secure mode but provide
> its own smp_ops when run natively.
That''s correct.

> In both cases, it would be simpler to use psci if available but
> fall back to mdesc->smp otherwise.
I agree.

On Wed, 27 Mar 2013, Nicolas Pitre wrote:
> On Wed, 27 Mar 2013, Stefano Stabellini wrote:
> 
> > On Wed, 27 Mar 2013, Rob Herring wrote:
> > > On 03/27/2013 11:23 AM, Stefano Stabellini wrote:
> > > > Would you agree on a patch that moves virt_smp_ops out of
mach-virt and
> > > > renames them to psci_smp_ops (maybe to
arch/arm/kernel/psci_smp_ops.c)?
> > > > 
> > > > Would you agree on initializing psci from setup_arch, right
after the
> > > > call to arm_dt_init_cpu_maps()?
> > > > 
> > > > Finally the most controversial point: would you agree on
using
> > > > psci_smp_ops by default if they are available?
> > > > If not, would you at least agree on letting Xen overwrite
the default
> > > > machine smp_ops?
> > > > We need one or the other for dom0 support.
> > > 
> > > It should not be *always* use PSCI smp ops if available, but use
them
> > > only if the platform does not define its own smp ops.
> > 
> > Well, that is the one additional problem that we have on Xen.
> > 
> > On x86 Xen replaces a lot of core native function calls with its own
> > implementations (see paravirt_ops).
> > On ARM we only need *one* set of calls: the smp_ops calls.
> > 
> > So if we don''t want to give priority to PSCI over the
platform smp_ops,
> > then we need a simple workaround just for Xen in common code like the
> > one appended below.
> > Not pretty, but at least small:
> [...]
> 
> What about the patch below that I''m carying in my MCPM branch 
> which has been posted here already:
It''s a step in the right direction but it still wouldn''t solve
the
problem.
Let me describe the scenario again:

- we have a versatile express machine with Xen running on it (or an
  exynos5, etc.);
- Xen boots Linux as Dom0, passing a versatile express device tree with
  the Xen hypervisor node added to it;
- Linux is booting, using the versatile express device tree for hardware
  discovery;
- Linux can access the hardware described in the device tree because Xen
  remapped the MMIO regions appropriately;
- Linux needs to bring secondary cpus up, but Xen only exports a PSCI
  interface, so the native versatile express smp_ops don''t work;
- Linux needs to detect that PSCI is available (or Xen is available with
  its set of xen_smp_ops) and preferable on this platform and uses it
  instead of the versatile express smp_ops.


> From: Jon Medhurst <tixy@linaro.org>
> Date: Thu, 13 Dec 2012 13:23:13 +0000
> Subject: [PATCH] ARM: Enable selection of SMP operations at boot time
> 
> 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>
> 
> diff --git a/arch/arm/include/asm/mach/arch.h
b/arch/arm/include/asm/mach/arch.h
> index 308ad7d6f9..c01bf53b85 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 3f6cbb2e3e..41edca8582 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -768,7 +768,10 @@ void __init setup_arch(char **cmdline_p)
>  	arm_dt_init_cpu_maps();
>  #ifdef CONFIG_SMP
>  	if (is_smp()) {
> -		smp_set_ops(mdesc->smp);
> +		if(mdesc->smp_init)
> +			(*mdesc->smp_init)();
> +		else
> +			smp_set_ops(mdesc->smp);
>  		smp_init_cpus();
>  	}
>  #endif
>

On 03/27/2013 01:12 PM, Will Deacon wrote:
> On Wed, Mar 27, 2013 at 05:50:51PM +0000, Arnd Bergmann wrote:
>> On Wednesday 27 March 2013, Will Deacon wrote:
>>> The channel is common, sure, but I wouldn''t expect the
semantics of each
>>> call to be identical between firmware implementations (going back
to my
>>> previous examples of CPU IDs and implementation-defined state
parameters).
>>>
>>> If a platform happens to have an id-mapping from smp_operations to
psci,
>>> then I still think there should be an indirection in there so that
we have
>>> the flexibility to change the smp_operations if we wish and not
give
>>> platforms the false impression that these two things are
equivalent.
>>
>> I think the only reasonably implementation for psci is if we can assume
>> that each callback with a specific property name has a well-defined
behavior,
>> and we should mandate that every platform that implements the callbacks
>> we need for SMP actually implements them according to the spec.
>>
>> What would be the point of a standard psci interface if the specific
>> implementation are not required to follow the same semantics?
> 
> The interface *is* standard. The functions have well-defined headers and
can
> be called in the same way between implementations. The difference is in the
> semantics of the parameters. For example:
> 
>   int cpu_off(u32 power_state);
> 
> If you look at the power_state parameter, it''s actually a struct
(see struct
> psci_power_state) with a u16 id field. The current specification describes
> that field as `This is platform specific, the number is understood by the
> firmware, and used to program the power controller.''.
> 
> So unless we get everybody to agree on the definition of that field, we
> can''t blindly plug the interfaces together. Furthermore, there are
other
> parameters like this and, as new functions are specified, I would expect
> them to grow.
Which is why I''ve said I think the ID field is a bad idea.
I''ve used it
in my implementation, but only in the case of system level reset,
power-off, and suspend. I don''t see how it would be used otherwise.

I guess you could define a value of 0 must be supported at a minimum and
then default implementation would at least work to some level.

Rob

On Wednesday 27 March 2013, Will Deacon wrote:
> The interface is standard. The functions have well-defined headers and can
> be called in the same way between implementations. The difference is in the
> semantics of the parameters. For example:
> 
>   int cpu_off(u32 power_state);
I think that is the opposite of well-defined :(
> If you look at the power_state parameter, it''s actually a struct
(see struct
> psci_power_state) with a u16 id field. The current specification describes
> that field as `This is platform specific, the number is understood by the
> firmware, and used to program the power controller.''.
> 
> So unless we get everybody to agree on the definition of that field, we
> can''t blindly plug the interfaces together. Furthermore, there are
other
> parameters like this and, as new functions are specified, I would expect
> them to grow.
I think it''s expected that there will be vendor specific extensions,
but any interface that is part of the standard has to be completely
specified there, anything else is pure madness.

Perhaps we could extend the device tree binding to add the missing bits,
and pass the values that you are supposed to pass there as properties
of the node, but it would be much more logical to require the interface
to be well-behaved in the first place.

	Arnd

On Wed, 27 Mar 2013, Will Deacon wrote:
> On Wed, Mar 27, 2013 at 04:23:15PM +0000, Stefano Stabellini wrote:
> > OK, let''s see if I can make this acceptable to you.
> > 
> > 
> > Would you agree on a patch that moves virt_smp_ops out of mach-virt
and
> > renames them to psci_smp_ops (maybe to
arch/arm/kernel/psci_smp_ops.c)?
> 
> Moving the code out of psci.c is certainly a good first step, yes.
OK, I''ll do that.

> > Would you agree on initializing psci from setup_arch, right after the
> > call to arm_dt_init_cpu_maps()?
> 
> Hmmm. An early_initcall runs before SMP is up, so why do you need this
> earlier than that? Is it because you don''t want to set the SMP ops
later on?
Because I need to set the right smp_ops before any of the smp_ops
functions are called.
If I wait until early_initcall, the wrong smp_init_cpus is going to be
called.

> > Finally the most controversial point: would you agree on using
> > psci_smp_ops by default if they are available?
> > If not, would you at least agree on letting Xen overwrite the default
> > machine smp_ops?
> > We need one or the other for dom0 support.
> 
> Again, I think there needs to be a dummy layer between the smp_ops and
PSCI,
> rather than assigning the things directly if we''re going to use
this as a
> default implementation. I still question whether default PSCI operations
make
> any sense though... I understand that you''re currently saying
`yes, Xen can
> use the same firmware interface as KVM'' but will that always be
true? What
> happens when we want to run virtual machines on multi-cluster platforms,
for
> example? Will KVM and Xen make sure that CPU affinities are described in
the
> same way? What if one or the other decides to pass side-band information in
> the power_state parameters?
> 
> In all of these cases, we''d have to split the code back up, so I
don''t see
> any long-term value in consolidating everything just because it might be
> possible today. The real problem you''re trying to solve seems to
stem from
> patching the smp_ops in your dom0 kernel.
That is correct.
Although having a clean generic solution would be preferable to me, if
we cannot reach an agreement or if just cannot be done, an hack that
patch smp_ops would also solve the Xen problem (see
http://marc.info/?l=linux-arm-kernel&m=136440425214113&w=2).
I distaste hacks as anybody else, so I would rather solve the issue in a
different way.

> Can you elaborate a bit more on
> what''s going on here please? How would having PSCI default smp_ops
help you?
As Arnd quickly explained
(http://marc.info/?l=linux-arm-kernel&m=136440746215589&w=2), the
hardware environment provided by Xen to Dom0 looks very similar to the
native hardware but with a few important differences:

- the amount of memory is probably lower, in fact Xen is going to edit
the device tree and only tell Dom0 about the memory that Dom0 can
actually see;

- the number of cpus might be different, in fact Xen is going to edit
the device tree and tell Dom0 about the vcpus that Dom0 can actually
see;

- not all the devices present might actually be exported to Dom0, in
fact Xen is going to remove these devices from the device tree so that
Dom0 won''t try to access them;

- the interface to bring up secondary cpus is different and based on
PSCI, in fact Xen is going to add a PSCI node to the device tree so that
Dom0 can use it.

Oh wait, Dom0 is not going to use the PSCI interface even if the node is
present on device tree because it''s going to prefer the platform
smp_ops
instead.

Fail.


Xen can however add any nodes to device tree besides PSCI, it already
adds a Xen hypervisor node, unfortunately the smp_ops are not detected
via device tree at the moment so that won''t help.

So I guess what we really need is a way to export a set of firmware
operations for cpu bring up via device tree. I thought that PSCI was
exactly meant for this, but if it isn''t, then we need to come up with
something else.

On Thu, 28 Mar 2013, Stefano Stabellini wrote:
> - the interface to bring up secondary cpus is different and based on
> PSCI, in fact Xen is going to add a PSCI node to the device tree so that
> Dom0 can use it.
> 
> Oh wait, Dom0 is not going to use the PSCI interface even if the node is
> present on device tree because it''s going to prefer the platform
smp_ops
> instead.
Waitaminute...  I must have missed this part.

Who said platform specific methods must be used in preference to PSCI?

If DT does provide PSCI description, then PSCI should be used.  Doing 
otherwise is senseless.  If PSCI is not to be used, then it should not 
be present in DT.


Nicolas

On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
> On Thu, 28 Mar 2013, Stefano Stabellini wrote:
> 
>> - the interface to bring up secondary cpus is different and based on
>> PSCI, in fact Xen is going to add a PSCI node to the device tree so
that
>> Dom0 can use it.
>>
>> Oh wait, Dom0 is not going to use the PSCI interface even if the node
is
>> present on device tree because it''s going to prefer the
platform smp_ops
>> instead.
> 
> Waitaminute...  I must have missed this part.
> 
> Who said platform specific methods must be used in preference to PSCI?
I did. Specifically, I said the platform should be allowed to provide
its own smp_ops. A platform may need to do addtional things on top of
PSCI for example.
> If DT does provide PSCI description, then PSCI should be used.  Doing 
> otherwise is senseless.  If PSCI is not to be used, then it should not 
> be present in DT.
You can''t assume the DT and kernel are in-sync. For example,
I''ve added
PSCI in the firmware and DTB (part of the firmware), but the highbank
kernel may or may not use it depending if I convert it.

Rob
> 
> 
> Nicolas
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

On Thu, 28 Mar 2013, Rob Herring wrote:
> On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
> > On Thu, 28 Mar 2013, Stefano Stabellini wrote:
> > 
> >> - the interface to bring up secondary cpus is different and based
on
> >> PSCI, in fact Xen is going to add a PSCI node to the device tree
so that
> >> Dom0 can use it.
> >>
> >> Oh wait, Dom0 is not going to use the PSCI interface even if the
node is
> >> present on device tree because it''s going to prefer the
platform smp_ops
> >> instead.
> > 
> > Waitaminute...  I must have missed this part.
> > 
> > Who said platform specific methods must be used in preference to PSCI?
> 
> I did. Specifically, I said the platform should be allowed to provide
> its own smp_ops. A platform may need to do addtional things on top of
> PSCI for example.
> 
> > If DT does provide PSCI description, then PSCI should be used.  Doing 
> > otherwise is senseless.  If PSCI is not to be used, then it should not
> > be present in DT.
> 
> You can''t assume the DT and kernel are in-sync. For example,
I''ve added
> PSCI in the firmware and DTB (part of the firmware), but the highbank
> kernel may or may not use it depending if I convert it.
You are saying that we might want to run an old kernel, without PSCI
support on a machine that exports a DT with PSCI?
But in that case the platform smp_ops will just be chosen as usual.

And if you have a new kernel with PSCI support on a machine that exports
a DT with PSCI won''t be OK to use the PSCI calls?

The only problematic case would be if you actually need some platform
specific calls to wrap around the PSCI firmare interface. But in that
case what is the point of exporting the PSCI node on device tree anyway?

On Thu, 28 Mar 2013, Rob Herring wrote:
> On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
> > On Thu, 28 Mar 2013, Stefano Stabellini wrote:
> > 
> >> - the interface to bring up secondary cpus is different and based
on
> >> PSCI, in fact Xen is going to add a PSCI node to the device tree
so that
> >> Dom0 can use it.
> >>
> >> Oh wait, Dom0 is not going to use the PSCI interface even if the
node is
> >> present on device tree because it''s going to prefer the
platform smp_ops
> >> instead.
> > 
> > Waitaminute...  I must have missed this part.
> > 
> > Who said platform specific methods must be used in preference to PSCI?
> 
> I did. Specifically, I said the platform should be allowed to provide
> its own smp_ops. A platform may need to do addtional things on top of
> PSCI for example.
Then the platform should have its special hook that would override the 
default PSCI methods.  But, by *default* the PSCI methods should be used 
if the related DT information is present.
> > If DT does provide PSCI description, then PSCI should be used.  Doing 
> > otherwise is senseless.  If PSCI is not to be used, then it should not
> > be present in DT.
> 
> You can''t assume the DT and kernel are in-sync. For example,
I''ve added
> PSCI in the firmware and DTB (part of the firmware), but the highbank
> kernel may or may not use it depending if I convert it.
If the kernel does not understand PSCI bindings in the DT, it naturally 
won''t use PSCI, right?  Conversely, if the firmware and therefore 
provided DT don''t have PSCI, then the PSCI enabled kernel
won''t use PSCI
either. So what is the problem?


Nicolas

On Thu, Mar 28, 2013 at 03:39:42PM +0000, Nicolas Pitre
wrote:
> On Thu, 28 Mar 2013, Rob Herring wrote:
> 
> > On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
> > > On Thu, 28 Mar 2013, Stefano Stabellini wrote:
> > > 
> > >> - the interface to bring up secondary cpus is different and
based on
> > >> PSCI, in fact Xen is going to add a PSCI node to the device
tree so that
> > >> Dom0 can use it.
> > >>
> > >> Oh wait, Dom0 is not going to use the PSCI interface even if
the node is
> > >> present on device tree because it''s going to prefer
the platform smp_ops
> > >> instead.
> > > 
> > > Waitaminute...  I must have missed this part.
> > > 
> > > Who said platform specific methods must be used in preference to
PSCI?
> > 
> > I did. Specifically, I said the platform should be allowed to provide
> > its own smp_ops. A platform may need to do addtional things on top of
> > PSCI for example.
> 
> Then the platform should have its special hook that would override the 
> default PSCI methods.  But, by *default* the PSCI methods should be used 
> if the related DT information is present.
I''m fine with a default PSCI-based implementation, providing that
it''s actually
a layer between the smp ops and the psci code, not just glueing pointers
together.

KVM and Xen can then use the default implementation, but it does mean that
they have to agree on that interface as it expands in the future. If Xen
relies on the default ops in order to boot, then that''s a good
incentive not
to deviate from them on the firmware side.

Will

On Thu, 28 Mar 2013, Will Deacon wrote:
> On Thu, Mar 28, 2013 at 03:39:42PM +0000, Nicolas Pitre wrote:
> > On Thu, 28 Mar 2013, Rob Herring wrote:
> > 
> > > On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
> > > > On Thu, 28 Mar 2013, Stefano Stabellini wrote:
> > > > 
> > > >> - the interface to bring up secondary cpus is different
and based on
> > > >> PSCI, in fact Xen is going to add a PSCI node to the
device tree so that
> > > >> Dom0 can use it.
> > > >>
> > > >> Oh wait, Dom0 is not going to use the PSCI interface
even if the node is
> > > >> present on device tree because it''s going to
prefer the platform smp_ops
> > > >> instead.
> > > > 
> > > > Waitaminute...  I must have missed this part.
> > > > 
> > > > Who said platform specific methods must be used in
preference to PSCI?
> > > 
> > > I did. Specifically, I said the platform should be allowed to
provide
> > > its own smp_ops. A platform may need to do addtional things on
top of
> > > PSCI for example.
> > 
> > Then the platform should have its special hook that would override the
> > default PSCI methods.  But, by *default* the PSCI methods should be
used
> > if the related DT information is present.
> 
> I''m fine with a default PSCI-based implementation, providing that
it''s actually
> a layer between the smp ops and the psci code, not just glueing pointers
> together.
Absolutely.

Even in the MCPM case, the PSCI is wrapped into a MCPM backend while 
MCPM provides its own SMP ops methods.


Nicolas

On Thu, 28 Mar 2013, Will Deacon wrote:
> On Thu, Mar 28, 2013 at 03:39:42PM +0000, Nicolas Pitre wrote:
> > On Thu, 28 Mar 2013, Rob Herring wrote:
> > 
> > > On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
> > > > On Thu, 28 Mar 2013, Stefano Stabellini wrote:
> > > > 
> > > >> - the interface to bring up secondary cpus is different
and based on
> > > >> PSCI, in fact Xen is going to add a PSCI node to the
device tree so that
> > > >> Dom0 can use it.
> > > >>
> > > >> Oh wait, Dom0 is not going to use the PSCI interface
even if the node is
> > > >> present on device tree because it''s going to
prefer the platform smp_ops
> > > >> instead.
> > > > 
> > > > Waitaminute...  I must have missed this part.
> > > > 
> > > > Who said platform specific methods must be used in
preference to PSCI?
> > > 
> > > I did. Specifically, I said the platform should be allowed to
provide
> > > its own smp_ops. A platform may need to do addtional things on
top of
> > > PSCI for example.
> > 
> > Then the platform should have its special hook that would override the
> > default PSCI methods.  But, by *default* the PSCI methods should be
used
> > if the related DT information is present.
> 
> I''m fine with a default PSCI-based implementation, providing that
it''s actually
> a layer between the smp ops and the psci code, not just glueing pointers
> together.
OK. I''ll rename virt_smp_ops and move it to its own file rather than
psci.c and we''ll take it from there.

> KVM and Xen can then use the default implementation, but it does mean that
> they have to agree on that interface as it expands in the future. If Xen
> relies on the default ops in order to boot, then that''s a good
incentive not
> to deviate from them on the firmware side.
I am OK with that.

On 03/28/2013 10:39 AM, Nicolas Pitre wrote:
> On Thu, 28 Mar 2013, Rob Herring wrote:
> 
>> On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
>>> On Thu, 28 Mar 2013, Stefano Stabellini wrote:
>>>
>>>> - the interface to bring up secondary cpus is different and
based on
>>>> PSCI, in fact Xen is going to add a PSCI node to the device
tree so that
>>>> Dom0 can use it.
>>>>
>>>> Oh wait, Dom0 is not going to use the PSCI interface even if
the node is
>>>> present on device tree because it''s going to prefer
the platform smp_ops
>>>> instead.
>>>
>>> Waitaminute...  I must have missed this part.
>>>
>>> Who said platform specific methods must be used in preference to
PSCI?
>>
>> I did. Specifically, I said the platform should be allowed to provide
>> its own smp_ops. A platform may need to do addtional things on top of
>> PSCI for example.
> 
> Then the platform should have its special hook that would override the 
> default PSCI methods.  But, by *default* the PSCI methods should be used 
> if the related DT information is present.
Agreed. The special hook to override is setting mach desc smp_ops, right?
>>> If DT does provide PSCI description, then PSCI should be used. 
Doing
>>> otherwise is senseless.  If PSCI is not to be used, then it should
not
>>> be present in DT.
>>
>> You can''t assume the DT and kernel are in-sync. For example,
I''ve added
>> PSCI in the firmware and DTB (part of the firmware), but the highbank
>> kernel may or may not use it depending if I convert it.
> 
> If the kernel does not understand PSCI bindings in the DT, it naturally 
> won''t use PSCI, right?  Conversely, if the firmware and therefore 
> provided DT don''t have PSCI, then the PSCI enabled kernel
won''t use PSCI
> either. So what is the problem?
I''m distinguishing the kernel in general is enabled for PSCI and a
platform is enabled. The kernel may have PSCI smp_ops and the DTB may
have PSCI data, but that alone should not make a platform use the
default PSCI smp_ops. The platform has to make the decision and it
cannot be just based on the platform''s dtb having PSCI data.

I have firmware (dtb is part of the firmware) with PSCI support and
older firmware without. Old/existing kernels are fine on both firmware
versions and don''t use PSCI. New kernels with default PSCI ops should
continue to work with both versions. When/If I convert highbank to use
PSCI in the kernel, only then will new kernels require the new firmware
version. Or perhaps I need to support both in the kernel for a while
before ripping out non PSCI code. There is enough lag in distro kernels
that I don''t think this is necessary.

Rob

On Thu, 28 Mar 2013, Rob Herring wrote:
> On 03/28/2013 10:39 AM, Nicolas Pitre wrote:
> > On Thu, 28 Mar 2013, Rob Herring wrote:
> > 
> >> On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
> >>> On Thu, 28 Mar 2013, Stefano Stabellini wrote:
> >>>
> >>>> - the interface to bring up secondary cpus is different
and based on
> >>>> PSCI, in fact Xen is going to add a PSCI node to the
device tree so that
> >>>> Dom0 can use it.
> >>>>
> >>>> Oh wait, Dom0 is not going to use the PSCI interface even
if the node is
> >>>> present on device tree because it''s going to
prefer the platform smp_ops
> >>>> instead.
> >>>
> >>> Waitaminute...  I must have missed this part.
> >>>
> >>> Who said platform specific methods must be used in preference
to PSCI?
> >>
> >> I did. Specifically, I said the platform should be allowed to
provide
> >> its own smp_ops. A platform may need to do addtional things on top
of
> >> PSCI for example.
> > 
> > Then the platform should have its special hook that would override the
> > default PSCI methods.  But, by *default* the PSCI methods should be
used
> > if the related DT information is present.
> 
> Agreed. The special hook to override is setting mach desc smp_ops, right?
If you consider the mach smp_ops a platform specific override, then
again PSCI and providing a PSCI node on DT doesn''t solve the Xen
problem
at all.

See above: Xen adds a PSCI node to DT, and Linux still does not use it.

> >>> If DT does provide PSCI description, then PSCI should be used.
Doing
> >>> otherwise is senseless.  If PSCI is not to be used, then it
should not
> >>> be present in DT.
> >>
> >> You can''t assume the DT and kernel are in-sync. For
example, I''ve added
> >> PSCI in the firmware and DTB (part of the firmware), but the
highbank
> >> kernel may or may not use it depending if I convert it.
> > 
> > If the kernel does not understand PSCI bindings in the DT, it
naturally
> > won''t use PSCI, right?  Conversely, if the firmware and
therefore
> > provided DT don''t have PSCI, then the PSCI enabled kernel
won''t use PSCI
> > either. So what is the problem?
> 
> I''m distinguishing the kernel in general is enabled for PSCI and a
> platform is enabled. The kernel may have PSCI smp_ops and the DTB may
> have PSCI data, but that alone should not make a platform use the
> default PSCI smp_ops. The platform has to make the decision and it
> cannot be just based on the platform''s dtb having PSCI data.
I can see how this would give greater flexibility to firmware
developers, but on the other hand it would limit the flexibility of the
kernel.

In fact, unfortunately, it is diametrically the opposite of what Xen
needs.

I would kindly ask the maintainers to let me know what direction I
should take to move forward.

On 03/29/2013 08:22 AM, Stefano Stabellini wrote:
> On Thu, 28 Mar 2013, Rob Herring wrote:
>> On 03/28/2013 10:39 AM, Nicolas Pitre wrote:
>>> On Thu, 28 Mar 2013, Rob Herring wrote:
>>>
>>>> On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
>>>>> On Thu, 28 Mar 2013, Stefano Stabellini wrote:
>>>>>
>>>>>> - the interface to bring up secondary cpus is different
and based on
>>>>>> PSCI, in fact Xen is going to add a PSCI node to the
device tree so that
>>>>>> Dom0 can use it.
>>>>>>
>>>>>> Oh wait, Dom0 is not going to use the PSCI interface
even if the node is
>>>>>> present on device tree because it''s going to
prefer the platform smp_ops
>>>>>> instead.
>>>>>
>>>>> Waitaminute...  I must have missed this part.
>>>>>
>>>>> Who said platform specific methods must be used in
preference to PSCI?
>>>>
>>>> I did. Specifically, I said the platform should be allowed to
provide
>>>> its own smp_ops. A platform may need to do addtional things on
top of
>>>> PSCI for example.
>>>
>>> Then the platform should have its special hook that would override
the
>>> default PSCI methods.  But, by *default* the PSCI methods should be
used
>>> if the related DT information is present.
>>
>> Agreed. The special hook to override is setting mach desc smp_ops,
right?
> 
> If you consider the mach smp_ops a platform specific override, then
> again PSCI and providing a PSCI node on DT doesn''t solve the Xen
problem
> at all.
> 
> See above: Xen adds a PSCI node to DT, and Linux still does not use it.
Okay, I see. I wasn''t distinguishing Dom0 vs DomU cases. Is this really
the only issue with having a platform run in Dom0? We expect all
platforms to work without any modifications? I would think for more
complex platforms there would be some other work needed.

How is Xen going to really do physical cpu power management if a
platform does not provide PSCI firmware? Are you going to pull all the
platform specific code we have in the kernel now into Xen? If you make
PSCI firmware a requirement for Xen, then you would only be modifying
existing PSCI data to the DTB and the platform would be converted to use
PSCI already.
>>>>> If DT does provide PSCI description, then PSCI should be
used.  Doing
>>>>> otherwise is senseless.  If PSCI is not to be used, then it
should not
>>>>> be present in DT.
>>>>
>>>> You can''t assume the DT and kernel are in-sync. For
example, I''ve added
>>>> PSCI in the firmware and DTB (part of the firmware), but the
highbank
>>>> kernel may or may not use it depending if I convert it.
>>>
>>> If the kernel does not understand PSCI bindings in the DT, it
naturally
>>> won''t use PSCI, right?  Conversely, if the firmware and
therefore
>>> provided DT don''t have PSCI, then the PSCI enabled kernel
won''t use PSCI
>>> either. So what is the problem?
>>
>> I''m distinguishing the kernel in general is enabled for PSCI
and a
>> platform is enabled. The kernel may have PSCI smp_ops and the DTB may
>> have PSCI data, but that alone should not make a platform use the
>> default PSCI smp_ops. The platform has to make the decision and it
>> cannot be just based on the platform''s dtb having PSCI data.
> 
> I can see how this would give greater flexibility to firmware
> developers, but on the other hand it would limit the flexibility of the
> kernel.
It limits the flexibility of the kernel too. If PSCI is present in the
DTB, then the kernel must use it and the platform has no say? That''s
not
flexible.
> 
> In fact, unfortunately, it is diametrically the opposite of what Xen
> needs.
> 
> I would kindly ask the maintainers to let me know what direction I
> should take to move forward.
My argument is somewhat academic. I fully expect to convert highbank
over to PSCI for 3.10 assuming this patch gets sorted out in time. So it
is not really an issue for me. Adding Nico''s smp_init function could
give the platform flexibility later if needed.

We''re only talking about the behavior of a small portion of the patch,
so I would go ahead with implementing the rest of the feedback.

Rob

On Fri, 29 Mar 2013, Rob Herring wrote:
> On 03/29/2013 08:22 AM, Stefano Stabellini wrote:
> > On Thu, 28 Mar 2013, Rob Herring wrote:
> >> On 03/28/2013 10:39 AM, Nicolas Pitre wrote:
> >>> On Thu, 28 Mar 2013, Rob Herring wrote:
> >>>
> >>>> On 03/28/2013 09:51 AM, Nicolas Pitre wrote:
> >>>>> On Thu, 28 Mar 2013, Stefano Stabellini wrote:
> >>>>>
> >>>>>> - the interface to bring up secondary cpus is
different and based on
> >>>>>> PSCI, in fact Xen is going to add a PSCI node to
the device tree so that
> >>>>>> Dom0 can use it.
> >>>>>>
> >>>>>> Oh wait, Dom0 is not going to use the PSCI
interface even if the node is
> >>>>>> present on device tree because it''s going
to prefer the platform smp_ops
> >>>>>> instead.
> >>>>>
> >>>>> Waitaminute...  I must have missed this part.
> >>>>>
> >>>>> Who said platform specific methods must be used in
preference to PSCI?
> >>>>
> >>>> I did. Specifically, I said the platform should be allowed
to provide
> >>>> its own smp_ops. A platform may need to do addtional
things on top of
> >>>> PSCI for example.
> >>>
> >>> Then the platform should have its special hook that would
override the
> >>> default PSCI methods.  But, by *default* the PSCI methods
should be used
> >>> if the related DT information is present.
> >>
> >> Agreed. The special hook to override is setting mach desc smp_ops,
right?
> > 
> > If you consider the mach smp_ops a platform specific override, then
> > again PSCI and providing a PSCI node on DT doesn''t solve the
Xen problem
> > at all.
> > 
> > See above: Xen adds a PSCI node to DT, and Linux still does not use
it.
> 
> Okay, I see. I wasn''t distinguishing Dom0 vs DomU cases. Is this
really
> the only issue with having a platform run in Dom0? We expect all
> platforms to work without any modifications? I would think for more
> complex platforms there would be some other work needed.
No, I think that''s all we need. At least I fail to see the need for
something else at the moment :-)

> How is Xen going to really do physical cpu power management if a
> platform does not provide PSCI firmware? Are you going to pull all the
> platform specific code we have in the kernel now into Xen? If you make
> PSCI firmware a requirement for Xen, then you would only be modifying
> existing PSCI data to the DTB and the platform would be converted to use
> PSCI already.
That is a good question. Realistically there are only few platforms that
support ARMv7 virtualization extensions today, so we could have those
platform specific functions in Xen. I am hopeful that in the future the
new platforms that will support ARMv7 virtualization extensions and the
coming ARMv8 platforms will also support PSCI.
Otherwise yes, Xen will have to know about platform specific power
management.

> >>>>> If DT does provide PSCI description, then PSCI should
be used.  Doing
> >>>>> otherwise is senseless.  If PSCI is not to be used,
then it should not
> >>>>> be present in DT.
> >>>>
> >>>> You can''t assume the DT and kernel are in-sync.
For example, I''ve added
> >>>> PSCI in the firmware and DTB (part of the firmware), but
the highbank
> >>>> kernel may or may not use it depending if I convert it.
> >>>
> >>> If the kernel does not understand PSCI bindings in the DT, it
naturally
> >>> won''t use PSCI, right?  Conversely, if the firmware
and therefore
> >>> provided DT don''t have PSCI, then the PSCI enabled
kernel won''t use PSCI
> >>> either. So what is the problem?
> >>
> >> I''m distinguishing the kernel in general is enabled for
PSCI and a
> >> platform is enabled. The kernel may have PSCI smp_ops and the DTB
may
> >> have PSCI data, but that alone should not make a platform use the
> >> default PSCI smp_ops. The platform has to make the decision and it
> >> cannot be just based on the platform''s dtb having PSCI
data.
> > 
> > I can see how this would give greater flexibility to firmware
> > developers, but on the other hand it would limit the flexibility of
the
> > kernel.
> 
> It limits the flexibility of the kernel too. If PSCI is present in the
> DTB, then the kernel must use it and the platform has no say?
That''s not
> flexible.
I am OK with having an override, but the override can''t be hardcoded in
the kernel sources. We could have a kernel command line parameter to
disable PSCI for example. Something like no_psci.

> > In fact, unfortunately, it is diametrically the opposite of what Xen
> > needs.
> > 
> > I would kindly ask the maintainers to let me know what direction I
> > should take to move forward.
> 
> My argument is somewhat academic. I fully expect to convert highbank
> over to PSCI for 3.10 assuming this patch gets sorted out in time. So it
> is not really an issue for me. Adding Nico''s smp_init function
could
> give the platform flexibility later if needed.
> 
> We''re only talking about the behavior of a small portion of the
patch,
> so I would go ahead with implementing the rest of the feedback.
OK, I''ll do that.
But I have to highlight that without those two lines in setup_arch
we''ll
have broken SMP in Dom0.