Re: [Xen-devel] [PATCH v2 6/6] ARM: mach-virt: add SMP support using PSCI

[Marc Zyngier <marc.zyngier@xxxxxxx>]

On 18/12/12 12:19, Stefano Stabellini wrote:
> On Mon, 17 Dec 2012, Will Deacon wrote:
>> This patch adds support for SMP to mach-virt using the PSCI
>> infrastructure.
>>
>> Signed-off-by: Will Deacon <will.deacon@xxxxxxx>
>>
>>  arch/arm/mach-virt/Kconfig   |  1 +
>>  arch/arm/mach-virt/Makefile  |  1 +
>>  arch/arm/mach-virt/platsmp.c | 76 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  arch/arm/mach-virt/virt.c    |  6 ++++
>>  4 files changed, 84 insertions(+)
>>  create mode 100644 arch/arm/mach-virt/platsmp.c
>>
>> diff --git a/arch/arm/mach-virt/Kconfig b/arch/arm/mach-virt/Kconfig
>> index a568a2a..8958f0d 100644
>> --- a/arch/arm/mach-virt/Kconfig
>> +++ b/arch/arm/mach-virt/Kconfig
>> @@ -3,6 +3,7 @@ config ARCH_VIRT
>>      select ARCH_WANT_OPTIONAL_GPIOLIB
>>      select ARM_GIC
>>      select ARM_ARCH_TIMER
>> +    select ARM_PSCI
>>      select HAVE_SMP
>>      select CPU_V7
>>      select SPARSE_IRQ
> 
> Considering that PSCI is actually needed only to boot secondary cpus,
> maybe we want to select it if CONFIG_SMP is enabled?

Well, I was considering using it to "power-off" the VM when the last CPU
powers itself off, and this would apply to UP as well.

>> diff --git a/arch/arm/mach-virt/Makefile b/arch/arm/mach-virt/Makefile
>> index 7ddbfa6..042afc1 100644
>> --- a/arch/arm/mach-virt/Makefile
>> +++ b/arch/arm/mach-virt/Makefile
>> @@ -3,3 +3,4 @@
>>  #
>>  
>>  obj-y                                       := virt.o
>> +obj-$(CONFIG_SMP)                   += platsmp.o
>> diff --git a/arch/arm/mach-virt/platsmp.c b/arch/arm/mach-virt/platsmp.c
>> new file mode 100644
>> index 0000000..930362b
>> --- /dev/null
>> +++ b/arch/arm/mach-virt/platsmp.c
>> @@ -0,0 +1,76 @@
>> +/*
>> + * Dummy Virtual Machine - does what it says on the tin.
>> + *
>> + * Copyright (C) 2012 ARM Ltd
>> + * Author: Will Deacon <will.deacon@xxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/smp.h>
>> +#include <linux/of.h>
>> +
>> +#include <asm/psci.h>
>> +#include <asm/smp_plat.h>
>> +#include <asm/hardware/gic.h>
>> +
>> +extern void secondary_startup(void);
>> +
>> +/*
>> + * Enumerate the possible CPU set from the device tree.
>> + */
>> +static void __init virt_smp_init_cpus(void)
>> +{
>> +    struct device_node *dn = NULL;
>> +    int cpu = 0;
>> +
>> +    while ((dn = of_find_node_by_type(dn, "cpu"))) {
>> +            if (cpu < NR_CPUS)
>> +                    set_cpu_possible(cpu, true);
>> +            cpu++;
>> +    }
>> +
>> +    /* sanity check */
>> +    if (cpu > NR_CPUS)
>> +            pr_warning("no. of cores (%d) greater than configured maximum "
>> +                       "of %d - clipping\n",
>> +                       cpu, NR_CPUS);
>> +
>> +    set_smp_cross_call(gic_raise_softirq);
>> +}
>> +
>> +static void __init virt_smp_prepare_cpus(unsigned int max_cpus)
>> +{
>> +}
>> +
>> +static int __cpuinit virt_boot_secondary(unsigned int cpu,
>> +                                     struct task_struct *idle)
>> +{
>> +    if (psci_ops.cpu_on)
>> +            return psci_ops.cpu_on(cpu_logical_map(cpu),
>> +                                   __pa(secondary_startup));
>> +    return -ENODEV;
>> +}
> 
> Isn't there a better way to check whether PSCI is actually "enabled", as
> in present in the device tree and initialized correctly?

All methods are optional, so I'm afraid you have to check for their
validity each time you want to access one.

> Maybe we need a pcsi_enabled() static inline of some sort?
> 
> 
>> +static void __cpuinit virt_secondary_init(unsigned int cpu)
>> +{
>> +    gic_secondary_init(0);
>> +}
>> +
>> +struct smp_operations __initdata virt_smp_ops = {
>> +    .smp_init_cpus          = virt_smp_init_cpus,
>> +    .smp_prepare_cpus       = virt_smp_prepare_cpus,
>> +    .smp_secondary_init     = virt_secondary_init,
>> +    .smp_boot_secondary     = virt_boot_secondary,
>> +};
>> diff --git a/arch/arm/mach-virt/virt.c b/arch/arm/mach-virt/virt.c
>> index 174b9da..d764835 100644
>> --- a/arch/arm/mach-virt/virt.c
>> +++ b/arch/arm/mach-virt/virt.c
>> @@ -20,6 +20,7 @@
>>  
>>  #include <linux/of_irq.h>
>>  #include <linux/of_platform.h>
>> +#include <linux/smp.h>
>>  
>>  #include <asm/arch_timer.h>
>>  #include <asm/hardware/gic.h>
>> @@ -56,10 +57,15 @@ static struct sys_timer virt_timer = {
>>      .init = virt_timer_init,
>>  };
>>  
>> +#ifdef CONFIG_SMP
>> +extern struct smp_operations virt_smp_ops;
>> +#endif
>> +
>>  DT_MACHINE_START(VIRT, "Dummy Virtual Machine")
>>      .init_irq       = gic_init_irq,
>>      .handle_irq     = gic_handle_irq,
>>      .timer          = &virt_timer,
>>      .init_machine   = virt_init,
>> +    .smp            = smp_ops(virt_smp_ops),
>>      .dt_compat      = virt_dt_match,
>>  MACHINE_END
>> -- 
>> 1.8.0
> 


-- 
Jazz is not dead. It just smells funny...


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