[PATCH v2 0/6] xen/arm: move to mach-virt and support SMP

[PATCH v2 0/6] xen/arm: move to mach-virt and support SMP

[Stefano Stabellini]

Hi all,
this patch series, based on 3.9-rc3, moves xenvm to mach-virt and
implements SMP support in Xen on ARM.


Changes from v1:
- move the percpu variable argument fix in xen_init_events to a separate
  patch;
- remove unused variable in xen_init_events;
- add an RFC patch to initialize PSCI from smp_set_ops and use it if
  available.


Stefano Stabellini (6):
      xen/arm: actually pass a non-NULL percpu pointer to request_percpu_irq
      xen/arm: SMP support
      xen: move the xenvm machine to mach-virt
      xen/arm: implement HYPERVISOR_vcpu_op
      xenvm: add a simple PSCI node and a second cpu
      [RFC] arm: use PSCI if available

 arch/arm/boot/dts/xenvm-4.2.dts      |   12 +++++++
 arch/arm/include/asm/psci.h          |    1 +
 arch/arm/include/asm/xen/hypercall.h |    1 +
 arch/arm/kernel/psci.c               |   42 ++++++++++++++++++++++--
 arch/arm/kernel/smp.c                |    7 +++-
 arch/arm/mach-vexpress/v2m.c         |    1 -
 arch/arm/mach-virt/Makefile          |    1 -
 arch/arm/mach-virt/platsmp.c         |   58 ----------------------------------
 arch/arm/mach-virt/virt.c            |    4 +--
 arch/arm/xen/enlighten.c             |   41 +++++++++++++++++++++++-
 arch/arm/xen/hypercall.S             |    1 +
 11 files changed, 99 insertions(+), 70 deletions(-)

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

Cheers,

Stefano

[PATCH v2 1/6] xen/arm: actually pass a non-NULL percpu pointer to request_percpu_irq

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/xen/enlighten.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 8dc0605..99ce189 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -239,7 +239,7 @@ static int __init xen_init_events(void)
 	xen_init_IRQ();
 
 	if (request_percpu_irq(xen_events_irq, xen_arm_callback,
-			"events", xen_vcpu)) {
+			"events", &xen_vcpu)) {
 		pr_err("Error requesting IRQ %d\n", xen_events_irq);
 		return -EINVAL;
 	}
-- 
1.7.2.5

[PATCH v2 2/6] xen/arm: SMP support

[Stefano Stabellini]

Map vcpu_info using VCPUOP_register_vcpu_info on secondary cpus.

Call enable_percpu_irq on every cpu.

Changed in v2:
- move the percpu variable argument fix to a separate patch;
- remove unused variable.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/xen/enlighten.c |   38 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 99ce189..94bbf3b 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -2,6 +2,7 @@
 #include <xen/events.h>
 #include <xen/grant_table.h>
 #include <xen/hvm.h>
+#include <xen/interface/vcpu.h>
 #include <xen/interface/xen.h>
 #include <xen/interface/memory.h>
 #include <xen/interface/hvm/params.h>
@@ -32,6 +33,7 @@ struct shared_info xen_dummy_shared_info;
 struct shared_info *HYPERVISOR_shared_info = (void *)&xen_dummy_shared_info;
 
 DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
+DEFINE_PER_CPU(struct vcpu_info, xen_vcpu_info);
 
 /* These are unused until we support booting "pre-ballooned" */
 unsigned long xen_released_pages;
@@ -148,6 +150,32 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
 }
 EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
 
+static int __init xen_secondary_init(unsigned int cpu)
+{
+	struct vcpu_register_vcpu_info info;
+	struct vcpu_info *vcpup;
+	int err;
+
+	if (cpu == 0)
+		return 0;
+
+	pr_info("Xen: initializing cpu%d\n", cpu);
+	vcpup = &per_cpu(xen_vcpu_info, cpu);
+
+	info.mfn = __pa(vcpup) >> PAGE_SHIFT;
+	info.offset = offset_in_page(vcpup);
+
+	err = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_info, cpu, &info);
+	if (err) {
+		pr_debug("register_vcpu_info failed: err=%d\n", err);
+	} else {
+		/* This cpu is using the registered vcpu info, even if
+		   later ones fail to. */
+		per_cpu(xen_vcpu, cpu) = vcpup;
+	}
+	return 0;
+}
+
 /*
  * see Documentation/devicetree/bindings/arm/xen.txt for the
  * documentation of the Xen Device Tree format.
@@ -163,6 +191,7 @@ static int __init xen_guest_init(void)
 	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) {
@@ -216,6 +245,8 @@ static int __init xen_guest_init(void)
 	 * is required to use VCPUOP_register_vcpu_info to place vcpu info
 	 * for secondary CPUs as they are brought up. */
 	per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0];
