diff mbox

[v2,3/6] arm: parse PSCI node from the host device-tree

Message ID 1385982538-17855-4-git-send-email-andre.przywara@linaro.org
State New
Headers show

Commit Message

Andre Przywara Dec. 2, 2013, 11:08 a.m. UTC
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

Comments

Julien Grall Dec. 2, 2013, 1:28 p.m. UTC | #1
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);
>
Andre Przywara Dec. 2, 2013, 1:44 p.m. UTC | #2
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);
>>
>
Julien Grall Dec. 2, 2013, 3 p.m. UTC | #3
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 :)
Ian Campbell Dec. 2, 2013, 3:05 p.m. UTC | #4
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.
Ian Campbell Dec. 2, 2013, 3:14 p.m. UTC | #5
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.
Andre Przywara Dec. 4, 2013, 12:37 p.m. UTC | #6
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.
>
>
Ian Campbell Dec. 4, 2013, 12:41 p.m. UTC | #7
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.
> >
> >
>
Andre Przywara Dec. 4, 2013, 12:44 p.m. UTC | #8
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.
>>>
>>>
>>
>
>
diff mbox

Patch

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);