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

[Julien Grall <julien.grall@linaro.org>]

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