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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
> > + * 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);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel