Xen devel - Dec 2013 - [PATCH v2 0/6]

This is a major rework of last week''s ARM PSCI host support. The code
has been moved into a separate file and the code flow has been
changed substantially (see below for more details).
  ---------

Xen did not make use of the host provided ARM PSCI (Power State
Coordination Interface [1]) functionality so far, but relied on
platform specific SMP bringup functions.
This series adds support for PSCI on the host by reading the required
information from the DTB and invoking the appropriate handler when
bringing up each single CPU.
Since PSCI is defined for both ARM32 and ARM64, I added code for both
architectures, though only ARM32 is tested on Midway and VExpress
(without any PSCI support on the latter, just a regression test).
ARM64 code was compile tested only.

The ARM32 SMP boot flow is now as following:
The DTB is scanned for a node announcing PSCI support. If that is
available, call the PSCI handler via SMC to bringup the secondary
cores. If no PSCI has been found, call the platform specific cpu_up()
function. It is now the responsibility of those functions to do the
GIC SGI call to kick the secondary CPUs. For that a function is now
provided which does this, so three platforms now reference this
generic function instead of coding up an empty one and relying on the
GIC kick in Xen code later.

The ARM64 SMP boot flow is different: PSCI is not only described in a
root DTB node, but also advertised in each core''s node as the
preferred SMP bringup method. So the PSCI call wrapper function is
registered as the cpu_up function when the DTB is scanned.

This patch series is split up as follows:
1/6) rename psci.c to vpsci.c to make room for the new, host-related
     code
2/6) move the GIC SGI kick into a separate function and call it now
     directly from the platform code, removing the empty cpu_up()
3/6) parse the DTB for a PSCI node and store the needed function index
4/6) add a function to actually invoke the firmware''s PSCI handler
5/6) enable PSCI on ARM32 by adding the call to the PSCI handler
6/6) enable PSCI on ARM64 by registering the PSCI handler function


Changes from v1:
- much rework, PSCI host DTB scanning unchanged (v1: 1/4, v2: 3/6)
- moved host PSCI code to psci.c, renaming the old one to vpsci.c
- have a separate psci_available variable
- removing explicit "host" from function names
- returning standard error codes on DTB scanning
- do the ARM64 PSCI call from a registered function pointer
- move the GIC kick into a separate function
- more change to the code flow in general


Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

[1]:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0022b/index.html

Andre Przywara (6):
  arm: rename xen/arch/arm/psci.c into vpsci.c
  arm: move GIC SGI kicking into separate function
  arm: parse PSCI node from the host device-tree
  arm: add a function to invoke the PSCI handler
  arm32: enable PSCI secondary CPU bringup
  arm64: enable PSCI secondary CPU bringup

 xen/arch/arm/Makefile             |   1 +
 xen/arch/arm/arm32/smpboot.c      |   4 +-
 xen/arch/arm/arm64/smpboot.c      |  23 ++++++-
 xen/arch/arm/platform.c           |   6 +-
 xen/arch/arm/platforms/exynos5.c  |  11 +---
 xen/arch/arm/platforms/omap5.c    |  11 +---
 xen/arch/arm/platforms/vexpress.c |  10 +--
 xen/arch/arm/psci.c               | 127 +++++++++++++++++++-------------------
 xen/arch/arm/smpboot.c            |  22 +++++--
 xen/arch/arm/vpsci.c              |  93 ++++++++++++++++++++++++++++
 xen/include/asm-arm/psci.h        |   7 +++
 xen/include/asm-arm/smp.h         |   2 +
 12 files changed, 215 insertions(+), 102 deletions(-)
 create mode 100644 xen/arch/arm/vpsci.c

-- 
1.7.12.1

Follow the current convention of prefixing guest related names
with "v" by renaming the guest PSCI functionality into vpsci.c to make
room for the host PSCI functions.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/Makefile            | 2 +-
 xen/arch/arm/{psci.c => vpsci.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename xen/arch/arm/{psci.c => vpsci.c} (100%)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 003ac84..11cf663 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -5,7 +5,7 @@ subdir-y += platforms
 obj-$(EARLY_PRINTK) += early_printk.o
 obj-y += cpu.o
 obj-y += domain.o
-obj-y += psci.o
+obj-y += vpsci.o
 obj-y += domctl.o
 obj-y += sysctl.o
 obj-y += domain_build.o
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/vpsci.c
similarity index 100%
rename from xen/arch/arm/psci.c
rename to xen/arch/arm/vpsci.c
-- 
1.7.12.1

Currently we unconditionally send SGIs to all cores on SMP bringup.
PSCI will not need this, so we move this into a function and call it
explicitly from the platforms that need it. This gets us get rid of
the empty cpu_up() platform functions in ARM32 and the comment in
there.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/arm64/smpboot.c      |  2 +-
 xen/arch/arm/platform.c           |  2 +-
 xen/arch/arm/platforms/exynos5.c  | 11 +----------
 xen/arch/arm/platforms/omap5.c    | 11 +----------
 xen/arch/arm/platforms/vexpress.c | 10 +---------
 xen/arch/arm/smpboot.c            | 15 ++++++++++-----
 xen/include/asm-arm/smp.h         |  2 ++
 7 files changed, 17 insertions(+), 36 deletions(-)

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 8696ed6..6a34bd4 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -38,7 +38,7 @@ static int __init smp_spin_table_cpu_up(int cpu)
 
     sev();
 
-    return 0;
+    return cpu_up_send_sgi(cpu);
 }
 
 static void __init smp_spin_table_init(int cpu, struct dt_device_node *dn)
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index a7f9ee4..056d462 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -112,7 +112,7 @@ int __init platform_cpu_up(int cpu)
     if ( platform && platform->cpu_up )
         return platform->cpu_up(cpu);
 
-    return -EAGAIN;
+    return -ENODEV;
 }
 
 int __init platform_smp_init(void)
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 0e76cac..7880815 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -85,15 +85,6 @@ static int __init exynos5_smp_init(void)
     return 0;
 }
 
-static int __init exynos5_cpu_up(int cpu)
-{
-    /* Nothing to do here, the generic sev() will suffice to kick CPUs
-     * out of either the firmware or our own smp_up_cpu gate,
-     * depending on where they have ended up. */
-
-    return 0;
-}
-
 static void exynos5_reset(void)
 {
     void __iomem *pmu;
@@ -137,7 +128,7 @@ PLATFORM_START(exynos5, "SAMSUNG EXYNOS5")
     .init_time = exynos5_init_time,
     .specific_mapping = exynos5_specific_mapping,
     .smp_init = exynos5_smp_init,
-    .cpu_up = exynos5_cpu_up,
+    .cpu_up = cpu_up_send_sgi,
     .reset = exynos5_reset,
     .quirks = exynos5_quirks,
     .blacklist_dev = exynos5_blacklist_dev,
diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index 54fa5ff..4be8e8e 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -144,15 +144,6 @@ static int __init omap5_smp_init(void)
     return 0;
 }
 
-static int __init omap5_cpu_up(int cpu)
-{
-    /* Nothing to do here, the generic sev() will suffice to kick CPUs
-     * out of either the firmware or our own smp_up_cpu gate,
-     * depending on where they have ended up. */
-
-    return 0;
-}
-
 static uint32_t omap5_quirks(void)
 {
     return PLATFORM_QUIRK_DOM0_MAPPING_11;
@@ -169,7 +160,7 @@ PLATFORM_START(omap5, "TI OMAP5")
     .init_time = omap5_init_time,
     .specific_mapping = omap5_specific_mapping,
     .smp_init = omap5_smp_init,
-    .cpu_up = omap5_cpu_up,
+    .cpu_up = cpu_up_send_sgi,
     .quirks = omap5_quirks,
 PLATFORM_END
 
diff --git a/xen/arch/arm/platforms/vexpress.c
b/xen/arch/arm/platforms/vexpress.c
index 9056366..6132056 100644
--- a/xen/arch/arm/platforms/vexpress.c
+++ b/xen/arch/arm/platforms/vexpress.c
@@ -144,14 +144,6 @@ static int __init vexpress_smp_init(void)
     return 0;
 }
 
-static int __init vexpress_cpu_up(int cpu)
-{
-    /* Nothing to do here, the generic sev() will suffice to kick CPUs
-     * out of either the firmware or our own smp_up_cpu gate,
-     * depending on where they have ended up. */
-
-    return 0;
-}
 #endif
 
 static const char * const vexpress_dt_compat[] __initconst @@ -180,7 +172,7 @@
PLATFORM_START(vexpress, "VERSATILE EXPRESS")
     .compatible = vexpress_dt_compat,
 #ifdef CONFIG_ARM_32
     .smp_init = vexpress_smp_init,
-    .cpu_up = vexpress_cpu_up,
+    .cpu_up = cpu_up_send_sgi,
 #endif
     .reset = vexpress_reset,
     .blacklist_dev = vexpress_blacklist_dev,
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 6b4a18c..52cef30 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -343,6 +343,16 @@ void stop_cpu(void)
         wfi();
 }
 
+int __init cpu_up_send_sgi(int cpu)
+{
+    /* We don''t know the GIC ID of the CPU until it has woken up, so
just
+     * signal everyone and rely on our own smp_up_cpu gate to ensure only
+     * the one we want gets through. */
+    send_SGI_allbutself(GIC_SGI_EVENT_CHECK);
+
+    return 0;
+}
+
 /* Bring up a remote CPU */
 int __cpu_up(unsigned int cpu)
 {
@@ -376,11 +386,6 @@ int __cpu_up(unsigned int cpu)
         return rc;
     }
 