+	for_each_online_cpu(i)
+		xen_secondary_init(i);
 
 	gnttab_init();
 	if (!xen_initial_domain())
@@ -231,6 +262,11 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static __init void xen_percpu_enable_events(void *unused)
+{
+	enable_percpu_irq(xen_events_irq, 0);
+}
+
 static int __init xen_init_events(void)
 {
 	if (!xen_domain() || xen_events_irq < 0)
@@ -244,7 +280,7 @@ static int __init xen_init_events(void)
 		return -EINVAL;
 	}
 
-	enable_percpu_irq(xen_events_irq, 0);
+	on_each_cpu(xen_percpu_enable_events, NULL, 0);
 
 	return 0;
 }
-- 
1.7.2.5

[PATCH v2 3/6] xen: move the xenvm machine to mach-virt

[Stefano Stabellini]

xenvm is based on mach-vexpress, move it to mach-virt.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
CC: will.deacon@arm.com
CC: arnd@arndb.de
CC: rob.herring@calxeda.com
---
 arch/arm/mach-vexpress/v2m.c |    1 -
 arch/arm/mach-virt/virt.c    |    1 +
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index 915683c..c43ec78 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -469,7 +469,6 @@ static void __init v2m_dt_init(void)
 
 static const char * const v2m_dt_match[] __initconst = {
 	"arm,vexpress",
-	"xen,xenvm",
 	NULL,
 };
 
diff --git a/arch/arm/mach-virt/virt.c b/arch/arm/mach-virt/virt.c
index 31666f6..528c05e 100644
--- a/arch/arm/mach-virt/virt.c
+++ b/arch/arm/mach-virt/virt.c
@@ -40,6 +40,7 @@ static void __init virt_timer_init(void)
 
 static const char *virt_dt_match[] = {
 	"linux,dummy-virt",
+	"xen,xenvm",
 	NULL
 };
 
-- 
1.7.2.5

[PATCH v2 4/6] xen/arm: implement HYPERVISOR_vcpu_op

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/include/asm/xen/hypercall.h |    1 +
 arch/arm/xen/enlighten.c             |    1 +
 arch/arm/xen/hypercall.S             |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index 8a82325..799f42e 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -46,6 +46,7 @@ int HYPERVISOR_event_channel_op(int cmd, void *arg);
 unsigned long HYPERVISOR_hvm_op(int op, void *arg);
 int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
 int HYPERVISOR_physdev_op(int cmd, void *arg);
+int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
 
 static inline void
 MULTI_update_va_mapping(struct multicall_entry *mcl, unsigned long va,
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 94bbf3b..b002822 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -295,4 +295,5 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_sched_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_hvm_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_memory_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op);
+EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op);
 EXPORT_SYMBOL_GPL(privcmd_call);
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index 71f7239..199cb2d 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -87,6 +87,7 @@ HYPERCALL2(event_channel_op);
 HYPERCALL2(hvm_op);
 HYPERCALL2(memory_op);
 HYPERCALL2(physdev_op);
+HYPERCALL3(vcpu_op);
 
 ENTRY(privcmd_call)
 	stmdb sp!, {r4}
-- 
1.7.2.5

[PATCH v2 5/6] xenvm: add a simple PSCI node and a second cpu

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: rob.herring@calxeda.com
CC: will.deacon@arm.com
CC: marc.zyngier@arm.com
CC: arnd@arndb.de
---
 arch/arm/boot/dts/xenvm-4.2.dts |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/xenvm-4.2.dts b/arch/arm/boot/dts/xenvm-4.2.dts
index 7a2cf30..66efd22 100644
--- a/arch/arm/boot/dts/xenvm-4.2.dts
+++ b/arch/arm/boot/dts/xenvm-4.2.dts
@@ -29,6 +29,18 @@
 			compatible = "arm,cortex-a15";
 			reg = <0>;
 		};
