Xen did not make use of the host provided ARM PSCI (Power State Coordination Interface) 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 put the code in a file shared by both. The ARM32 code was tested on Midway, but the ARM64 code was compile tested only. This approach seems to be the least intrusive, but one could also use more of the current ARM64 code by copying the PSCI/spin-table distinction code to a shared file and use that from both architectures. However that seems more complicated. Please take a look and complain ;-) Signed-off-by: Andre Przywara <andre.przywara@linaro.org> Andre Przywara (4): arm: parse PSCI node from the host device-tree arm: add a function to invoke the PSCI handler and use it arm: dont give up on EAGAIN if PSCI is defined arm64: defer CPU initialization on ARM64 if PSCI is present xen/arch/arm/arm32/smpboot.c | 1 - xen/arch/arm/arm64/smpboot.c | 7 +++- xen/arch/arm/smpboot.c | 89 +++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 88 insertions(+), 9 deletions(-) -- 1.7.12.1
Andre Przywara
2013-Nov-25 12:02 UTC
[PATCH 1/4] arm: parse PSCI node from the host device-tree
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 callling
method, as it does not make much sense currently under Xen.
Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
xen/arch/arm/smpboot.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 6c90fa6..97bd414 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -89,6 +89,44 @@ smp_clear_cpu_maps (void)
cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK;
}
+uint32_t psci_host_cpu_on_nr;
+
+static int __init psci_host_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 1;
+
+ ret = dt_property_read_string(psci, "method", &prop_str);
+ if ( ret )
+ {
+ printk("/psci node does not provide a method (%d)\n", ret);
+ return 2;
+ }
+
+ /* 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 3;
+ }
+
+ if ( !dt_property_read_u32(psci, "cpu_on",
&psci_host_cpu_on_nr) )
+ {
+ printk("/psci node is missing the \"cpu_on\"
property\n");
+ return 4;
+ }
+
+ return 0;
+}
+
/* Parse the device tree and build the logical map array containing
* MPIDR values related to logical cpus
* Code base on Linux arch/arm/kernel/devtree.c
@@ -107,6 +145,11 @@ void __init smp_init_cpus(void)
bool_t bootcpu_valid = 0;
int rc;
+ if ( psci_host_init() == 0 )
+ {
+ printk(XENLOG_INFO "Using PSCI for SMP bringup\n");
+ }
+
if ( (rc = arch_smp_init()) < 0 )
{
printk(XENLOG_WARNING "SMP init failed (%d)\n"
--
1.7.12.1
Andre Przywara
2013-Nov-25 12:02 UTC
[PATCH 2/4] arm: add a function to invoke the PSCI handler and use it
The PSCI handler is invoked via a secure monitor call with the
arguments defined in registers [1]. Copy the function from the
Linux code and adjust it to work on both ARM32 and ARM64.
Later use that function instead of the generic GIC SEV kick to
actually bring up the secondary CPUs.
[1]:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0022b/index.html
Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
xen/arch/arm/arm32/smpboot.c | 1 -
xen/arch/arm/smpboot.c | 39 +++++++++++++++++++++++++++++++++++----
2 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c
index 88fe8fb..fcf653f 100644
--- a/xen/arch/arm/arm32/smpboot.c
+++ b/xen/arch/arm/arm32/smpboot.c
@@ -10,7 +10,6 @@ int __init arch_smp_init(void)
int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
{
- /* TODO handle PSCI init */
return 0;
}
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 97bd414..44326d8 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -89,6 +89,29 @@ smp_clear_cpu_maps (void)
cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK;
}
+#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
+
uint32_t psci_host_cpu_on_nr;
static int __init psci_host_init(void)
@@ -393,10 +416,18 @@ 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);
+ if ( psci_host_cpu_on_nr != 0 )
+ {
+ /* If the DTB provided a PSCI node, use this for kicking the CPUs */
+ __invoke_psci_fn_smc(
+ psci_host_cpu_on_nr, cpu, __pa(init_secondary), 0);
+ } else
+ {
+ /* 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) )
{
--
1.7.12.1
Andre Przywara
2013-Nov-25 12:02 UTC
[PATCH 3/4] arm: dont give up on EAGAIN if PSCI is defined
Currently the platforms define an empty, zero-returning cpu_up
function to state that they don''t need any special treatment beside
the GIC SEV kick to come up.
Allow platforms which only provide PSCI to not define a platform
specific cpu_up() function at all. For this we need to handle the
EAGAIN error code that the platform returns in this case and ignore
that if a PSCI node was found in the DTB.
Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
xen/arch/arm/smpboot.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 44326d8..14774c5 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -412,8 +412,11 @@ int __cpu_up(unsigned int cpu)
if ( rc < 0 )
{
- printk("Failed to bring up CPU%d\n", cpu);
- return rc;
+ if ( rc != -EAGAIN || psci_host_cpu_on_nr == 0 )
+ {
+ printk("Failed to bring up CPU%d\n", cpu);
+ return rc;
+ }
}
if ( psci_host_cpu_on_nr != 0 )
--
1.7.12.1
Andre Przywara
2013-Nov-25 12:02 UTC
[PATCH 4/4] arm64: defer CPU initialization on ARM64 if PSCI is present
Since PSCI is now handled in ARM generic smpboot code, we can remove
the PSCI todo from the ARM64 specific code and defer the actual PSCI
handling.
Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
xen/arch/arm/arm64/smpboot.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 8239590..715c44e 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -61,8 +61,11 @@ 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
+ else if ( !strcmp(enable_method, "psci") )
+ {
+ /* PSCI code is handled somewhere else */
+ return -EAGAIN;
+ } else
{
printk("CPU%d has unknown enable method \"%s\"\n",
cpu, enable_method);
return -EINVAL;
--
1.7.12.1
On Mon, Nov 25, 2013 at 12:02 PM, Andre Przywara <andre.przywara@linaro.org> wrote:> Xen did not make use of the host provided ARM PSCI (Power State > Coordination Interface) 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 put the code in a > file shared by both. > The ARM32 code was tested on Midway, but the ARM64 code was compile > tested only. > > This approach seems to be the least intrusive, but one could also use > more of the current ARM64 code by copying the PSCI/spin-table > distinction code to a shared file and use that from both > architectures. However that seems more complicated. > > Please take a look and complain ;-) > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org>Ian, do you agree that this is too late for 4.4? -George
On Mon, 2013-11-25 at 13:00 +0000, George Dunlap wrote:> On Mon, Nov 25, 2013 at 12:02 PM, Andre Przywara > <andre.przywara@linaro.org> wrote: > > Xen did not make use of the host provided ARM PSCI (Power State > > Coordination Interface) 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 put the code in a > > file shared by both. > > The ARM32 code was tested on Midway, but the ARM64 code was compile > > tested only. > > > > This approach seems to be the least intrusive, but one could also use > > more of the current ARM64 code by copying the PSCI/spin-table > > distinction code to a shared file and use that from both > > architectures. However that seems more complicated. > > > > Please take a look and complain ;-) > > > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > > Ian, do you agree that this is too late for 4.4?I''m in two minds. On the one hand none of the existing platforms currently require this functionality, so it has clearly not been necessary up to now. On the other hand it plays into the strategy of allowing people to trivially support their platform, and since it is a standard way to do power control on ARM (albeit quite new and so far uptake is not huge) I think it is expected that many new platforms will use it. Of our current platforms Midway can optionally use PSCI (we have "native" code at the minute) and sunxi is going to need it whenever SMP is enabled (patches to u-boot are circulating now). I''m inclined towards punting on this for 4.4.0 but be open to the idea of adding it in 4.4.1 if it turns out to be something that people are needing in practice.. An alternative could be requiring for 4.4 that the platform code explicitly call into/request PSCI for 4.4 and only move to automatically using it in the absence of the platform code saying otherwise for 4.5. This has the advantage of being zero risk, but the downside of not being very well tested (we could enable it for Midway, with the attendant increase in risk). Ian.
On 11/25/2013 03:03 PM, Ian Campbell wrote:> On Mon, 2013-11-25 at 13:00 +0000, George Dunlap wrote: >> On Mon, Nov 25, 2013 at 12:02 PM, Andre Przywara >> <andre.przywara@linaro.org> wrote: >>> Xen did not make use of the host provided ARM PSCI (Power State >>> Coordination Interface) 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 put the code in a >>> file shared by both. >>> The ARM32 code was tested on Midway, but the ARM64 code was compile >>> tested only. >>> >>> This approach seems to be the least intrusive, but one could also use >>> more of the current ARM64 code by copying the PSCI/spin-table >>> distinction code to a shared file and use that from both >>> architectures. However that seems more complicated. >>> >>> Please take a look and complain ;-) >>> >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> >> Ian, do you agree that this is too late for 4.4? > > I''m in two minds. On the one hand none of the existing platforms > currently require this functionality, so it has clearly not been > necessary up to now. > > On the other hand it plays into the strategy of allowing people to > trivially support their platform, and since it is a standard way to do > power control on ARM (albeit quite new and so far uptake is not huge) I > think it is expected that many new platforms will use it. > > Of our current platforms Midway can optionally use PSCI (we have > "native" code at the minute)but which is not upstream yet, right? So if you are considering dropping PSCI for 4.4, I''d like to know so that I can ack Julien''s "native" SMP patch. I hope at least this patch can make it for 4.4?> and sunxi is going to need it whenever SMP > is enabled (patches to u-boot are circulating now).That is a good point. We would get sunxi SMP support basically for free, also the timing for this is independent of any Xen release cycle.> I''m inclined towards punting on this for 4.4.0 but be open to the idea > of adding it in 4.4.1 if it turns out to be something that people are > needing in practice.4.4.1 sounds OK for me. But it would be nice to have the native SMP support then in 4.4.0.> An alternative could be requiring for 4.4 that the platform code > explicitly call into/request PSCI for 4.4 and only move to automatically > using it in the absence of the platform code saying otherwise for 4.5.So you are thinking about a change in the priorities? The Linux kernel prefers PSCI over a native method, which is how I modeled the Xen patch also. This has the advantage of having control in the DTB, so if PSCI fails in Xen, one could do "fdt rm /psci" in u-boot to get the old behavior back.> This has the advantage of being zero risk, but the downside of not being > very well tested (we could enable it for Midway, with the attendant > increase in risk).So are you concerned about one of the existing platforms breaking SMP as soon as it gets PSCI support? One could change the patch to only use PSCI if platform_cpu_up() does _not_ return an explicit "ignore PSCI" value, if that helps. Regards, Andre.
On Mon, 2013-11-25 at 15:21 +0100, Andre Przywara wrote:> On 11/25/2013 03:03 PM, Ian Campbell wrote: > > On Mon, 2013-11-25 at 13:00 +0000, George Dunlap wrote: > >> On Mon, Nov 25, 2013 at 12:02 PM, Andre Przywara > >> <andre.przywara@linaro.org> wrote: > >>> Xen did not make use of the host provided ARM PSCI (Power State > >>> Coordination Interface) 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 put the code in a > >>> file shared by both. > >>> The ARM32 code was tested on Midway, but the ARM64 code was compile > >>> tested only. > >>> > >>> This approach seems to be the least intrusive, but one could also use > >>> more of the current ARM64 code by copying the PSCI/spin-table > >>> distinction code to a shared file and use that from both > >>> architectures. However that seems more complicated. > >>> > >>> Please take a look and complain ;-) > >>> > >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > >> > >> Ian, do you agree that this is too late for 4.4? > > > > I''m in two minds. On the one hand none of the existing platforms > > currently require this functionality, so it has clearly not been > > necessary up to now. > > > > On the other hand it plays into the strategy of allowing people to > > trivially support their platform, and since it is a standard way to do > > power control on ARM (albeit quite new and so far uptake is not huge) I > > think it is expected that many new platforms will use it. > > > > Of our current platforms Midway can optionally use PSCI (we have > > "native" code at the minute) > > but which is not upstream yet, right?Oh right, I forgot it was still waiting for an Ack from you and thought I''d committed it when I had not.> So if you are considering dropping PSCI for 4.4, I''d like to know so > that I can ack Julien''s "native" SMP patch. > I hope at least this patch can make it for 4.4?Yes, one or the other should definitely go in for 4.4. It changes the argument for the PSCI stuff a bit too, since we can now enable midway and make it easier for other platforms at the same time. [...]> > An alternative could be requiring for 4.4 that the platform code > > explicitly call into/request PSCI for 4.4 and only move to automatically > > using it in the absence of the platform code saying otherwise for 4.5. > > So you are thinking about a change in the priorities?I was only suggesting as a way to mitigate risk for 4.4 -- long term we should certainly do as Linux does and prefer PSCI. (I confess I wasn''t sure how this manifests in Linux, if its at odds with what I wrote then ...oops)> The Linux kernel > prefers PSCI over a native method, which is how I modeled the Xen patch > also. This has the advantage of having control in the DTB, so if PSCI > fails in Xen, one could do "fdt rm /psci" in u-boot to get the old > behavior back. > > > This has the advantage of being zero risk, but the downside of not being > > very well tested (we could enable it for Midway, with the attendant > > increase in risk). > > So are you concerned about one of the existing platforms breaking SMP as > soon as it gets PSCI support? One could change the patch to only use > PSCI if platform_cpu_up() does _not_ return an explicit "ignore PSCI" > value, if that helps.I''m addressing George''s concerns as release manager about the risk of taking any sort of PSCI patches at this stage. Ian.
On 11/25/2013 03:50 PM, Ian Campbell wrote:> On Mon, 2013-11-25 at 15:21 +0100, Andre Przywara wrote: >> On 11/25/2013 03:03 PM, Ian Campbell wrote: >>> On Mon, 2013-11-25 at 13:00 +0000, George Dunlap wrote: >>>> On Mon, Nov 25, 2013 at 12:02 PM, Andre Przywara >>>> <andre.przywara@linaro.org> wrote: >>>>> Xen did not make use of the host provided ARM PSCI (Power State >>>>> Coordination Interface) 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 put the code in a >>>>> file shared by both. >>>>> The ARM32 code was tested on Midway, but the ARM64 code was compile >>>>> tested only. >>>>> >>>>> This approach seems to be the least intrusive, but one could also use >>>>> more of the current ARM64 code by copying the PSCI/spin-table >>>>> distinction code to a shared file and use that from both >>>>> architectures. However that seems more complicated. >>>>> >>>>> Please take a look and complain ;-) >>>>> >>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>> >>>> Ian, do you agree that this is too late for 4.4? >>> >>> I''m in two minds. On the one hand none of the existing platforms >>> currently require this functionality, so it has clearly not been >>> necessary up to now. >>> >>> On the other hand it plays into the strategy of allowing people to >>> trivially support their platform, and since it is a standard way to do >>> power control on ARM (albeit quite new and so far uptake is not huge) I >>> think it is expected that many new platforms will use it. >>> >>> Of our current platforms Midway can optionally use PSCI (we have >>> "native" code at the minute) >> >> but which is not upstream yet, right? > > Oh right, I forgot it was still waiting for an Ack from you and thought > I''d committed it when I had not.I deliberately held back my ACK: on this one to give PSCI a chance, since it turned out to be easier than I thought. Technically I am OK with Julien''s patch, so I can ACK it as well if you like.>> So if you are considering dropping PSCI for 4.4, I''d like to know so >> that I can ack Julien''s "native" SMP patch. >> I hope at least this patch can make it for 4.4? > > Yes, one or the other should definitely go in for 4.4. It changes the > argument for the PSCI stuff a bit too, since we can now enable midway > and make it easier for other platforms at the same time.That was my thinking. But I see both George''s and your point with a release manager''s hat on, so I am OK with whatever you decide. Thanks for caring! Andre> [...] >>> An alternative could be requiring for 4.4 that the platform code >>> explicitly call into/request PSCI for 4.4 and only move to automatically >>> using it in the absence of the platform code saying otherwise for 4.5. >> >> So you are thinking about a change in the priorities? > > I was only suggesting as a way to mitigate risk for 4.4 -- long term we > should certainly do as Linux does and prefer PSCI. (I confess I wasn''t > sure how this manifests in Linux, if its at odds with what I wrote > then ...oops) >> The Linux kernel >> prefers PSCI over a native method, which is how I modeled the Xen patch >> also. This has the advantage of having control in the DTB, so if PSCI >> fails in Xen, one could do "fdt rm /psci" in u-boot to get the old >> behavior back. >> >>> This has the advantage of being zero risk, but the downside of not being >>> very well tested (we could enable it for Midway, with the attendant >>> increase in risk). >> >> So are you concerned about one of the existing platforms breaking SMP as >> soon as it gets PSCI support? One could change the patch to only use >> PSCI if platform_cpu_up() does _not_ return an explicit "ignore PSCI" >> value, if that helps. > > I''m addressing George''s concerns as release manager about the risk of > taking any sort of PSCI patches at this stage. > > Ian. >
On 11/25/2013 02:03 PM, Ian Campbell wrote:> On Mon, 2013-11-25 at 13:00 +0000, George Dunlap wrote: >> On Mon, Nov 25, 2013 at 12:02 PM, Andre Przywara >> <andre.przywara@linaro.org> wrote: >>> Xen did not make use of the host provided ARM PSCI (Power State >>> Coordination Interface) 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 put the code in a >>> file shared by both. >>> The ARM32 code was tested on Midway, but the ARM64 code was compile >>> tested only. >>> >>> This approach seems to be the least intrusive, but one could also use >>> more of the current ARM64 code by copying the PSCI/spin-table >>> distinction code to a shared file and use that from both >>> architectures. However that seems more complicated. >>> >>> Please take a look and complain ;-) >>> >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> Ian, do you agree that this is too late for 4.4? > I''m in two minds. On the one hand none of the existing platforms > currently require this functionality, so it has clearly not been > necessary up to now. > > On the other hand it plays into the strategy of allowing people to > trivially support their platform, and since it is a standard way to do > power control on ARM (albeit quite new and so far uptake is not huge) I > think it is expected that many new platforms will use it. > > Of our current platforms Midway can optionally use PSCI (we have > "native" code at the minute) and sunxi is going to need it whenever SMP > is enabled (patches to u-boot are circulating now). > > I''m inclined towards punting on this for 4.4.0 but be open to the idea > of adding it in 4.4.1 if it turns out to be something that people are > needing in practice..At some point every cost/benefits analysis comes down to a judgement call; in cases where the release coordinator doesn''t have the experience to make the call themselves, their job should be to help set the criteria, ask questions, and clarify the thinking. Both arguments -- "We should risk including this because it will enable other platforms, in particular sunxi", and "We should wait until 4.4.1", sound reasonable to me. So I think I''ll have to leave it up to you to judge which is a better bet at this point. :-) -George
On Mon, 2013-11-25 at 16:35 +0000, George Dunlap wrote:> At some point every cost/benefits analysis comes down to a judgement > call; in cases where the release coordinator doesn''t have the experience > to make the call themselves, their job should be to help set the > criteria, ask questions, and clarify the thinking. > > Both arguments -- "We should risk including this because it will enable > other platforms, in particular sunxi", and "We should wait until 4.4.1", > sound reasonable to me. So I think I''ll have to leave it up to you to > judge which is a better bet at this point. :-)I think given that I was mistaken about the native Midway SMP support being in we should take either that patch or this PSCI series and since PSCI also enables other platforms I think it should be this one. Ian.
On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote:> Xen did not make use of the host provided ARM PSCI (Power State > Coordination Interface) 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 put the code in a > file shared by both. > The ARM32 code was tested on Midway, but the ARM64 code was compile > tested only. > > This approach seems to be the least intrusive, but one could also use > more of the current ARM64 code by copying the PSCI/spin-table > distinction code to a shared file and use that from both > architectures. However that seems more complicated.I don''t think that is needed (since armv7 spintable vs armv8 spintable mechanisms are a bit different). But I would like to see the psci code in a separate psci.c. It''s not much code right now but it will likely grow. I''ll comment a bit more on the individual patches shortly. Thanks. Ian.> > Please take a look and complain ;-) > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > > Andre Przywara (4): > arm: parse PSCI node from the host device-tree > arm: add a function to invoke the PSCI handler and use it > arm: dont give up on EAGAIN if PSCI is defined > arm64: defer CPU initialization on ARM64 if PSCI is present > > xen/arch/arm/arm32/smpboot.c | 1 - > xen/arch/arm/arm64/smpboot.c | 7 +++- > xen/arch/arm/smpboot.c | 89 +++++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 88 insertions(+), 9 deletions(-) >
Ian Campbell
2013-Nov-26 11:12 UTC
Re: [PATCH 1/4] arm: parse PSCI node from the host device-tree
On Mon, 2013-11-25 at 13:02 +0100, 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 calllingToo many l''s in callling.> method, as it does not make much sense currently under Xen. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/smpboot.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index 6c90fa6..97bd414 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -89,6 +89,44 @@ smp_clear_cpu_maps (void) > cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK; > } > > +uint32_t psci_host_cpu_on_nr;Can we make this static and have a global "psci_enable" flag to use elsewhere please.> +static int __init psci_host_init(void)s/host// I think? Generally we have e.g. gic_init for the host and vgic_init for the guest.> +{ > + struct dt_device_node *psci; > + int ret; > + const char *prop_str; > + > + psci = dt_find_compatible_node(NULL, NULL, "arm,psci"); > + if ( !psci ) > + return 1;You''ve made up some return codes here? Please can you return -Efoo (whichever one(s) seem appropriate). If you need to distinguish PSCI not present from PSCI present but buggy then perhaps use ENODEV or something similarly unique+relevant for the former case?> + ret = dt_property_read_string(psci, "method", &prop_str); > + if ( ret ) > + { > + printk("/psci node does not provide a method (%d)\n", ret); > + return 2; > + } > + > + /* 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 3; > + } > + > + if ( !dt_property_read_u32(psci, "cpu_on", &psci_host_cpu_on_nr) ) > + { > + printk("/psci node is missing the \"cpu_on\" property\n"); > + return 4; > + }http://www.spinics.net/lists/devicetree/msg05348.html updates the bindings for PSCI 0.2. I suppose Midway must only support 0.1? I''d be OK with only supporting 0.1 right now, but it would be useful to comment either in the code or in the commit message. (nb: I''m not sure of v4 was the final version of that series)> + > + return 0; > +} > + > /* Parse the device tree and build the logical map array containing > * MPIDR values related to logical cpus > * Code base on Linux arch/arm/kernel/devtree.c > @@ -107,6 +145,11 @@ void __init smp_init_cpus(void) > bool_t bootcpu_valid = 0; > int rc; > > + if ( psci_host_init() == 0 ) > + { > + printk(XENLOG_INFO "Using PSCI for SMP bringup\n"); > + } > + > if ( (rc = arch_smp_init()) < 0 )arch_smp_init is empty on both platforms. arm32 has a comment "TODO: PSCI" ;-) I think we can nuke this function while we are here, since it''s only purpose was as a PSCI placehoder. Ian.
Ian Campbell
2013-Nov-26 11:18 UTC
Re: [PATCH 2/4] arm: add a function to invoke the PSCI handler and use it
On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote:> The PSCI handler is invoked via a secure monitor call with the > arguments defined in registers [1]. Copy the function from the > Linux code and adjust it to work on both ARM32 and ARM64. > Later use that function instead of the generic GIC SEV kick to > actually bring up the secondary CPUs. > > [1]: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0022b/index.html > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/arm32/smpboot.c | 1 - > xen/arch/arm/smpboot.c | 39 +++++++++++++++++++++++++++++++++++---- > 2 files changed, 35 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c > index 88fe8fb..fcf653f 100644 > --- a/xen/arch/arm/arm32/smpboot.c > +++ b/xen/arch/arm/arm32/smpboot.c > @@ -10,7 +10,6 @@ int __init arch_smp_init(void) > > int __init arch_cpu_init(int cpu, struct dt_device_node *dn) > { > - /* TODO handle PSCI init */ > return 0; > } > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index 97bd414..44326d8 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -89,6 +89,29 @@ smp_clear_cpu_maps (void) > cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK; > } > > +#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)Please can you put this in psci.c and provide wrappers e.g. psci_cpu_up(). THese can return some suitable errno if PSCI isn''t enabled. Or we could add a psci_enabled() call Why noinline?> +{ > + 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 > + > uint32_t psci_host_cpu_on_nr; > > static int __init psci_host_init(void) > @@ -393,10 +416,18 @@ 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); > + if ( psci_host_cpu_on_nr != 0 )We could go for a set of smp function pointers initialised by psci_init() but I think for now if (psci_cpu_up(cpu, __pa(...)) < 0) { /* No PSCI, send a manual ... blah. We don''t know the GIC * ID, etc etc */ send_SGI...(...) } would be OK? Or could use a psci_enable() call, I have slightly less preference for that.> + { > + /* If the DTB provided a PSCI node, use this for kicking the CPUs */ > + __invoke_psci_fn_smc( > + psci_host_cpu_on_nr, cpu, __pa(init_secondary), 0); > + } else > + { > + /* 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) ) > {
Ian Campbell
2013-Nov-26 11:20 UTC
Re: [PATCH 3/4] arm: dont give up on EAGAIN if PSCI is defined
On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote:> Currently the platforms define an empty, zero-returning cpu_up > function to state that they don''t need any special treatment beside > the GIC SEV kick to come up. > Allow platforms which only provide PSCI to not define a platform > specific cpu_up() function at all. For this we need to handle the > EAGAIN error code that the platform returns in this case and ignore > that if a PSCI node was found in the DTB.I think we should only call the arch hook if PSCI is not enabled. Either that or we should only try PSCI if there is no arch hook. I think probably the former. I think the call to arch_cpu_up can be moved inside the PSCI conditional which follows in the patch context e.g. just before the SGI kick.> > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/smpboot.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index 44326d8..14774c5 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -412,8 +412,11 @@ int __cpu_up(unsigned int cpu) > > if ( rc < 0 ) > { > - printk("Failed to bring up CPU%d\n", cpu); > - return rc; > + if ( rc != -EAGAIN || psci_host_cpu_on_nr == 0 ) > + { > + printk("Failed to bring up CPU%d\n", cpu); > + return rc; > + } > } > > if ( psci_host_cpu_on_nr != 0 )
Ian Campbell
2013-Nov-26 11:24 UTC
Re: [PATCH 4/4] arm64: defer CPU initialization on ARM64 if PSCI is present
On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote:> Since PSCI is now handled in ARM generic smpboot code, we can remove > the PSCI todo from the ARM64 specific code and defer the actual PSCI > handling. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/arm64/smpboot.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c > index 8239590..715c44e 100644 > --- a/xen/arch/arm/arm64/smpboot.c > +++ b/xen/arch/arm/arm64/smpboot.c > @@ -61,8 +61,11 @@ 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 > + else if ( !strcmp(enable_method, "psci") ) > + { > + /* PSCI code is handled somewhere else */ > + return -EAGAIN;I think this should check for the presence of PSCI and either return an error or success (on the basis that nothing needs doing). Ian.
Ian Campbell
2013-Nov-26 11:25 UTC
Re: [PATCH 1/4] arm: parse PSCI node from the host device-tree
On Tue, 2013-11-26 at 11:12 +0000, Ian Campbell wrote:> On Mon, 2013-11-25 at 13:02 +0100, 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 callling > > Too many l''s in callling. > > > method, as it does not make much sense currently under Xen. > > > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > > --- > > xen/arch/arm/smpboot.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 43 insertions(+) > > > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > > index 6c90fa6..97bd414 100644 > > --- a/xen/arch/arm/smpboot.c > > +++ b/xen/arch/arm/smpboot.c > > @@ -89,6 +89,44 @@ smp_clear_cpu_maps (void) > > cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK; > > } > > > > +uint32_t psci_host_cpu_on_nr; > > Can we make this static and have a global "psci_enable" flag to use > elsewhere please.I think it will have become clear in my comments on patch #0/#2 but also can we have this in a separate psci.c please. It would be useful to have a no-psci command line option too, please. Ian.
On 11/26/2013 12:05 PM, Ian Campbell wrote:> On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote: >> Xen did not make use of the host provided ARM PSCI (Power State >> Coordination Interface) 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 put the code in a >> file shared by both. >> The ARM32 code was tested on Midway, but the ARM64 code was compile >> tested only. >> >> This approach seems to be the least intrusive, but one could also use >> more of the current ARM64 code by copying the PSCI/spin-table >> distinction code to a shared file and use that from both >> architectures. However that seems more complicated.Ian, thanks for the review and for the willingness to take the patches for 4.4 still. Will address the comments ASAP.> I don''t think that is needed (since armv7 spintable vs armv8 spintable > mechanisms are a bit different). But I would like to see the psci code > in a separate psci.c. It''s not much code right now but it will likely > grow.But there is already a xen/arch/arm/psci.c file, which holds the two functions to bring up/take down vCPUs. Not much in here, I could easily add my code there, but the scope of those two is quite different and I would find it strange to have both host and guest PSCI functions in one file. That was one reason to not create a new file, the other was that I found smpboot.c an obvious place to keep functions for SMP bringup in. So should I use psci_host.c? Or keep it in smpboot.c (at least for the time being)? And beside that there is quite some other (only guest related) code in Xen which simply uses *psci* in identifiers and filenames (like asm/psci.h), that''s why I used the psci_host prefix to tell them apart. Regards, Andre.> > I''ll comment a bit more on the individual patches shortly. > > Thanks. > > Ian. >> >> Please take a look and complain ;-) >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> >> Andre Przywara (4): >> arm: parse PSCI node from the host device-tree >> arm: add a function to invoke the PSCI handler and use it >> arm: dont give up on EAGAIN if PSCI is defined >> arm64: defer CPU initialization on ARM64 if PSCI is present >> >> xen/arch/arm/arm32/smpboot.c | 1 - >> xen/arch/arm/arm64/smpboot.c | 7 +++- >> xen/arch/arm/smpboot.c | 89 +++++++++++++++++++++++++++++++++++++++++--- >> 3 files changed, 88 insertions(+), 9 deletions(-) >> > >
On Wed, 2013-11-27 at 14:45 +0100, Andre Przywara wrote:> On 11/26/2013 12:05 PM, Ian Campbell wrote: > > On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote: > >> Xen did not make use of the host provided ARM PSCI (Power State > >> Coordination Interface) 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 put the code in a > >> file shared by both. > >> The ARM32 code was tested on Midway, but the ARM64 code was compile > >> tested only. > >> > >> This approach seems to be the least intrusive, but one could also use > >> more of the current ARM64 code by copying the PSCI/spin-table > >> distinction code to a shared file and use that from both > >> architectures. However that seems more complicated. > > Ian, > > thanks for the review and for the willingness to take the patches for > 4.4 still. Will address the comments ASAP. > > > I don''t think that is needed (since armv7 spintable vs armv8 spintable > > mechanisms are a bit different). But I would like to see the psci code > > in a separate psci.c. It''s not much code right now but it will likely > > grow. > > But there is already a xen/arch/arm/psci.c file, which holds the two > functions to bring up/take down vCPUs.Ah, sorry, I didn''t know about this. This file should be called vpscsi.c IMHO with the non-v version for the host stuff. Do you mind renaming as you go? [...]> And beside that there is quite some other (only guest related) code in > Xen which simply uses *psci* in identifiers and filenames (like > asm/psci.h), that''s why I used the psci_host prefix to tell them apart.Urk. I think the #defines are common since they are part of the spec and the do_pscsi could stand being do_vpscsi. Probably no need for vpsci.h here. I seem to be unable to type PSCI without typing PSCSI instead today, sorry... Ian.
Andre Przywara
2013-Nov-28 10:56 UTC
Re: [PATCH 1/4] arm: parse PSCI node from the host device-tree
On 11/26/2013 12:12 PM, Ian Campbell wrote:> On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote: >> + >> + if ( !dt_property_read_u32(psci, "cpu_on", &psci_host_cpu_on_nr) ) >> + { >> + printk("/psci node is missing the \"cpu_on\" property\n"); >> + return 4; >> + } > > http://www.spinics.net/lists/devicetree/msg05348.html updates the > bindings for PSCI 0.2. I suppose Midway must only support 0.1? > > I''d be OK with only supporting 0.1 right now, but it would be useful to > comment either in the code or in the commit message. > > (nb: I''m not sure of v4 was the final version of that series)If I look at http://www.spinics.net/lists/devicetree/msg12293.html I would keep it as it is - at least until the binding changes. Hopefully there will be a compatibility fallback if cpu_on should change to cpu_on-{32,64}. I will talk to Rob about this and will later send a fix if that is needed. Midway provides an implementation conforming to PSCI 0.2, however the device tree binding is still the one from the current kernel git HEAD.> >> + >> + return 0; >> +} >> + >> /* Parse the device tree and build the logical map array containing >> * MPIDR values related to logical cpus >> * Code base on Linux arch/arm/kernel/devtree.c >> @@ -107,6 +145,11 @@ void __init smp_init_cpus(void) >> bool_t bootcpu_valid = 0; >> int rc; >> >> + if ( psci_host_init() == 0 ) >> + { >> + printk(XENLOG_INFO "Using PSCI for SMP bringup\n"); >> + } >> + >> if ( (rc = arch_smp_init()) < 0 ) > > arch_smp_init is empty on both platforms. arm32 has a comment "TODO: > PSCI" ;-) > > I think we can nuke this function while we are here, since it''s only > purpose was as a PSCI placehoder. >But arch_smp_init() calls platform_smp_init(), which is not empty for the Exynos, VExpress and OMAP5 (it contains the native SMP bringup for these platforms). So shall I skip the superfluous arch_smp_init() and call platform_smp_init() directly from smpboot.c? Or keep it as it is and we clean it up later? Thanks, Andre.
Andre Przywara
2013-Nov-28 10:59 UTC
Re: [PATCH 2/4] arm: add a function to invoke the PSCI handler and use it
On 11/26/2013 12:18 PM, Ian Campbell wrote:> On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote: >> The PSCI handler is invoked via a secure monitor call with the >> arguments defined in registers [1]. Copy the function from the >> Linux code and adjust it to work on both ARM32 and ARM64. >> Later use that function instead of the generic GIC SEV kick to >> actually bring up the secondary CPUs. >> >> [1]: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0022b/index.html >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/arm32/smpboot.c | 1 - >> xen/arch/arm/smpboot.c | 39 +++++++++++++++++++++++++++++++++++---- >> 2 files changed, 35 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c >> index 88fe8fb..fcf653f 100644 >> --- a/xen/arch/arm/arm32/smpboot.c >> +++ b/xen/arch/arm/arm32/smpboot.c >> @@ -10,7 +10,6 @@ int __init arch_smp_init(void) >> >> int __init arch_cpu_init(int cpu, struct dt_device_node *dn) >> { >> - /* TODO handle PSCI init */ >> return 0; >> } >> >> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c >> index 97bd414..44326d8 100644 >> --- a/xen/arch/arm/smpboot.c >> +++ b/xen/arch/arm/smpboot.c >> @@ -89,6 +89,29 @@ smp_clear_cpu_maps (void) >> cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK; >> } >> >> +#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) > > Please can you put this in psci.c and provide wrappers e.g. > psci_cpu_up(). THese can return some suitable errno if PSCI isn''t > enabled. Or we could add a psci_enabled() call > > Why noinline?This was copied from Linux and I thought it was there for a reason, so I kept it. But looking closer this was to make sure Linux can use function pointers later (to abstract hvc and smc calling), so we can remove this for Xen. Regards, Andre.