-    /* We don''t know the GIC ID of the CPU until it has woken up, so
just signal
-     * everyone and rely on our own smp_up_cpu gate to ensure only the one we
-     * want gets through. */
-    send_SGI_allbutself(GIC_SGI_EVENT_CHECK);
-
     while ( !cpu_online(cpu) )
     {
         cpu_relax();
diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
index 1485cc6..a1de03c 100644
--- a/xen/include/asm-arm/smp.h
+++ b/xen/include/asm-arm/smp.h
@@ -21,6 +21,8 @@ extern int arch_smp_init(void);
 extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
 extern int arch_cpu_up(int cpu);
 
+int cpu_up_send_sgi(int cpu);
+
 /* Secondary CPU entry point */
 extern void init_secondary(void);
 
-- 
1.7.12.1

The availability of a PSCI handler is advertised in the DTB.
Find and parse the node (described in the Linux device-tree binding)
and save the function number for bringing up a CPU for later usage.
We do some sanity checks, especially we deny using HVC as a calling
method, as it does not make much sense currently under Xen.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/Makefile      |  1 +
 xen/arch/arm/psci.c        | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/smpboot.c     |  7 +++++
 xen/include/asm-arm/psci.h |  6 +++++
 4 files changed, 78 insertions(+)
 create mode 100644 xen/arch/arm/psci.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 11cf663..d70f6d5 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -5,6 +5,7 @@ subdir-y += platforms
 obj-$(EARLY_PRINTK) += early_printk.o
 obj-y += cpu.o
 obj-y += domain.o
+obj-y += psci.o
 obj-y += vpsci.o
 obj-y += domctl.o
 obj-y += sysctl.o
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
new file mode 100644
index 0000000..9ff06cd
--- /dev/null
+++ b/xen/arch/arm/psci.c
@@ -0,0 +1,64 @@
+/*
+ * xen/arch/arm/psci.c
+ *
+ * PSCI host support
+ *
+ * Andre Przywara <andre.przywara@linaro.org>
+ * Copyright (c) 2013 Linaro Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+
+#include <xen/types.h>
+#include <xen/mm.h>
+#include <xen/smp.h>
+#include <asm/psci.h>
+
+int psci_available;
+
+static uint32_t psci_cpu_on_nr;
+
+int __init psci_init(void)
+{
+    struct dt_device_node *psci;
+    int ret;
+    const char *prop_str;
+
+    psci = dt_find_compatible_node(NULL, NULL, "arm,psci");
+    if ( !psci )
+        return -ENODEV;
+
+    ret = dt_property_read_string(psci, "method", &prop_str);
+    if ( ret )
+    {
+        printk("/psci node does not provide a method (%d)\n", ret);
+        return -EINVAL;
+    }
+
+    /* Since Xen runs in HYP all of the time, it does not make sense to
+     * let it call into HYP for PSCI handling, since the handler won''t
+     * just be there. So bail out with an error if "smc" is not used.
+     */
+    if ( strcmp(prop_str, "smc") )
+    {
+        printk("/psci method must be smc, but is:
\"%s\"\n", prop_str);
+        return -EINVAL;
+    }
+
+    if ( !dt_property_read_u32(psci, "cpu_on", &psci_cpu_on_nr) )
+    {
+        printk("/psci node is missing the \"cpu_on\"
property\n");
+        return -ENOENT;
+    }
+
+    return 0;
+}
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 52cef30..3a9be90 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -30,6 +30,7 @@
 #include <xen/irq.h>
 #include <xen/console.h>
 #include <asm/gic.h>
+#include <asm/psci.h>
 
 cpumask_t cpu_online_map;
 cpumask_t cpu_present_map;
@@ -105,6 +106,12 @@ void __init smp_init_cpus(void)
     bool_t bootcpu_valid = 0;
     int rc;
 
+    if ( psci_init() == 0 )
+    {
+        printk(XENLOG_INFO "Using PSCI for SMP bringup\n");
+        psci_available = 1;
+    }
+
     if ( (rc = arch_smp_init()) < 0 )
     {
         printk(XENLOG_WARNING "SMP init failed (%d)\n"
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 67d4c35..2f37612 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -6,6 +6,12 @@
 #define PSCI_EINVAL  -2
 #define PSCI_DENIED  -3
 
+/* availability of PSCI on the host for SMP bringup */
+extern int psci_available;
+
+int psci_init(void);
+
+/* functions to handle guest PSCI requests */
 int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
 int do_psci_cpu_off(uint32_t power_state);
 int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
-- 
1.7.12.1

The PSCI handler is invoked via a secure monitor call with the
arguments defined in registers. Copy the function from the
Linux code and adjust it to work on both ARM32 and ARM64.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/psci.c        | 28 ++++++++++++++++++++++++++++
 xen/include/asm-arm/psci.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 9ff06cd..cc382be 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -25,8 +25,36 @@
 
 int psci_available;
 
+#ifdef CONFIG_ARM_32
+#define REG_PREFIX "r"
+#else
+#define REG_PREFIX "x"
+#endif
+
+static noinline int __invoke_psci_fn_smc(u32 function_id, u32 arg0, u32 arg1,
+                                         u32 arg2)
+{
+    asm volatile(
+        __asmeq("%0", REG_PREFIX"0")
+        __asmeq("%1", REG_PREFIX"1")
+        __asmeq("%2", REG_PREFIX"2")
+        __asmeq("%3", REG_PREFIX"3")
+        "smc #0"
+        : "+r" (function_id)
+        : "r" (arg0), "r" (arg1), "r" (arg2));
+
+    return function_id;
+}
+
+#undef REG_PREFIX
+
 static uint32_t psci_cpu_on_nr;
 
+int call_psci_cpu_on(int cpu, void *smp_pen)
+{
+    return __invoke_psci_fn_smc(psci_cpu_on_nr, cpu, __pa(smp_pen), 0);
+}
+
 int __init psci_init(void)
 {
     struct dt_device_node *psci;
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 2f37612..50513bf 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -10,6 +10,7 @@
 extern int psci_available;
 
 int psci_init(void);
+int call_psci_cpu_on(int cpu, void *smp_pen);
 
 /* functions to handle guest PSCI requests */
 int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
-- 
1.7.12.1

If the device tree contains a PSCI node, we bring up secondary CPUs
by invoking the appropriate PSCI handler.
This will take priority over platform specific functions (which could
call the PSCI wrapper themselves if needed), so any PSCI enablement
of a platform will automatically be used (as on Linux).

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/arm32/smpboot.c | 4 +++-
 xen/arch/arm/platform.c      | 4 ++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c
index 88fe8fb..2a77f29 100644
--- a/xen/arch/arm/arm32/smpboot.c
+++ b/xen/arch/arm/arm32/smpboot.c
@@ -10,7 +10,9 @@ int __init arch_smp_init(void)
 
 int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
 {
-    /* TODO handle PSCI init */
+    /* Not needed on ARM32, as there is no relevant information in
+     * the CPU device tree node for ARMv7 CPUs.
+     */
     return 0;
 }
 
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 056d462..608e689 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -20,6 +20,7 @@
 #include <asm/platform.h>
 #include <xen/device_tree.h>
 #include <xen/init.h>
+#include <asm/psci.h>
 
 extern const struct platform_desc _splatform[], _eplatform[];
 
@@ -109,6 +110,9 @@ int __init platform_specific_mapping(struct domain *d)
 #ifdef CONFIG_ARM_32
 int __init platform_cpu_up(int cpu)
 {
+    if ( psci_available )
+        return call_psci_cpu_on(cpu, init_secondary);
+
     if ( platform && platform->cpu_up )
         return platform->cpu_up(cpu);
 
-- 
1.7.12.1

If the device tree contains a PSCI node and the DTB CPU node tells us
to use PSCI for enabling secondary cores, we set the function pointer
to the PSCI wrapper function to enable PSCI SMP bringup.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/arm64/smpboot.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 6a34bd4..a70b860 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -6,6 +6,7 @@
 #include <xen/smp.h>
 #include <xen/vmap.h>
 #include <asm/io.h>
+#include <asm/psci.h>
 
 struct smp_enable_ops {
         int             (*prepare_cpu)(int);
@@ -52,6 +53,23 @@ static void __init smp_spin_table_init(int cpu, struct
dt_device_node *dn)
     smp_enable_ops[cpu].prepare_cpu = smp_spin_table_cpu_up;
 }
 
+static int __init psci_cpu_up(int cpu)
+{
+    return call_psci_cpu_on(cpu, init_secondary);
+}
+
+static int __init smp_psci_init(int cpu)
+{
+    if ( !psci_available )
+    {
+        printk("CPU%d asks for PSCI, but DTB has no PSCI node\n",
cpu);
+        return -ENODEV;
+    }
+
+    smp_enable_ops[cpu].prepare_cpu = psci_cpu_up;
+    return 0;
+}
+
 int __init arch_smp_init(void)
 {
     /* Nothing */
@@ -71,7 +89,8 @@ int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
 
     if ( !strcmp(enable_method, "spin-table") )
         smp_spin_table_init(cpu, dn);
-    /* TODO: method "psci" */
+    else if ( !strcmp(enable_method, "psci") )
+        return smp_psci_init(cpu);
     else
     {
         printk("CPU%d has unknown enable method \"%s\"\n",
cpu, enable_method);
-- 
1.7.12.1

On 12/02/2013 11:08 AM, Andre Przywara wrote:
> Follow the current convention of prefixing guest related names
> with "v" by renaming the guest PSCI functionality into vpsci.c to
make
> room for the host PSCI functions.
I would also rename the header asm/psci.h => asm/vpsci.h
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/Makefile            | 2 +-
>   xen/arch/arm/{psci.c => vpsci.c} | 0
>   2 files changed, 1 insertion(+), 1 deletion(-)
>   rename xen/arch/arm/{psci.c => vpsci.c} (100%)
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 003ac84..11cf663 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -5,7 +5,7 @@ subdir-y += platforms
>   obj-$(EARLY_PRINTK) += early_printk.o
>   obj-y += cpu.o
>   obj-y += domain.o
> -obj-y += psci.o
> +obj-y += vpsci.o
>   obj-y += domctl.o
>   obj-y += sysctl.o
>   obj-y += domain_build.o
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/vpsci.c
> similarity index 100%
> rename from xen/arch/arm/psci.c
> rename to xen/arch/arm/vpsci.c
>
-- 
Julien Grall

On Mon, 2013-12-02 at 13:02 +0000, Julien Grall wrote:
> 
> On 12/02/2013 11:08 AM, Andre Przywara wrote:
> > Follow the current convention of prefixing guest related names
> > with "v" by renaming the guest PSCI functionality into
vpsci.c to make
> > room for the host PSCI functions.
> 
> I would also rename the header asm/psci.h => asm/vpsci.h
The majority of the header is stuff defined by the spec which is equally
common to be host and guest psci. Apart from a couple of prototypes for
the guest entry points.

I don''t think it makes sense to rename the header, or even to split the
tiny guest side parts into a new vpsci.h

Ian.

On 12/02/2013 01:06 PM, Ian Campbell wrote:
> On Mon, 2013-12-02 at 13:02 +0000, Julien Grall wrote:
>>
>> On 12/02/2013 11:08 AM, Andre Przywara wrote:
>>> Follow the current convention of prefixing guest related names
>>> with "v" by renaming the guest PSCI functionality into
vpsci.c to make
>>> room for the host PSCI functions.
>>
>> I would also rename the header asm/psci.h => asm/vpsci.h
>
> The majority of the header is stuff defined by the spec which is equally
> common to be host and guest psci. Apart from a couple of prototypes for
> the guest entry points.
>
> I don''t think it makes sense to rename the header, or even to
split the
> tiny guest side parts into a new vpsci.h
In this case, can we at least add "guest" (or something else) in the 
function name?

-- 
Julien Grall

On 12/02/2013 11:08 AM, Andre Przywara wrote:
> Currently we unconditionally send SGIs to all cores on SMP bringup.
> PSCI will not need this, so we move this into a function and call it
> explicitly from the platforms that need it. This gets us get rid of
> the empty cpu_up() platform functions in ARM32 and the comment in
> there.
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/arm64/smpboot.c      |  2 +-
>   xen/arch/arm/platform.c           |  2 +-
>   xen/arch/arm/platforms/exynos5.c  | 11 +----------
>   xen/arch/arm/platforms/omap5.c    | 11 +----------
>   xen/arch/arm/platforms/vexpress.c | 10 +---------
>   xen/arch/arm/smpboot.c            | 15 ++++++++++-----
>   xen/include/asm-arm/smp.h         |  2 ++
>   7 files changed, 17 insertions(+), 36 deletions(-)
>
[..]
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index a7f9ee4..056d462 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -112,7 +112,7 @@ int __init platform_cpu_up(int cpu)
>       if ( platform && platform->cpu_up )
>           return platform->cpu_up(cpu);
>
> -    return -EAGAIN;
> +    return -ENODEV;
This change seems unrelated to this patch.

-- 
Julien Grall

On 12/02/2013 02:16 PM, Julien Grall wrote:
>
>
> On 12/02/2013 11:08 AM, Andre Przywara wrote:
>> Currently we unconditionally send SGIs to all cores on SMP bringup.
>> PSCI will not need this, so we move this into a function and call it
>> explicitly from the platforms that need it. This gets us get rid of
>> the empty cpu_up() platform functions in ARM32 and the comment in
>> there.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   xen/arch/arm/arm64/smpboot.c      |  2 +-
>>   xen/arch/arm/platform.c           |  2 +-
>>   xen/arch/arm/platforms/exynos5.c  | 11 +----------
>>   xen/arch/arm/platforms/omap5.c    | 11 +----------
>>   xen/arch/arm/platforms/vexpress.c | 10 +---------
>>   xen/arch/arm/smpboot.c            | 15 ++++++++++-----
>>   xen/include/asm-arm/smp.h         |  2 ++
>>   7 files changed, 17 insertions(+), 36 deletions(-)
>>
>
> [..]
>
>> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
>> index a7f9ee4..056d462 100644
>> --- a/xen/arch/arm/platform.c
>> +++ b/xen/arch/arm/platform.c
>> @@ -112,7 +112,7 @@ int __init platform_cpu_up(int cpu)
>>       if ( platform && platform->cpu_up )
>>           return platform->cpu_up(cpu);
>>
>> -    return -EAGAIN;
>> +    return -ENODEV;
>
> This change seems unrelated to this patch.
That is disputable. If I read the original code directly, the intention 
of EAGAIN was like: "There is no platform specific code, try something 
else." Now with PSCI and the GIC kicking already folded into this 
function, the answer to "no PSCI and no platform code" is simply: I 
don''t know how to enable SMP, so there is no SMP => ENODEV.

I can add a comment if this helps.

Regards,
Andre.

On 12/02/2013 11:08 AM, Andre Przywara wrote:
> The availability of a PSCI handler is advertised in the DTB.
> Find and parse the node (described in the Linux device-tree binding)
> and save the function number for bringing up a CPU for later usage.
> We do some sanity checks, especially we deny using HVC as a calling
> method, as it does not make much sense currently under Xen.
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/Makefile      |  1 +
>   xen/arch/arm/psci.c        | 64
++++++++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/smpboot.c     |  7 +++++
>   xen/include/asm-arm/psci.h |  6 +++++
>   4 files changed, 78 insertions(+)
>   create mode 100644 xen/arch/arm/psci.c
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 11cf663..d70f6d5 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -5,6 +5,7 @@ subdir-y += platforms
>   obj-$(EARLY_PRINTK) += early_printk.o
>   obj-y += cpu.o
>   obj-y += domain.o
> +obj-y += psci.o
>   obj-y += vpsci.o
>   obj-y += domctl.o
>   obj-y += sysctl.o
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> new file mode 100644
> index 0000000..9ff06cd
> --- /dev/null
> +++ b/xen/arch/arm/psci.c
> @@ -0,0 +1,64 @@
> +/*
> + * xen/arch/arm/psci.c
> + *
> + * PSCI host support
> + *
> + * Andre Przywara <andre.przywara@linaro.org>
> + * Copyright (c) 2013 Linaro Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +
> +#include <xen/types.h>
> +#include <xen/mm.h>
> +#include <xen/smp.h>
> +#include <asm/psci.h>
> +
> +int psci_available;
I would use bool_t here.
> +
> +static uint32_t psci_cpu_on_nr;
What about handling shutdown/reboot?
> +
> +int __init psci_init(void)
> +{
> +    struct dt_device_node *psci;
You don''t modify psci so you can add const before.
> +    int ret;
> +    const char *prop_str;
> +
> +    psci = dt_find_compatible_node(NULL, NULL, "arm,psci");
> +    if ( !psci )
> +        return -ENODEV;
> +
> +    ret = dt_property_read_string(psci, "method",
&prop_str);
> +    if ( ret )
> +    {
> +        printk("/psci node does not provide a method (%d)\n",
ret);
> +        return -EINVAL;
> +    }
> +
> +    /* Since Xen runs in HYP all of the time, it does not make sense to
> +     * let it call into HYP for PSCI handling, since the handler
won''t
> +     * just be there. So bail out with an error if "smc" is not
used.
> +     */
> +    if ( strcmp(prop_str, "smc") )
As we only handle "hvc" method, why not checking if we use the right 
method and bail out in all other case? It will help for the future, if 
PSCI guys decide to implement another method.
> +    {
> +        printk("/psci method must be smc, but is:
\"%s\"\n", prop_str);
> +        return -EINVAL;
> +    }
> +
> +    if ( !dt_property_read_u32(psci, "cpu_on",
&psci_cpu_on_nr) )
> +    {
> +        printk("/psci node is missing the \"cpu_on\"
property\n");
> +        return -ENOENT;
> +    }
> +
> +    return 0;
> +}
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 52cef30..3a9be90 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -30,6 +30,7 @@
>   #include <xen/irq.h>
>   #include <xen/console.h>
>   #include <asm/gic.h>
> +#include <asm/psci.h>
>
>   cpumask_t cpu_online_map;
>   cpumask_t cpu_present_map;
> @@ -105,6 +106,12 @@ void __init smp_init_cpus(void)
>       bool_t bootcpu_valid = 0;
>       int rc;
>
> +    if ( psci_init() == 0 )
> +    {
> +        printk(XENLOG_INFO "Using PSCI for SMP bringup\n");
> +        psci_available = 1;
> +    }
> +
>       if ( (rc = arch_smp_init()) < 0 )
>       {
>           printk(XENLOG_WARNING "SMP init failed (%d)\n"
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index 67d4c35..2f37612 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -6,6 +6,12 @@
>   #define PSCI_EINVAL  -2
>   #define PSCI_DENIED  -3
>
> +/* availability of PSCI on the host for SMP bringup */
> +extern int psci_available;
> +
> +int psci_init(void);
> +
> +/* functions to handle guest PSCI requests */
>   int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
>   int do_psci_cpu_off(uint32_t power_state);
>   int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
>
-- 
Julien Grall

On 12/02/2013 02:28 PM, Julien Grall wrote:
>
>
> On 12/02/2013 11:08 AM, Andre Przywara wrote:
>> The availability of a PSCI handler is advertised in the DTB.
>> Find and parse the node (described in the Linux device-tree binding)
>> and save the function number for bringing up a CPU for later usage.
>> We do some sanity checks, especially we deny using HVC as a calling
>> method, as it does not make much sense currently under Xen.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   xen/arch/arm/Makefile      |  1 +
>>   xen/arch/arm/psci.c        | 64
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>   xen/arch/arm/smpboot.c     |  7 +++++
>>   xen/include/asm-arm/psci.h |  6 +++++
>>   4 files changed, 78 insertions(+)
>>   create mode 100644 xen/arch/arm/psci.c
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 11cf663..d70f6d5 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -5,6 +5,7 @@ subdir-y += platforms
>>   obj-$(EARLY_PRINTK) += early_printk.o
>>   obj-y += cpu.o
>>   obj-y += domain.o
>> +obj-y += psci.o
>>   obj-y += vpsci.o
>>   obj-y += domctl.o
>>   obj-y += sysctl.o
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> new file mode 100644
>> index 0000000..9ff06cd
>> --- /dev/null
>> +++ b/xen/arch/arm/psci.c
>> @@ -0,0 +1,64 @@
>> +/*
>> + * xen/arch/arm/psci.c
>> + *
>> + * PSCI host support
>> + *
>> + * Andre Przywara <andre.przywara@linaro.org>
>> + * Copyright (c) 2013 Linaro Limited.
>> + *
>> + * This program is free software; you can redistribute it and/or
modify
>> + * it under the terms of the GNU General Public License as published
by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * 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.
>> + */
>> +
>> +
>> +#include <xen/types.h>
>> +#include <xen/mm.h>
>> +#include <xen/smp.h>
>> +#include <asm/psci.h>
>> +
>> +int psci_available;
>
> I would use bool_t here.
Yes.
>
>> +
>> +static uint32_t psci_cpu_on_nr;
>
> What about handling shutdown/reboot?
Given the pressure on the release, I''d hold any extra wishes back for 
now and just enable PSCI for SMP bringup.
>
>> +
>> +int __init psci_init(void)
>> +{
>> +    struct dt_device_node *psci;
>
> You don''t modify psci so you can add const before.
Agreed.
>> +    int ret;
>> +    const char *prop_str;
>> +
>> +    psci = dt_find_compatible_node(NULL, NULL, "arm,psci");
>> +    if ( !psci )
>> +        return -ENODEV;
>> +
>> +    ret = dt_property_read_string(psci, "method",
&prop_str);
>> +    if ( ret )
>> +    {
>> +        printk("/psci node does not provide a method
(%d)\n", ret);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* Since Xen runs in HYP all of the time, it does not make sense
to
>> +     * let it call into HYP for PSCI handling, since the handler
won''t
>> +     * just be there. So bail out with an error if "smc" is
not used.
>> +     */
>> +    if ( strcmp(prop_str, "smc") )
>
> As we only handle "hvc" method, why not checking if we use the
right
> method and bail out in all other case? It will help for the future, if
> PSCI guys decide to implement another method.
We don''t handle hvc, but only smc. Remember the inverse logic of
strcmp.

Thanks for the review!
Andre.
>
>> +    {
>> +        printk("/psci method must be smc, but is:
\"%s\"\n", prop_str);
>> +        return -EINVAL;
>> +    }
>> +
>> +    if ( !dt_property_read_u32(psci, "cpu_on",
&psci_cpu_on_nr) )
>> +    {
>> +        printk("/psci node is missing the \"cpu_on\"
property\n");
>> +        return -ENOENT;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index 52cef30..3a9be90 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -30,6 +30,7 @@
>>   #include <xen/irq.h>
>>   #include <xen/console.h>
>>   #include <asm/gic.h>
>> +#include <asm/psci.h>
>>
>>   cpumask_t cpu_online_map;
>>   cpumask_t cpu_present_map;
>> @@ -105,6 +106,12 @@ void __init smp_init_cpus(void)
>>       bool_t bootcpu_valid = 0;
>>       int rc;
>>
>> +    if ( psci_init() == 0 )
>> +    {
>> +        printk(XENLOG_INFO "Using PSCI for SMP bringup\n");
>> +        psci_available = 1;
>> +    }
>> +
>>       if ( (rc = arch_smp_init()) < 0 )
>>       {
>>           printk(XENLOG_WARNING "SMP init failed (%d)\n"
>> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
>> index 67d4c35..2f37612 100644
>> --- a/xen/include/asm-arm/psci.h
>> +++ b/xen/include/asm-arm/psci.h
>> @@ -6,6 +6,12 @@
>>   #define PSCI_EINVAL  -2
>>   #define PSCI_DENIED  -3
>>
>> +/* availability of PSCI on the host for SMP bringup */
>> +extern int psci_available;
>> +
>> +int psci_init(void);
>> +
>> +/* functions to handle guest PSCI requests */
>>   int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
>>   int do_psci_cpu_off(uint32_t power_state);
>>   int do_psci_cpu_suspend(uint32_t power_state, register_t
entry_point);
>>
>

On 12/02/2013 01:24 PM, Andre Przywara wrote:
> On 12/02/2013 02:16 PM, Julien Grall wrote:
>>
>>
>> On 12/02/2013 11:08 AM, Andre Przywara wrote:
>>> Currently we unconditionally send SGIs to all cores on SMP bringup.
>>> PSCI will not need this, so we move this into a function and call
it
>>> explicitly from the platforms that need it. This gets us get rid of
>>> the empty cpu_up() platform functions in ARM32 and the comment in
>>> there.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>> ---
>>>   xen/arch/arm/arm64/smpboot.c      |  2 +-
>>>   xen/arch/arm/platform.c           |  2 +-
>>>   xen/arch/arm/platforms/exynos5.c  | 11 +----------
>>>   xen/arch/arm/platforms/omap5.c    | 11 +----------
>>>   xen/arch/arm/platforms/vexpress.c | 10 +---------
>>>   xen/arch/arm/smpboot.c            | 15 ++++++++++-----
>>>   xen/include/asm-arm/smp.h         |  2 ++
>>>   7 files changed, 17 insertions(+), 36 deletions(-)
>>>
>>
>> [..]
>>
>>> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
>>> index a7f9ee4..056d462 100644
>>> --- a/xen/arch/arm/platform.c
>>> +++ b/xen/arch/arm/platform.c
>>> @@ -112,7 +112,7 @@ int __init platform_cpu_up(int cpu)
>>>       if ( platform && platform->cpu_up )
>>>           return platform->cpu_up(cpu);
>>>
>>> -    return -EAGAIN;
>>> +    return -ENODEV;
>>
>> This change seems unrelated to this patch.
>
> That is disputable. If I read the original code directly, the intention
> of EAGAIN was like: "There is no platform specific code, try something
> else." Now with PSCI and the GIC kicking already folded into this
> function, the answer to "no PSCI and no platform code" is simply:
I
> don''t know how to enable SMP, so there is no SMP => ENODEV.
>
> I can add a comment if this helps.
Thanks for the explanation, I don''t think it needs a comment.

Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> This is a major rework of last week''s ARM PSCI host support. The
code
> has been moved into a separate file and the code flow has been
> changed substantially (see below for more details).
>   ---------
> 
> Xen did not make use of the host provided ARM PSCI (Power State
> Coordination Interface [1]) functionality so far, but relied on
> platform specific SMP bringup functions.
> This series adds support for PSCI on the host by reading the required
> information from the DTB and invoking the appropriate handler when
> bringing up each single CPU.
> Since PSCI is defined for both ARM32 and ARM64, I added code for both
> architectures, though only ARM32 is tested on Midway and VExpress
> (without any PSCI support on the latter, just a regression test).
> ARM64 code was compile tested only.
> 
> The ARM32 SMP boot flow is now as following:
> The DTB is scanned for a node announcing PSCI support. If that is
> available, call the PSCI handler via SMC to bringup the secondary
> cores. If no PSCI has been found, call the platform specific cpu_up()
> function.
Is this the most useful way round? If both PSCI and a hook are present
might it be expected that the hook might want to make the choice to
either go manual or call back to PSCI?

Perhaps to handle some quirk (aka bug) in the platform which needs
something else before/after the PSCI stuff.
>  It is now the responsibility of those functions to do the
> GIC SGI call to kick the secondary CPUs. For that a function is now
> provided which does this, so three platforms now reference this
> generic function instead of coding up an empty one and relying on the
> GIC kick in Xen code later.
Note that the existing kick serves two purposes. The first is to wake up
the processor from the firmware on platforms which need that. The second
is to wake up secondaries from the loop in head.S under:
        /* Non-boot CPUs wait here until __cpu_up is ready for them */
where they wait for smp_up_cpu to become them.

I should go read the patches to see what you''ve actually done here.
> The ARM64 SMP boot flow is different: PSCI is not only described in a
> root DTB node, but also advertised in each core''s node as the
> preferred SMP bringup method. So the PSCI call wrapper function is
> registered as the cpu_up function when the DTB is scanned.
> 
> This patch series is split up as follows:
> 1/6) rename psci.c to vpsci.c to make room for the new, host-related
>      code
> 2/6) move the GIC SGI kick into a separate function and call it now
>      directly from the platform code, removing the empty cpu_up()
> 3/6) parse the DTB for a PSCI node and store the needed function index
> 4/6) add a function to actually invoke the firmware''s PSCI handler
> 5/6) enable PSCI on ARM32 by adding the call to the PSCI handler
> 6/6) enable PSCI on ARM64 by registering the PSCI handler function
> 
> 
> Changes from v1:
> - much rework, PSCI host DTB scanning unchanged (v1: 1/4, v2: 3/6)
> - moved host PSCI code to psci.c, renaming the old one to vpsci.c
> - have a separate psci_available variable
> - removing explicit "host" from function names
> - returning standard error codes on DTB scanning
> - do the ARM64 PSCI call from a registered function pointer
> - move the GIC kick into a separate function
> - more change to the code flow in general
> 
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> 
> [1]:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0022b/index.html
> 
> Andre Przywara (6):
>   arm: rename xen/arch/arm/psci.c into vpsci.c
>   arm: move GIC SGI kicking into separate function
>   arm: parse PSCI node from the host device-tree
>   arm: add a function to invoke the PSCI handler
>   arm32: enable PSCI secondary CPU bringup
>   arm64: enable PSCI secondary CPU bringup
> 
>  xen/arch/arm/Makefile             |   1 +
>  xen/arch/arm/arm32/smpboot.c      |   4 +-
>  xen/arch/arm/arm64/smpboot.c      |  23 ++++++-
>  xen/arch/arm/platform.c           |   6 +-
>  xen/arch/arm/platforms/exynos5.c  |  11 +---
>  xen/arch/arm/platforms/omap5.c    |  11 +---
>  xen/arch/arm/platforms/vexpress.c |  10 +--
>  xen/arch/arm/psci.c               | 127
+++++++++++++++++++-------------------
>  xen/arch/arm/smpboot.c            |  22 +++++--
>  xen/arch/arm/vpsci.c              |  93 ++++++++++++++++++++++++++++
>  xen/include/asm-arm/psci.h        |   7 +++
>  xen/include/asm-arm/smp.h         |   2 +
>  12 files changed, 215 insertions(+), 102 deletions(-)
>  create mode 100644 xen/arch/arm/vpsci.c
>

On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> Follow the current convention of prefixing guest related names
> with "v" by renaming the guest PSCI functionality into vpsci.c to
make
> room for the host PSCI functions.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

On 12/02/2013 01:44 PM, Andre Przywara wrote:
> On 12/02/2013 02:28 PM, Julien Grall wrote:
>>
>>
>> On 12/02/2013 11:08 AM, Andre Przywara wrote:
>>> The availability of a PSCI handler is advertised in the DTB.
>>> Find and parse the node (described in the Linux device-tree
binding)
>>> and save the function number for bringing up a CPU for later usage.
>>> We do some sanity checks, especially we deny using HVC as a calling
>>> method, as it does not make much sense currently under Xen.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>> ---
>>>   xen/arch/arm/Makefile      |  1 +
>>>   xen/arch/arm/psci.c        | 64
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>   xen/arch/arm/smpboot.c     |  7 +++++
>>>   xen/include/asm-arm/psci.h |  6 +++++
>>>   4 files changed, 78 insertions(+)
>>>   create mode 100644 xen/arch/arm/psci.c
>>>
>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>>> index 11cf663..d70f6d5 100644
>>> --- a/xen/arch/arm/Makefile
>>> +++ b/xen/arch/arm/Makefile
>>> @@ -5,6 +5,7 @@ subdir-y += platforms
>>>   obj-$(EARLY_PRINTK) += early_printk.o
>>>   obj-y += cpu.o
>>>   obj-y += domain.o
>>> +obj-y += psci.o
>>>   obj-y += vpsci.o
>>>   obj-y += domctl.o
>>>   obj-y += sysctl.o
>>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>>> new file mode 100644
>>> index 0000000..9ff06cd
>>> --- /dev/null
>>> +++ b/xen/arch/arm/psci.c
>>> @@ -0,0 +1,64 @@
>>> +/*
>>> + * xen/arch/arm/psci.c
>>> + *
>>> + * PSCI host support
>>> + *
>>> + * Andre Przywara <andre.przywara@linaro.org>
>>> + * Copyright (c) 2013 Linaro Limited.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
modify
>>> + * it under the terms of the GNU General Public License as
published by
>>> + * the Free Software Foundation; either version 2 of the License,
or
>>> + * (at your option) any later version.
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +
>>> +#include <xen/types.h>
>>> +#include <xen/mm.h>
>>> +#include <xen/smp.h>
>>> +#include <asm/psci.h>
>>> +
>>> +int psci_available;
>>
>> I would use bool_t here.
>
> Yes.
>
>>
>>> +
>>> +static uint32_t psci_cpu_on_nr;
>>
>> What about handling shutdown/reboot?
>
> Given the pressure on the release, I''d hold any extra wishes back
for
> now and just enable PSCI for SMP bringup.
It''s a bit annoying to not being able to reboot the platform via 
"reboot" command in dom0.

Even if you don''t implement reboot for this patch series, how about 
creating an array to store the different value ie (on, off,...)?
>>> +    int ret;
>>> +    const char *prop_str;
>>> +
>>> +    psci = dt_find_compatible_node(NULL, NULL,
"arm,psci");
>>> +    if ( !psci )
>>> +        return -ENODEV;
>>> +
>>> +    ret = dt_property_read_string(psci, "method",
&prop_str);
>>> +    if ( ret )
>>> +    {
>>> +        printk("/psci node does not provide a method
(%d)\n", ret);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* Since Xen runs in HYP all of the time, it does not make
sense to
>>> +     * let it call into HYP for PSCI handling, since the handler
won''t
>>> +     * just be there. So bail out with an error if "smc"
is not used.
>>> +     */
>>> +    if ( strcmp(prop_str, "smc") )
>>
>> As we only handle "hvc" method, why not checking if we use
the right
>> method and bail out in all other case? It will help for the future, if
>> PSCI guys decide to implement another method.
>
> We don''t handle hvc, but only smc. Remember the inverse logic of
strcmp.
Oh right, I was thinking about guest, no host :)


-- 
Julien Grall

On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> Currently we unconditionally send SGIs to all cores on SMP bringup.
> PSCI will not need this, so we move this into a function and call it
> explicitly from the platforms that need it. This gets us get rid of
> the empty cpu_up() platform functions in ARM32 and the comment in
> there.
I don''t think this is quite true -- even on a PSCI system the kick is
required to get past the gate in head.S.

I wonder how this interacts with PSCI implementations which use an SGI
themselves internally...
> @@ -376,11 +386,6 @@ int __cpu_up(unsigned int cpu)
>          return rc;
>      }
>  
> -    /* We don''t know the GIC ID of the CPU until it has woken up,
so just signal
> -     * everyone and rely on our own smp_up_cpu gate to ensure only the one
we
> -     * want gets through. */
> -    send_SGI_allbutself(GIC_SGI_EVENT_CHECK);
> -
So, I was saying in the 00 mail I''m not sure we can get rid of this
altogether.

But I suppose it is the intention that the platform code always has both
its own logic and this SGI kick (possibly coalesced) in such
circumstances? Which is probably ok?


>      while ( !cpu_online(cpu) )
>      {
>          cpu_relax();
> diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
> index 1485cc6..a1de03c 100644
> --- a/xen/include/asm-arm/smp.h
> +++ b/xen/include/asm-arm/smp.h
> @@ -21,6 +21,8 @@ extern int arch_smp_init(void);
>  extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
>  extern int arch_cpu_up(int cpu);
>  
> +int cpu_up_send_sgi(int cpu);
> +
>  /* Secondary CPU entry point */
>  extern void init_secondary(void);
>

On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> +int __init psci_init(void)
> +{
[...]
> +    /* Since Xen runs in HYP all of the time, it does not make sense to
> +     * let it call into HYP for PSCI handling, since the handler
won''t
> +     * just be there. So bail out with an error if "smc" is not
used.
s/won''t just/just won''t/.
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 52cef30..3a9be90 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> [...]
> @@ -105,6 +106,12 @@ void __init smp_init_cpus(void)
>      bool_t bootcpu_valid = 0;
>      int rc;
>  
> +    if ( psci_init() == 0 )
> +    {
> +        printk(XENLOG_INFO "Using PSCI for SMP bringup\n");
> +        psci_available = 1;
IMHO this log + flag twiddling belong as the last act of psci init.

Other than those two things this patch looks good. If you change at
least the second one then:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

(I leave the first one up to you in case I''ve simply misparsed it)

Ian.

On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> The PSCI handler is invoked via a secure monitor call with the
> arguments defined in registers. Copy the function from the
> Linux code and adjust it to work on both ARM32 and ARM64.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  xen/arch/arm/psci.c        | 28 ++++++++++++++++++++++++++++
>  xen/include/asm-arm/psci.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 9ff06cd..cc382be 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -25,8 +25,36 @@
>  
>  int psci_available;
>  
> +#ifdef CONFIG_ARM_32
> +#define REG_PREFIX "r"
> +#else
> +#define REG_PREFIX "x"
> +#endif
> +
> +static noinline int __invoke_psci_fn_smc(u32 function_id, u32 arg0, u32
arg1,
> +                                         u32 arg2)
I should reread the PSCI spec, but are these not 64-bit on AArch64?
> +{
> +    asm volatile(
> +        __asmeq("%0", REG_PREFIX"0")
> +        __asmeq("%1", REG_PREFIX"1")
> +        __asmeq("%2", REG_PREFIX"2")
> +        __asmeq("%3", REG_PREFIX"3")
> +        "smc #0"
> +        : "+r" (function_id)
> +        : "r" (arg0), "r" (arg1), "r"
(arg2));
> +
> +    return function_id;
> +}
> +
> +#undef REG_PREFIX
> +
>  static uint32_t psci_cpu_on_nr;
>  
> +int call_psci_cpu_on(int cpu, void *smp_pen)
> +{
> +    return __invoke_psci_fn_smc(psci_cpu_on_nr, cpu, __pa(smp_pen), 0);
> +}
> +
>  int __init psci_init(void)
>  {
>      struct dt_device_node *psci;
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index 2f37612..50513bf 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -10,6 +10,7 @@
>  extern int psci_available;
>  
>  int psci_init(void);
> +int call_psci_cpu_on(int cpu, void *smp_pen);
>  
>  /* functions to handle guest PSCI requests */
>  int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);

On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> @@ -109,6 +110,9 @@ int __init platform_specific_mapping(struct domain *d)
>  #ifdef CONFIG_ARM_32
>  int __init platform_cpu_up(int cpu)
>  {
> +    if ( psci_available )
> +        return call_psci_cpu_on(cpu, init_secondary);
> +
>      if ( platform && platform->cpu_up )
>          return platform->cpu_up(cpu);
>  
Make me wonder why we don''t either hardcode init_secondary in
call_psci_cpu_on or pass it as a parameter to cpu_up, but regardless:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(FWIW if you were to change things, I''d be in favour of hardcoding on
the PSCI side)

Ian.

On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> If the device tree contains a PSCI node and the DTB CPU node tells us
> to use PSCI for enabling secondary cores, we set the function pointer
> to the PSCI wrapper function to enable PSCI SMP bringup.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

On Mon, 2013-12-02 at 15:00 +0000, Julien Grall wrote:
> It''s a bit annoying to not being able to reboot the platform via 
> "reboot" command in dom0.
Followup patches will be considered.
> Even if you don''t implement reboot for this patch series, how
about
> creating an array to store the different value ie (on, off,...)?
Please don''t, a per-function variable is fine and I''d rather
have a
static psci_cpu_on_nr than psci_nrs[1] or something. Even if 1 is a
#define it''s unnecessary.

In any case there should be a wrapper function.

Ian.

On 12/02/2013 04:01 PM, Ian Campbell wrote:
> On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
>> Currently we unconditionally send SGIs to all cores on SMP bringup.
>> PSCI will not need this, so we move this into a function and call it
>> explicitly from the platforms that need it. This gets us get rid of
>> the empty cpu_up() platform functions in ARM32 and the comment in
>> there.
>
> I don''t think this is quite true -- even on a PSCI system the kick
is
> required to get past the gate in head.S.
Right, but this is the responsibility of the PSCI handler in the 
firmware, right? I was under the assumption that the semantics of cpu_on 
is to start executing code at the given address, whatever this takes 
internally.
Calxeda firmware for instance does the SGI kick.
>
> I wonder how this interacts with PSCI implementations which use an SGI
> themselves internally...
>
>> @@ -376,11 +386,6 @@ int __cpu_up(unsigned int cpu)
>>           return rc;
>>       }
>>
>> -    /* We don''t know the GIC ID of the CPU until it has woken
up, so just signal
>> -     * everyone and rely on our own smp_up_cpu gate to ensure only the
one we
>> -     * want gets through. */
>> -    send_SGI_allbutself(GIC_SGI_EVENT_CHECK);
>> -
>
> So, I was saying in the 00 mail I''m not sure we can get rid of
this
> altogether.
Please note that we do not get rid of this, but just move it. ARM64 
calls it in arm64/smpboot.c, ARM32 non-PSCI platforms call this now 
explicitly by pointing to that function in their platforms/foo.c file.
>
> But I suppose it is the intention that the platform code always has both
> its own logic and this SGI kick (possibly coalesced) in such
> circumstances? Which is probably ok?
That was my thinking, yes.

Regards,
Andre.

>>       while ( !cpu_online(cpu) )
>>       {
>>           cpu_relax();
>> diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
>> index 1485cc6..a1de03c 100644
>> --- a/xen/include/asm-arm/smp.h
>> +++ b/xen/include/asm-arm/smp.h
>> @@ -21,6 +21,8 @@ extern int arch_smp_init(void);
>>   extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
>>   extern int arch_cpu_up(int cpu);
>>
>> +int cpu_up_send_sgi(int cpu);
>> +
>>   /* Secondary CPU entry point */
>>   extern void init_secondary(void);
>>
>
>

On 12/02/2013 03:52 PM, Ian Campbell wrote:
> On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
>> This is a major rework of last week''s ARM PSCI host support.
The code
>> has been moved into a separate file and the code flow has been
>> changed substantially (see below for more details).
>>    ---------
>>
>> Xen did not make use of the host provided ARM PSCI (Power State
>> Coordination Interface [1]) functionality so far, but relied on
>> platform specific SMP bringup functions.
>> This series adds support for PSCI on the host by reading the required
>> information from the DTB and invoking the appropriate handler when
>> bringing up each single CPU.
>> Since PSCI is defined for both ARM32 and ARM64, I added code for both
>> architectures, though only ARM32 is tested on Midway and VExpress
>> (without any PSCI support on the latter, just a regression test).
>> ARM64 code was compile tested only.
>>
>> The ARM32 SMP boot flow is now as following:
>> The DTB is scanned for a node announcing PSCI support. If that is
>> available, call the PSCI handler via SMC to bringup the secondary
>> cores. If no PSCI has been found, call the platform specific cpu_up()
>> function.
>
> Is this the most useful way round? If both PSCI and a hook are present
> might it be expected that the hook might want to make the choice to
> either go manual or call back to PSCI?
>
> Perhaps to handle some quirk (aka bug) in the platform which needs
> something else before/after the PSCI stuff.
I thought about that too and had it actually that way in the first place.
My reason to change it was: What happens if platforms get PSCI support?
Thinking about sunxi or Arndale. If the device tree advertises PSCI, 
then this is supposed to work. If it doesn''t work, the DTB
shouldn''t
carry the PSCI node or u-boot & friends have to remove it.

In general I agree on the "fix" idea, but I''d ask for that
any bug in
PSCI kicking should be fixed in the PSCI firmware and not in Xen or 
other kernels. If we are really in need for SMP fixes, we could still 
put them into smp_init().

I can easily change it to prefer platform code, of course.
>
>>   It is now the responsibility of those functions to do the
>> GIC SGI call to kick the secondary CPUs. For that a function is now
>> provided which does this, so three platforms now reference this
>> generic function instead of coding up an empty one and relying on the
>> GIC kick in Xen code later.
>
> Note that the existing kick serves two purposes. The first is to wake up
> the processor from the firmware on platforms which need that. The second
> is to wake up secondaries from the loop in head.S under:
>          /* Non-boot CPUs wait here until __cpu_up is ready for them */
> where they wait for smp_up_cpu to become them.
>
> I should go read the patches to see what you''ve actually done
here.
But currently there is only one kick per CPU, right? I didn''t change 
anything in this respect, just moved the code into a function and called 
that a bit earlier.

Regards,
Andre.
>
>> The ARM64 SMP boot flow is different: PSCI is not only described in a
>> root DTB node, but also advertised in each core''s node as the
>> preferred SMP bringup method. So the PSCI call wrapper function is
>> registered as the cpu_up function when the DTB is scanned.
>>
>> This patch series is split up as follows:
>> 1/6) rename psci.c to vpsci.c to make room for the new, host-related
>>       code
>> 2/6) move the GIC SGI kick into a separate function and call it now
>>       directly from the platform code, removing the empty cpu_up()
>> 3/6) parse the DTB for a PSCI node and store the needed function index
>> 4/6) add a function to actually invoke the firmware''s PSCI
handler
>> 5/6) enable PSCI on ARM32 by adding the call to the PSCI handler
>> 6/6) enable PSCI on ARM64 by registering the PSCI handler function
>>
>>
>> Changes from v1:
>> - much rework, PSCI host DTB scanning unchanged (v1: 1/4, v2: 3/6)
>> - moved host PSCI code to psci.c, renaming the old one to vpsci.c
>> - have a separate psci_available variable
>> - removing explicit "host" from function names
>> - returning standard error codes on DTB scanning
>> - do the ARM64 PSCI call from a registered function pointer
>> - move the GIC kick into a separate function
>> - more change to the code flow in general
>>
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>
>> [1]:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0022b/index.html
>>
>> Andre Przywara (6):
>>    arm: rename xen/arch/arm/psci.c into vpsci.c
>>    arm: move GIC SGI kicking into separate function
>>    arm: parse PSCI node from the host device-tree
>>    arm: add a function to invoke the PSCI handler
>>    arm32: enable PSCI secondary CPU bringup
>>    arm64: enable PSCI secondary CPU bringup
>>
>>   xen/arch/arm/Makefile             |   1 +
>>   xen/arch/arm/arm32/smpboot.c      |   4 +-
>>   xen/arch/arm/arm64/smpboot.c      |  23 ++++++-
>>   xen/arch/arm/platform.c           |   6 +-
>>   xen/arch/arm/platforms/exynos5.c  |  11 +---
>>   xen/arch/arm/platforms/omap5.c    |  11 +---
>>   xen/arch/arm/platforms/vexpress.c |  10 +--
>>   xen/arch/arm/psci.c               | 127
+++++++++++++++++++-------------------
>>   xen/arch/arm/smpboot.c            |  22 +++++--
>>   xen/arch/arm/vpsci.c              |  93 ++++++++++++++++++++++++++++
>>   xen/include/asm-arm/psci.h        |   7 +++
>>   xen/include/asm-arm/smp.h         |   2 +
>>   12 files changed, 215 insertions(+), 102 deletions(-)
>>   create mode 100644 xen/arch/arm/vpsci.c
>>
>
>

On 12/02/2013 04:07 PM, Ian Campbell wrote:
> On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
>> The PSCI handler is invoked via a secure monitor call with the
>> arguments defined in registers. Copy the function from the
>> Linux code and adjust it to work on both ARM32 and ARM64.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   xen/arch/arm/psci.c        | 28 ++++++++++++++++++++++++++++
>>   xen/include/asm-arm/psci.h |  1 +
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 9ff06cd..cc382be 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -25,8 +25,36 @@
>>
>>   int psci_available;
>>
>> +#ifdef CONFIG_ARM_32
>> +#define REG_PREFIX "r"
>> +#else
>> +#define REG_PREFIX "x"
>> +#endif
>> +
>> +static noinline int __invoke_psci_fn_smc(u32 function_id, u32 arg0,
u32 arg1,
>> +                                         u32 arg2)
>
> I should reread the PSCI spec, but are these not 64-bit on AArch64?
Indeed. Not for all functions and parameters, but for the start address 
at least that makes sense ;-)
Is there a type in Xen which reliably holds a native word? ulong or 
something?

Thanks,
Andre.

>
>> +{
>> +    asm volatile(
>> +        __asmeq("%0", REG_PREFIX"0")
>> +        __asmeq("%1", REG_PREFIX"1")
>> +        __asmeq("%2", REG_PREFIX"2")
>> +        __asmeq("%3", REG_PREFIX"3")
>> +        "smc #0"
>> +        : "+r" (function_id)
>> +        : "r" (arg0), "r" (arg1), "r"
(arg2));
>> +
>> +    return function_id;
>> +}
>> +
>> +#undef REG_PREFIX
>> +
>>   static uint32_t psci_cpu_on_nr;
>>
>> +int call_psci_cpu_on(int cpu, void *smp_pen)
>> +{
>> +    return __invoke_psci_fn_smc(psci_cpu_on_nr, cpu, __pa(smp_pen),
0);
>> +}
>> +
>>   int __init psci_init(void)
>>   {
>>       struct dt_device_node *psci;
>> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
>> index 2f37612..50513bf 100644
>> --- a/xen/include/asm-arm/psci.h
>> +++ b/xen/include/asm-arm/psci.h
>> @@ -10,6 +10,7 @@
>>   extern int psci_available;
>>
>>   int psci_init(void);
>> +int call_psci_cpu_on(int cpu, void *smp_pen);
>>
>>   /* functions to handle guest PSCI requests */
>>   int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
>
>

On Wed, 2013-12-04 at 13:15 +0100, Andre Przywara wrote:
> On 12/02/2013 04:01 PM, Ian Campbell wrote:
> > On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> >> Currently we unconditionally send SGIs to all cores on SMP
bringup.
> >> PSCI will not need this, so we move this into a function and call
it
> >> explicitly from the platforms that need it. This gets us get rid
of
> >> the empty cpu_up() platform functions in ARM32 and the comment in
> >> there.
> >
> > I don''t think this is quite true -- even on a PSCI system the
kick is
> > required to get past the gate in head.S.
> 
> Right, but this is the responsibility of the PSCI handler in the 
> firmware, right?
Note that I am talking about a gate which is implemented in Xen''s
head.S, not in the firmware. It is Xen''s reponsibility to get the CPU
past that point, which is somewhat independent from the firmware wakeup,
except in reality it is intertwined because they use the same mechanism.

However, I think I was mistaken. In the case of PSCI we are able to wake
up specific targetted CPUs individually and we do so having already
opened the gate in out head.S, so the CPU must necessarily fall through
without waiting. I think this is worth mentioning in the commit log if
you don''t mind.
>  I was under the assumption that the semantics of cpu_on 
> is to start executing code at the given address, whatever this takes 
> internally.
Right, the issue here is a gate which we have subsequent to that
happening.
> Calxeda firmware for instance does the SGI kick.
> 
> >
> > I wonder how this interacts with PSCI implementations which use an SGI
> > themselves internally...
> >
> >> @@ -376,11 +386,6 @@ int __cpu_up(unsigned int cpu)
> >>           return rc;
> >>       }
> >>
> >> -    /* We don''t know the GIC ID of the CPU until it has
woken up, so just signal
> >> -     * everyone and rely on our own smp_up_cpu gate to ensure
only the one we
> >> -     * want gets through. */
> >> -    send_SGI_allbutself(GIC_SGI_EVENT_CHECK);
> >> -
> >
> > So, I was saying in the 00 mail I''m not sure we can get rid
of this
> > altogether.
> 
> Please note that we do not get rid of this, but just move it. ARM64 
> calls it in arm64/smpboot.c,
How does this interact with the sev() In smp_spin_table_cpu_up I wonder.

I think the GIC send should be only for the non-PSCI case here too.
>  ARM32 non-PSCI platforms call this now 
> explicitly by pointing to that function in their platforms/foo.c file.
OK.
> 
> >
> > But I suppose it is the intention that the platform code always has
both
> > its own logic and this SGI kick (possibly coalesced) in such
> > circumstances? Which is probably ok?
> 
> That was my thinking, yes.
> 
> Regards,
> Andre.
> 
> 
> >>       while ( !cpu_online(cpu) )
> >>       {
> >>           cpu_relax();
> >> diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
> >> index 1485cc6..a1de03c 100644
> >> --- a/xen/include/asm-arm/smp.h
> >> +++ b/xen/include/asm-arm/smp.h
> >> @@ -21,6 +21,8 @@ extern int arch_smp_init(void);
> >>   extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
> >>   extern int arch_cpu_up(int cpu);
> >>
> >> +int cpu_up_send_sgi(int cpu);
> >> +
> >>   /* Secondary CPU entry point */
> >>   extern void init_secondary(void);
> >>
> >
> >
>

On Wed, 2013-12-04 at 13:16 +0100, Andre Przywara wrote:
> On 12/02/2013 03:52 PM, Ian Campbell wrote:
> > On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> >> This is a major rework of last week''s ARM PSCI host
support. The code
> >> has been moved into a separate file and the code flow has been
> >> changed substantially (see below for more details).
> >>    ---------
> >>
> >> Xen did not make use of the host provided ARM PSCI (Power State
> >> Coordination Interface [1]) functionality so far, but relied on
> >> platform specific SMP bringup functions.
> >> This series adds support for PSCI on the host by reading the
required
> >> information from the DTB and invoking the appropriate handler when
> >> bringing up each single CPU.
> >> Since PSCI is defined for both ARM32 and ARM64, I added code for
both
> >> architectures, though only ARM32 is tested on Midway and VExpress
> >> (without any PSCI support on the latter, just a regression test).
> >> ARM64 code was compile tested only.
> >>
> >> The ARM32 SMP boot flow is now as following:
> >> The DTB is scanned for a node announcing PSCI support. If that is
> >> available, call the PSCI handler via SMC to bringup the secondary
> >> cores. If no PSCI has been found, call the platform specific
cpu_up()
> >> function.
> >
> > Is this the most useful way round? If both PSCI and a hook are present
> > might it be expected that the hook might want to make the choice to
> > either go manual or call back to PSCI?
> >
> > Perhaps to handle some quirk (aka bug) in the platform which needs
> > something else before/after the PSCI stuff.
> 
> I thought about that too and had it actually that way in the first place.
> My reason to change it was: What happens if platforms get PSCI support?
> Thinking about sunxi or Arndale. If the device tree advertises PSCI, 
> then this is supposed to work. If it doesn''t work, the DTB
shouldn''t
> carry the PSCI node or u-boot & friends have to remove it.
> 
> In general I agree on the "fix" idea, but I''d ask for
that any bug in
> PSCI kicking should be fixed in the PSCI firmware and not in Xen or 
> other kernels. If we are really in need for SMP fixes, we could still 
> put them into smp_init().
> 
> I can easily change it to prefer platform code, of course.
It''s ok, I think I''m convinced by your argument.
> >>   It is now the responsibility of those functions to do the
> >> GIC SGI call to kick the secondary CPUs. For that a function is
now
> >> provided which does this, so three platforms now reference this
> >> generic function instead of coding up an empty one and relying on
the
> >> GIC kick in Xen code later.
> >
> > Note that the existing kick serves two purposes. The first is to wake
up
> > the processor from the firmware on platforms which need that. The
second
> > is to wake up secondaries from the loop in head.S under:
> >          /* Non-boot CPUs wait here until __cpu_up is ready for them
*/
> > where they wait for smp_up_cpu to become them.
> >
> > I should go read the patches to see what you''ve actually done
here.
> 
> But currently there is only one kick per CPU, right? I didn''t
change
> anything in this respect, just moved the code into a function and called 
> that a bit earlier.
I think we''re covering this in the other subthread, so I won''t
repeat
here.

Ian

On Wed, 2013-12-04 at 13:25 +0100, Andre Przywara wrote:
> On 12/02/2013 04:07 PM, Ian Campbell wrote:
> > On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> >> The PSCI handler is invoked via a secure monitor call with the
> >> arguments defined in registers. Copy the function from the
> >> Linux code and adjust it to work on both ARM32 and ARM64.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >> ---
> >>   xen/arch/arm/psci.c        | 28 ++++++++++++++++++++++++++++
> >>   xen/include/asm-arm/psci.h |  1 +
> >>   2 files changed, 29 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> >> index 9ff06cd..cc382be 100644
> >> --- a/xen/arch/arm/psci.c
> >> +++ b/xen/arch/arm/psci.c
> >> @@ -25,8 +25,36 @@
> >>
> >>   int psci_available;
> >>
> >> +#ifdef CONFIG_ARM_32
> >> +#define REG_PREFIX "r"
> >> +#else
> >> +#define REG_PREFIX "x"
> >> +#endif
> >> +
> >> +static noinline int __invoke_psci_fn_smc(u32 function_id, u32
arg0, u32 arg1,
> >> +                                         u32 arg2)
> >
> > I should reread the PSCI spec, but are these not 64-bit on AArch64?
> 
> Indeed. Not for all functions and parameters, but for the start address 
> at least that makes sense ;-)
> Is there a type in Xen which reliably holds a native word? ulong or 
> something?
You can use vaddr_t or register_t depending on the semantics (they are
otherwise the same). Probably register_t here.


Ian.

On 12/04/2013 01:28 PM, Ian Campbell wrote:
> On Wed, 2013-12-04 at 13:15 +0100, Andre Przywara wrote:
>> On 12/02/2013 04:01 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
>>>> Currently we unconditionally send SGIs to all cores on SMP
bringup.
>>>> PSCI will not need this, so we move this into a function and
call it
>>>> explicitly from the platforms that need it. This gets us get
rid of
>>>> the empty cpu_up() platform functions in ARM32 and the comment
in
>>>> there.
>>>
>>> I don''t think this is quite true -- even on a PSCI system
the kick is
>>> required to get past the gate in head.S.
>>
>> Right, but this is the responsibility of the PSCI handler in the
>> firmware, right?
>
> Note that I am talking about a gate which is implemented in Xen''s
> head.S, not in the firmware. It is Xen''s reponsibility to get the
CPU
> past that point, which is somewhat independent from the firmware wakeup,
> except in reality it is intertwined because they use the same mechanism.
>
> However, I think I was mistaken. In the case of PSCI we are able to wake
> up specific targetted CPUs individually and we do so having already
> opened the gate in out head.S, so the CPU must necessarily fall through
> without waiting. I think this is worth mentioning in the commit log if
> you don''t mind.
>
>>   I was under the assumption that the semantics of cpu_on
>> is to start executing code at the given address, whatever this takes
>> internally.
>
> Right, the issue here is a gate which we have subsequent to that
> happening.
>
>> Calxeda firmware for instance does the SGI kick.
>>
>>>
>>> I wonder how this interacts with PSCI implementations which use an
SGI
>>> themselves internally...
>>>
>>>> @@ -376,11 +386,6 @@ int __cpu_up(unsigned int cpu)
>>>>            return rc;
>>>>        }
>>>>
>>>> -    /* We don''t know the GIC ID of the CPU until it
has woken up, so just signal
>>>> -     * everyone and rely on our own smp_up_cpu gate to ensure
only the one we
>>>> -     * want gets through. */
>>>> -    send_SGI_allbutself(GIC_SGI_EVENT_CHECK);
>>>> -
>>>
>>> So, I was saying in the 00 mail I''m not sure we can get
rid of this
>>> altogether.
>>
>> Please note that we do not get rid of this, but just move it. ARM64
>> calls it in arm64/smpboot.c,
>
> How does this interact with the sev() In smp_spin_table_cpu_up I wonder.
Me, too ;-)
The original code does the sev() in arm/arm64/smpboot.c, then returns to 
the caller in arm/smpboot.c, which does the GIC kick. I was already 
wondering if that is correct, I guess you could answer this much better.
> I think the GIC send should be only for the non-PSCI case here too.
It is. The arm64 code has the GIC kick moved into the spin_table 
function only. PSCI is a different beast, not using the GIC. Also arm32 
does it only in platform specific functions, which don''t get called
when
PSCI is available.

Regards,
Andre.
>
>>   ARM32 non-PSCI platforms call this now
>> explicitly by pointing to that function in their platforms/foo.c file.
>
> OK.
>
>>
>>>
>>> But I suppose it is the intention that the platform code always has
both
>>> its own logic and this SGI kick (possibly coalesced) in such
>>> circumstances? Which is probably ok?
>>
>> That was my thinking, yes.
>>
>> Regards,
>> Andre.
>>
>>
>>>>        while ( !cpu_online(cpu) )
>>>>        {
>>>>            cpu_relax();
>>>> diff --git a/xen/include/asm-arm/smp.h
b/xen/include/asm-arm/smp.h
>>>> index 1485cc6..a1de03c 100644
>>>> --- a/xen/include/asm-arm/smp.h
>>>> +++ b/xen/include/asm-arm/smp.h
>>>> @@ -21,6 +21,8 @@ extern int arch_smp_init(void);
>>>>    extern int arch_cpu_init(int cpu, struct dt_device_node
*dn);
>>>>    extern int arch_cpu_up(int cpu);
>>>>
>>>> +int cpu_up_send_sgi(int cpu);
>>>> +
>>>>    /* Secondary CPU entry point */
>>>>    extern void init_secondary(void);
>>>>
>>>
>>>
>>
>
>

On Wed, 2013-12-04 at 13:33 +0100, Andre Przywara wrote:
> On 12/04/2013 01:28 PM, Ian Campbell wrote:
> > On Wed, 2013-12-04 at 13:15 +0100, Andre Przywara wrote:
> >> On 12/02/2013 04:01 PM, Ian Campbell wrote:
> >>> On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> >>>> Currently we unconditionally send SGIs to all cores on SMP
bringup.
> >>>> PSCI will not need this, so we move this into a function
and call it
> >>>> explicitly from the platforms that need it. This gets us
get rid of
> >>>> the empty cpu_up() platform functions in ARM32 and the
comment in
> >>>> there.
> >>>
> >>> I don''t think this is quite true -- even on a PSCI
system the kick is
> >>> required to get past the gate in head.S.
> >>
> >> Right, but this is the responsibility of the PSCI handler in the
> >> firmware, right?
> >
> > Note that I am talking about a gate which is implemented in
Xen''s
> > head.S, not in the firmware. It is Xen''s reponsibility to get
the CPU
> > past that point, which is somewhat independent from the firmware
wakeup,
> > except in reality it is intertwined because they use the same
mechanism.
> >
> > However, I think I was mistaken. In the case of PSCI we are able to
wake
> > up specific targetted CPUs individually and we do so having already
> > opened the gate in out head.S, so the CPU must necessarily fall
through
> > without waiting. I think this is worth mentioning in the commit log if
> > you don''t mind.
> >
> >>   I was under the assumption that the semantics of cpu_on
> >> is to start executing code at the given address, whatever this
takes
> >> internally.
> >
> > Right, the issue here is a gate which we have subsequent to that
> > happening.
> >
> >> Calxeda firmware for instance does the SGI kick.
> >>
> >>>
> >>> I wonder how this interacts with PSCI implementations which
use an SGI
> >>> themselves internally...
> >>>
> >>>> @@ -376,11 +386,6 @@ int __cpu_up(unsigned int cpu)
> >>>>            return rc;
> >>>>        }
> >>>>
> >>>> -    /* We don''t know the GIC ID of the CPU until
it has woken up, so just signal
> >>>> -     * everyone and rely on our own smp_up_cpu gate to
ensure only the one we
> >>>> -     * want gets through. */
> >>>> -    send_SGI_allbutself(GIC_SGI_EVENT_CHECK);
> >>>> -
> >>>
> >>> So, I was saying in the 00 mail I''m not sure we can
get rid of this
> >>> altogether.
> >>
> >> Please note that we do not get rid of this, but just move it.
ARM64
> >> calls it in arm64/smpboot.c,
> >
> > How does this interact with the sev() In smp_spin_table_cpu_up I
wonder.
> 
> Me, too ;-)
> The original code does the sev() in arm/arm64/smpboot.c, then returns to 
> the caller in arm/smpboot.c, which does the GIC kick. I was already 
> wondering if that is correct, I guess you could answer this much better.
I think the firmware uses wfe while the Xen gate uses wfi so infact both
are needed.
> > I think the GIC send should be only for the non-PSCI case here too.
> 
> It is. The arm64 code has the GIC kick moved into the spin_table 
> function only. PSCI is a different beast, not using the GIC. Also arm32 
> does it only in platform specific functions, which don''t get
called when
> PSCI is available.
Great, I should have checked the code before replying.