+
+		cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <1>;
+		};
+	};
+
+	psci {
+		compatible      = "arm,psci";
+		method          = "hvc";
+		cpu_on          = <2>;
 	};
 
 	memory@80000000 {
-- 
1.7.2.5

Re: [PATCH v2 5/6] xenvm: add a simple PSCI node and a second cpu

[Arnd Bergmann]

On Tuesday 26 March 2013, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: rob.herring@calxeda.com
> CC: will.deacon@arm.com
> CC: marc.zyngier@arm.com
> CC: arnd@arndb.de

I wonder how this is supposed to work on real systems. Shouldn't the dt
blob be filled out with the correct number of CPUs at the time you start
the guest? This change looks like you just make all guests use a hardcoded
set of two CPUs instead of just one, but you probably want to allow any
number between 1 and the number of physically present cores.

	Arnd

Re: [PATCH v2 5/6] xenvm: add a simple PSCI node and a second cpu

[Stefano Stabellini]

On Tue, 26 Mar 2013, Arnd Bergmann wrote:
> On Tuesday 26 March 2013, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > CC: rob.herring@calxeda.com
> > CC: will.deacon@arm.com
> > CC: marc.zyngier@arm.com
> > CC: arnd@arndb.de
> 
> I wonder how this is supposed to work on real systems. Shouldn't the dt
> blob be filled out with the correct number of CPUs at the time you start
> the guest? 
> This change looks like you just make all guests use a hardcoded
> set of two CPUs instead of just one, but you probably want to allow any
> number between 1 and the number of physically present cores.

That's right, the DT passed on to the guests is generated by the
hypervisor. This is just an example, adding a cpu and a psci node is
just meant to make it clear that Xen supports multi-vcpu guests using
PSCI to bootstrap secondary cpus.

[PATCH v2 6/6] [RFC] arm: use PSCI if available

[Stefano Stabellini]

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.


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  |    1 +
 arch/arm/kernel/psci.c       |   42 +++++++++++++++++++++++++++---
 arch/arm/kernel/smp.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, 45 insertions(+), 67 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..e64ea84 100644
--- a/arch/arm/include/asm/psci.h
+++ b/arch/arm/include/asm/psci.h
@@ -32,5 +32,6 @@ struct psci_operations {
 };
 
 extern struct psci_operations psci_ops;
+int psci_init(struct smp_operations *ops);
 
 #endif /* __ASM_ARM_PSCI_H */
diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index 3653164..6f66d93 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,7 +24,9 @@
 #include <asm/opcodes-sec.h>
 #include <asm/opcodes-virt.h>
 #include <asm/psci.h>
+#include <asm/smp_plat.h>
 
+extern void secondary_startup(void);
 struct psci_operations psci_ops;
 
 static int (*invoke_psci_fn)(u32, u32, u32, u32);
@@ -153,20 +156,50 @@ static int psci_migrate(unsigned long cpuid)
 	return psci_to_linux_errno(err);
 }
 
+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)
+{
+	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);
+}
+
+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,
+};
+
 static const struct of_device_id psci_of_match[] __initconst = {
 	{ .compatible = "arm,psci",	},
 	{},
 };
 
-static int __init psci_init(void)
+int __init psci_init(struct smp_operations *ops)
 {
 	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");
 
@@ -203,9 +236,10 @@ static int __init psci_init(void)
 		psci_function_id[PSCI_FN_MIGRATE] = id;
 		psci_ops.migrate = psci_migrate;
 	}
+	*ops = psci_smp_ops;
+	rc = 0;
 
 out_put_node:
 	of_node_put(np);
-	return 0;
+	return rc;
 }
-early_initcall(psci_init);
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 31644f1..09eda3c 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -38,6 +38,7 @@
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
 #include <asm/processor.h>
+#include <asm/psci.h>
 #include <asm/sections.h>
 #include <asm/tlbflush.h>
 #include <asm/ptrace.h>
@@ -74,7 +75,11 @@ static struct smp_operations smp_ops;
 
 void __init smp_set_ops(struct smp_operations *ops)
 {
-	if (ops)
+	int rc = -ENODEV;
+#ifdef CONFIG_ARM_PSCI
+	rc = psci_init(&smp_ops);
+#endif
+	if (rc && ops)
 		smp_ops = *ops;
 };
 
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

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Arnd Bergmann]

On Tuesday 26 March 2013, 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.

Very nice, I had a similar idea but had not gotten around to write a patch.
This fits in nicely with my plans to make all fields of machine_desc optional.

>  void __init smp_set_ops(struct smp_operations *ops)
>  {
> -       if (ops)
> +       int rc = -ENODEV;
> +#ifdef CONFIG_ARM_PSCI
> +       rc = psci_init(&smp_ops);
> +#endif
> +       if (rc && ops)
>                smp_ops = *ops;
>  };

Could you move this into the caller, i.e. setup_arch() so we call smp_set_ops
either for psci_smp_ops or for machine_desc->smp?

	Arnd

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Stefano Stabellini]

On Tue, 26 Mar 2013, Arnd Bergmann wrote:
> On Tuesday 26 March 2013, 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.
> 
> Very nice, I had a similar idea but had not gotten around to write a patch.
> This fits in nicely with my plans to make all fields of machine_desc optional.
> 
> >  void __init smp_set_ops(struct smp_operations *ops)
> >  {
> > -       if (ops)
> > +       int rc = -ENODEV;
> > +#ifdef CONFIG_ARM_PSCI
> > +       rc = psci_init(&smp_ops);
> > +#endif
> > +       if (rc && ops)
> >                smp_ops = *ops;
> >  };
> 
> Could you move this into the caller, i.e. setup_arch() so we call smp_set_ops
> either for psci_smp_ops or for machine_desc->smp?

Sure, I can do that.

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Will Deacon]

Hi Stefano,

On Tue, Mar 26, 2013 at 02:41:15PM +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 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,
> +};

Whilst I like the idea of this, I don't think things will pan out this
nicely in practice. There will almost always be a level of indirection
required between the internal Linux SMP operations and the expectations of
the PSCI firmware, whether this is in CPU numbering or other,
platform-specific fields in various parameters.

Tying these two things together like this confuses the layering in my
opinion and will likely lead to potentially subtle breakages if platforms
start trying to adopt this.

If this can indeed work for the virtual platforms (Xen and KVM), then I
think it would be better expressed using `virt' smp_ops, which map directly
to PSCI, rather than putting them here. Even then, it's tying KVM and Xen
together on the firmware side of things...

Will

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Stefano Stabellini]

On Tue, 26 Mar 2013, Will Deacon wrote:
> Hi Stefano,
> 
> On Tue, Mar 26, 2013 at 02:41:15PM +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 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,
> > +};
> 
> Whilst I like the idea of this, I don't think things will pan out this
> nicely in practice. There will almost always be a level of indirection
> required between the internal Linux SMP operations and the expectations of
> the PSCI firmware, whether this is in CPU numbering or other,
> platform-specific fields in various parameters.
> 
> Tying these two things together like this confuses the layering in my
> opinion and will likely lead to potentially subtle breakages if platforms
> start trying to adopt this.

What you are saying is that psci could either be used directly, like we
are doing, or it could just be the base of some higher level platform
specific smp_ops.

Honestly I think that psci is already high level enough that I would
worry if somebody started to wrap it around something else.

However we still support that use case just fine:  they can just avoid
having a psci node on device tree and just keep using their machine
specific smp_ops. It's up to them really.
They can even base the implementation of their smp_ops on the current
psci code, in order to facilitate that I could get rid of psci_ops
(which initialization is based on device tree) and export the psci_cpu_*
functions instead, so that they can be called directly by other smp_ops.


> If this can indeed work for the virtual platforms (Xen and KVM), then I
> think it would be better expressed using `virt' smp_ops, which map directly
> to PSCI, rather than putting them here. Even then, it's tying KVM and Xen
> together on the firmware side of things...

Keep in mind that dom0 on Xen boots as a native machine (versatile
express or exynos5 for example) with a Xen hypervisor node on it.  We
would need to find a way to override the default machine smp_ops with
a set of xen_smp_ops early at boot.
I don't like this option very much, I think is fragile.

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Will Deacon]