Thanks,
Ian.
> 
> Regards,
> Andre.
> 
> >
> >>   ARM32 non-PSCI platforms call this now
> >> explicitly by pointing to that function in their platforms/foo.c
file.
> >
> > OK.
> >
> >>
> >>>
> >>> But I suppose it is the intention that the platform code
always has both
> >>> its own logic and this SGI kick (possibly coalesced) in such
> >>> circumstances? Which is probably ok?
> >>
> >> That was my thinking, yes.
> >>
> >> Regards,
> >> Andre.
> >>
> >>
> >>>>        while ( !cpu_online(cpu) )
> >>>>        {
> >>>>            cpu_relax();
> >>>> diff --git a/xen/include/asm-arm/smp.h
b/xen/include/asm-arm/smp.h
> >>>> index 1485cc6..a1de03c 100644
> >>>> --- a/xen/include/asm-arm/smp.h
> >>>> +++ b/xen/include/asm-arm/smp.h
> >>>> @@ -21,6 +21,8 @@ extern int arch_smp_init(void);
> >>>>    extern int arch_cpu_init(int cpu, struct dt_device_node
*dn);
> >>>>    extern int arch_cpu_up(int cpu);
> >>>>
> >>>> +int cpu_up_send_sgi(int cpu);
> >>>> +
> >>>>    /* Secondary CPU entry point */
> >>>>    extern void init_secondary(void);
> >>>>
> >>>
> >>>
> >>
> >
> >
>

On 12/02/2013 04:05 PM, Ian Campbell wrote:
> On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
>> +int __init psci_init(void)
>> +{
> [...]
>> +    /* Since Xen runs in HYP all of the time, it does not make sense
to
>> +     * let it call into HYP for PSCI handling, since the handler
won''t
>> +     * just be there. So bail out with an error if "smc" is
not used.
>
> s/won''t just/just won''t/.
>
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index 52cef30..3a9be90 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> [...]
>> @@ -105,6 +106,12 @@ void __init smp_init_cpus(void)
>>       bool_t bootcpu_valid = 0;
>>       int rc;
>>
>> +    if ( psci_init() == 0 )
>> +    {
>> +        printk(XENLOG_INFO "Using PSCI for SMP bringup\n");
>> +        psci_available = 1;
>
> IMHO this log + flag twiddling belong as the last act of psci init.
Well, it is already, at least kind of.
psci_available means that the DTB contains a valid and sane PSCI node, 
so Xen should use PSCI as the SMP bringup method (that''s why _available
and not _enabled).
And the message was to inform the user that PSCI is _going to be used_ 
for SMP bringup. I should change the wording to be more clear here.
On Linux this kind of information gave me valuable hints on debugging 
SMP issues in the past.

Thanks
>
> Other than those two things this patch looks good. If you change at
> least the second one then:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> (I leave the first one up to you in case I''ve simply misparsed it)
>
> Ian.
>
>

On Wed, 2013-12-04 at 13:37 +0100, Andre Przywara wrote:
> On 12/02/2013 04:05 PM, Ian Campbell wrote:
> > On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
> >> +int __init psci_init(void)
> >> +{
> > [...]
> >> +    /* Since Xen runs in HYP all of the time, it does not make
sense to
> >> +     * let it call into HYP for PSCI handling, since the handler
won''t
> >> +     * just be there. So bail out with an error if
"smc" is not used.
> >
> > s/won''t just/just won''t/.
> >
> >> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> >> index 52cef30..3a9be90 100644
> >> --- a/xen/arch/arm/smpboot.c
> >> +++ b/xen/arch/arm/smpboot.c
> >> [...]
> >> @@ -105,6 +106,12 @@ void __init smp_init_cpus(void)
> >>       bool_t bootcpu_valid = 0;
> >>       int rc;
> >>
> >> +    if ( psci_init() == 0 )
> >> +    {
> >> +        printk(XENLOG_INFO "Using PSCI for SMP
bringup\n");
> >> +        psci_available = 1;
> >
> > IMHO this log + flag twiddling belong as the last act of psci init.
> 
> Well, it is already, at least kind of.
I mean that it should literally be within that function, not in the
caller.
> psci_available means that the DTB contains a valid and sane PSCI node, 
> so Xen should use PSCI as the SMP bringup method (that''s why
_available
> and not _enabled).
> And the message was to inform the user that PSCI is _going to be used_ 
> for SMP bringup. I should change the wording to be more clear here.
> On Linux this kind of information gave me valuable hints on debugging 
> SMP issues in the past.
> 
> Thanks
> 
> >
> > Other than those two things this patch looks good. If you change at
> > least the second one then:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > (I leave the first one up to you in case I''ve simply
misparsed it)
> >
> > Ian.
> >
> >
>

On 12/04/2013 01:41 PM, Ian Campbell wrote:
> On Wed, 2013-12-04 at 13:37 +0100, Andre Przywara wrote:
>> On 12/02/2013 04:05 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
>>>> +int __init psci_init(void)
>>>> +{
>>> [...]
>>>> +    /* Since Xen runs in HYP all of the time, it does not make
sense to
>>>> +     * let it call into HYP for PSCI handling, since the
handler won''t
>>>> +     * just be there. So bail out with an error if
"smc" is not used.
>>>
>>> s/won''t just/just won''t/.
>>>
>>>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>>>> index 52cef30..3a9be90 100644
>>>> --- a/xen/arch/arm/smpboot.c
>>>> +++ b/xen/arch/arm/smpboot.c
>>>> [...]
>>>> @@ -105,6 +106,12 @@ void __init smp_init_cpus(void)
>>>>        bool_t bootcpu_valid = 0;
>>>>        int rc;
>>>>
>>>> +    if ( psci_init() == 0 )
>>>> +    {
>>>> +        printk(XENLOG_INFO "Using PSCI for SMP
bringup\n");
>>>> +        psci_available = 1;
>>>
>>> IMHO this log + flag twiddling belong as the last act of psci init.
>>
>> Well, it is already, at least kind of.
>
> I mean that it should literally be within that function, not in the
> caller.
Oh, sure. Will fix this. Sorry for the noise ;-)

Regards,
Andre.
>
>> psci_available means that the DTB contains a valid and sane PSCI node,
>> so Xen should use PSCI as the SMP bringup method (that''s why
_available
>> and not _enabled).
>> And the message was to inform the user that PSCI is _going to be used_
>> for SMP bringup. I should change the wording to be more clear here.
>> On Linux this kind of information gave me valuable hints on debugging
>> SMP issues in the past.
>>
>> Thanks
>>
>>>
>>> Other than those two things this patch looks good. If you change at
>>> least the second one then:
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> (I leave the first one up to you in case I''ve simply
misparsed it)
>>>
>>> Ian.
>>>
>>>
>>
>
>