On Tue, Mar 26, 2013 at 03:25:55PM +0000, Stefano Stabellini wrote:
> On Tue, 26 Mar 2013, Will Deacon wrote:
> > On Tue, Mar 26, 2013 at 02:41:15PM +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,
> > > +};
> > 
> > Whilst I like the idea of this, I don't think things will pan out this
> > nicely in practice. There will almost always be a level of indirection
> > required between the internal Linux SMP operations and the expectations of
> > the PSCI firmware, whether this is in CPU numbering or other,
> > platform-specific fields in various parameters.
> > 
> > Tying these two things together like this confuses the layering in my
> > opinion and will likely lead to potentially subtle breakages if platforms
> > start trying to adopt this.
> 
> What you are saying is that psci could either be used directly, like we
> are doing, or it could just be the base of some higher level platform
> specific smp_ops.
> 
> Honestly I think that psci is already high level enough that I would
> worry if somebody started to wrap it around something else.

I don't agree. PSCI is a low-level firmware interface, which will naturally
have implementation-specific parts to it. For example, many of the CPU power
functions have platform-specific state ID parameters which we can't just
ignore. Furthermore, the method by which a CPU is identified needn't match
the value in our logical map. The purpose of the PSCI code in Linux is to
provide a basic abstraction on top of this interface, so that platforms can
incorporate them into higher-level power management functions, which
themselves might be plumbed into smp_operations structures.

> However we still support that use case just fine:  they can just avoid
> having a psci node on device tree and just keep using their machine
> specific smp_ops. It's up to them really.

Why get rid of the node? That's there to initialise the PSCI backend
accordingly and shouldn't have anything to do with SMP.

> They can even base the implementation of their smp_ops on the current
> psci code, in order to facilitate that I could get rid of psci_ops
> (which initialization is based on device tree) and export the psci_cpu_*
> functions instead, so that they can be called directly by other smp_ops.

Again, I think this destroys the layering. The whole point is that the PSCI
functions are called from within something that understands precisely how to
talk to the firmware and what it is capable of.

> > If this can indeed work for the virtual platforms (Xen and KVM), then I
> > think it would be better expressed using `virt' smp_ops, which map directly
> > to PSCI, rather than putting them here. Even then, it's tying KVM and Xen
> > together on the firmware side of things...
> 
> Keep in mind that dom0 on Xen boots as a native machine (versatile
> express or exynos5 for example) with a Xen hypervisor node on it.  We
> would need to find a way to override the default machine smp_ops with
> a set of xen_smp_ops early at boot.
> I don't like this option very much, I think is fragile.

Why can't dom0 use whatever smp ops the native machine would use?

Will

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Arnd Bergmann]

On Tuesday 26 March 2013, Will Deacon wrote:
> > They can even base the implementation of their smp_ops on the current
> > psci code, in order to facilitate that I could get rid of psci_ops
> > (which initialization is based on device tree) and export the psci_cpu_*
> > functions instead, so that they can be called directly by other smp_ops.
> 
> Again, I think this destroys the layering. The whole point is that the PSCI
> functions are called from within something that understands precisely how to
> talk to the firmware and what it is capable of.

Right, we probably the psci smp ops to be separate from the rest of the psci
code, but I also think that Stefano is right that we should let any platform
use the psci smp ops if possible, rather than having to implement their own.

> > > If this can indeed work for the virtual platforms (Xen and KVM), then I
> > > think it would be better expressed using `virt' smp_ops, which map directly
> > > to PSCI, rather than putting them here. Even then, it's tying KVM and Xen
> > > together on the firmware side of things...
> > 
> > Keep in mind that dom0 on Xen boots as a native machine (versatile
> > express or exynos5 for example) with a Xen hypervisor node on it.  We
> > would need to find a way to override the default machine smp_ops with
> > a set of xen_smp_ops early at boot.
> > I don't like this option very much, I think is fragile.
> 
> Why can't dom0 use whatever smp ops the native machine would use?

The part that I'm most interested in is making it possible for a platform
to kill off its native smp ops in the kernel by implementing the psci
ops. I think it's a good strategy to use psci by default if both 
platform and psci implementations are available.

	Arnd

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Stefano Stabellini]

On Tue, 26 Mar 2013, Arnd Bergmann wrote:
> On Tuesday 26 March 2013, Will Deacon wrote:
> > > They can even base the implementation of their smp_ops on the current
> > > psci code, in order to facilitate that I could get rid of psci_ops
> > > (which initialization is based on device tree) and export the psci_cpu_*
> > > functions instead, so that they can be called directly by other smp_ops.
> > 
> > Again, I think this destroys the layering. The whole point is that the PSCI
> > functions are called from within something that understands precisely how to
> > talk to the firmware and what it is capable of.
> 
> Right, we probably the psci smp ops to be separate from the rest of the psci
> code, but I also think that Stefano is right that we should let any platform
> use the psci smp ops if possible, rather than having to implement their own.

[...]
 
> The part that I'm most interested in is making it possible for a platform
> to kill off its native smp ops in the kernel by implementing the psci
> ops. I think it's a good strategy to use psci by default if both 
> platform and psci implementations are available.

I fully agree, and Xen would use it as is.

If the psci node on DT only signifies the presence of psci, not that it
should be used for smp_ops, then we need another DT node or property to
say "this machine uses smp_psci_ops".

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Nicolas Pitre]

On Tue, 26 Mar 2013, Arnd Bergmann wrote:

> On Tuesday 26 March 2013, Will Deacon wrote:
> > > They can even base the implementation of their smp_ops on the current
> > > psci code, in order to facilitate that I could get rid of psci_ops
> > > (which initialization is based on device tree) and export the psci_cpu_*
> > > functions instead, so that they can be called directly by other smp_ops.
> > 
> > Again, I think this destroys the layering. The whole point is that the PSCI
> > functions are called from within something that understands precisely how to
> > talk to the firmware and what it is capable of.
> 
> Right, we probably the psci smp ops to be separate from the rest of the psci
> code, but I also think that Stefano is right that we should let any platform
> use the psci smp ops if possible, rather than having to implement their own.

Oh absolutely.  It is always best to use an existing standard.  But PSCI 
probably won't be the only firmware interface standard.  It therefore 
shouldn't be used as the Linux internal interface model.


Nicolas

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Stefano Stabellini]

On Tue, 26 Mar 2013, Nicolas Pitre wrote:
> On Tue, 26 Mar 2013, Arnd Bergmann wrote:
> 
> > On Tuesday 26 March 2013, Will Deacon wrote:
> > > > They can even base the implementation of their smp_ops on the current
> > > > psci code, in order to facilitate that I could get rid of psci_ops
> > > > (which initialization is based on device tree) and export the psci_cpu_*
> > > > functions instead, so that they can be called directly by other smp_ops.
> > > 
> > > Again, I think this destroys the layering. The whole point is that the PSCI
> > > functions are called from within something that understands precisely how to
> > > talk to the firmware and what it is capable of.
> > 
> > Right, we probably the psci smp ops to be separate from the rest of the psci
> > code, but I also think that Stefano is right that we should let any platform
> > use the psci smp ops if possible, rather than having to implement their own.
> 
> Oh absolutely.  It is always best to use an existing standard.  But PSCI 
> probably won't be the only firmware interface standard.  It therefore 
> shouldn't be used as the Linux internal interface model.

I am not proposing to use PSCI as an interal Linux API.

I am proposing to use a set of PSCI based smp_ops (instead of the ones
that come with machine_desc, if any) if a PSCI node is available on
device tree.

smp_ops remains the internal Linux API.

I am also saying that we should let people reuse the PSCI functions in
their own machine-specific smp_ops, if they want to.

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Stefano Stabellini]

On Tue, 26 Mar 2013, Will Deacon wrote:
> On Tue, Mar 26, 2013 at 03:25:55PM +0000, Stefano Stabellini wrote:
> > On Tue, 26 Mar 2013, Will Deacon wrote:
> > > On Tue, Mar 26, 2013 at 02:41:15PM +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,
> > > > +};
> > > 
> > > Whilst I like the idea of this, I don't think things will pan out this
> > > nicely in practice. There will almost always be a level of indirection
> > > required between the internal Linux SMP operations and the expectations of
> > > the PSCI firmware, whether this is in CPU numbering or other,
> > > platform-specific fields in various parameters.
> > > 
> > > Tying these two things together like this confuses the layering in my
> > > opinion and will likely lead to potentially subtle breakages if platforms
> > > start trying to adopt this.
> > 
> > What you are saying is that psci could either be used directly, like we
> > are doing, or it could just be the base of some higher level platform
> > specific smp_ops.
> > 
> > Honestly I think that psci is already high level enough that I would
> > worry if somebody started to wrap it around something else.
> 
> I don't agree. PSCI is a low-level firmware interface, which will naturally
> have implementation-specific parts to it. For example, many of the CPU power
> functions have platform-specific state ID parameters which we can't just
> ignore. Furthermore, the method by which a CPU is identified needn't match
> the value in our logical map. The purpose of the PSCI code in Linux is to
> provide a basic abstraction on top of this interface, so that platforms can
> incorporate them into higher-level power management functions, which
> themselves might be plumbed into smp_operations structures.
> 
> > However we still support that use case just fine:  they can just avoid
> > having a psci node on device tree and just keep using their machine
> > specific smp_ops. It's up to them really.
> 
> Why get rid of the node? That's there to initialise the PSCI backend
> accordingly and shouldn't have anything to do with SMP.
> 
> > They can even base the implementation of their smp_ops on the current
> > psci code, in order to facilitate that I could get rid of psci_ops
> > (which initialization is based on device tree) and export the psci_cpu_*
> > functions instead, so that they can be called directly by other smp_ops.
> 
> Again, I think this destroys the layering. The whole point is that the PSCI
> functions are called from within something that understands precisely how to
> talk to the firmware and what it is capable of.

All right, I am not going to pretend to know better the final purpose of
PSCI :-)

Assuming that you are correct, I can't see any other options than
have a Xen specific override setup.c, but it's really ugly:

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 3f6cbb2..7876865 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -768,7 +768,12 @@ void __init setup_arch(char **cmdline_p)
 	arm_dt_init_cpu_maps();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
-		smp_set_ops(mdesc->smp);
+		int rc = -ENODEV;
+#ifdef CONFIG_XEN
+		rc = xen_init_smp();
+#endif
+		if (rc)
+			smp_set_ops(mdesc->smp);
 		smp_init_cpus();
 	}
 #endif


> > > If this can indeed work for the virtual platforms (Xen and KVM), then I
> > > think it would be better expressed using `virt' smp_ops, which map directly
> > > to PSCI, rather than putting them here. Even then, it's tying KVM and Xen
> > > together on the firmware side of things...
> > 
> > Keep in mind that dom0 on Xen boots as a native machine (versatile
> > express or exynos5 for example) with a Xen hypervisor node on it.  We
> > would need to find a way to override the default machine smp_ops with
> > a set of xen_smp_ops early at boot.
> > I don't like this option very much, I think is fragile.
> 
> Why can't dom0 use whatever smp ops the native machine would use?

Because Xen doesn't export them (and it's not going to).

Re: [PATCH v2 6/6] [RFC] arm: use PSCI if available

[Nicolas Pitre]

On Tue, 26 Mar 2013, Will Deacon wrote:

> On Tue, Mar 26, 2013 at 03:25:55PM +0000, Stefano Stabellini wrote:
> > On Tue, 26 Mar 2013, Will Deacon wrote:
> > > On Tue, Mar 26, 2013 at 02:41:15PM +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,
> > > > +};
> > > 
> > > Whilst I like the idea of this, I don't think things will pan out this
> > > nicely in practice. There will almost always be a level of indirection
> > > required between the internal Linux SMP operations and the expectations of
> > > the PSCI firmware, whether this is in CPU numbering or other,
> > > platform-specific fields in various parameters.
> > > 
> > > Tying these two things together like this confuses the layering in my
> > > opinion and will likely lead to potentially subtle breakages if platforms
> > > start trying to adopt this.
> > 
> > What you are saying is that psci could either be used directly, like we
> > are doing, or it could just be the base of some higher level platform
> > specific smp_ops.
> > 
> > Honestly I think that psci is already high level enough that I would
> > worry if somebody started to wrap it around something else.
> 
> I don't agree. PSCI is a low-level firmware interface, which will naturally
> have implementation-specific parts to it. For example, many of the CPU power
> functions have platform-specific state ID parameters which we can't just
> ignore. Furthermore, the method by which a CPU is identified needn't match
> the value in our logical map. The purpose of the PSCI code in Linux is to
> provide a basic abstraction on top of this interface, so that platforms can
> incorporate them into higher-level power management functions, which
> themselves might be plumbed into smp_operations structures.

Absolutely.  PSCI is _not_ a Linux API.  It is a firmware API.  And 
remember that Linux has no stable API by design. So it is best to keep 
PSCI as one possible way to talk to the firmware, but a flexible shim 
layer (flexible as in "we can change its interface whenever we want to") 
around PSCI to provide a Linux API which also encompass all possible 
low-level implementations alternatives is a better idea.


Nicolas