| Message ID | 1364388639-11210-1-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/03/13 12:50, Stefano Stabellini wrote: > Check for the presence of PSCI before setting smp_ops, use PSCI if it is > available. > > This is useful because at least when running on Xen it's possible to have a > PSCI node for example on a Versatile Express or an Exynos5 machine. In these > cases the PSCI SMP calls should be the ones to be called. > > Remove virt_smp_ops and platsmp.c from mach-virt because they aren't needed > anymore. > > > > This patch was originally part of this series: > > http://marc.info/?l=linux-arm-kernel&m=136430903110734&w=2 > > I am keeping it separate now since it is the only non-obvious change and > it is not Xen related. > > > > > Changes in v3: > - move the call to psci_init to setup_arch; > - export psci_smp_ops from psci.h; > - introduce psci_smp_available; > - introduce stub functions for psci_init and psci_smp_available ifndef > CONFIG_ARM_PSCI; > - only compile psci_smp functions ifdef CONFIG_SMP. > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > CC: will.deacon@arm.com > CC: arnd@arndb.de > CC: marc.zyngier@arm.com > CC: linux@arm.linux.org.uk > CC: nico@linaro.org > --- > arch/arm/include/asm/psci.h | 9 ++++ > arch/arm/kernel/psci.c | 97 ++++++++++++++++++++++++++++++++++-------- > arch/arm/kernel/setup.c | 7 +++- > arch/arm/mach-virt/Makefile | 1 - > arch/arm/mach-virt/platsmp.c | 58 ------------------------- > arch/arm/mach-virt/virt.c | 3 - > 6 files changed, 94 insertions(+), 81 deletions(-) > delete mode 100644 arch/arm/mach-virt/platsmp.c > > diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h > index ce0dbe7..ddef231 100644 > --- a/arch/arm/include/asm/psci.h > +++ b/arch/arm/include/asm/psci.h > @@ -32,5 +32,14 @@ struct psci_operations { > }; > > extern struct psci_operations psci_ops; > +extern struct smp_operations psci_smp_ops; > + > +#ifdef CONFIG_ARM_PSCI > +int psci_init(void); > +bool psci_smp_available(void); > +#else > +static inline int psci_init(void) { return -ENODEV; } > +static inline bool psci_smp_available(void) { return false; } > +#endif > > #endif /* __ASM_ARM_PSCI_H */ > diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c > index 3653164..90f0839 100644 > --- a/arch/arm/kernel/psci.c > +++ b/arch/arm/kernel/psci.c > @@ -16,6 +16,7 @@ > #define pr_fmt(fmt) "psci: " fmt > > #include <linux/init.h> > +#include <linux/irqchip/arm-gic.h> > #include <linux/of.h> > > #include <asm/compiler.h> > @@ -23,8 +24,9 @@ > #include <asm/opcodes-sec.h> > #include <asm/opcodes-virt.h> > #include <asm/psci.h> > +#include <asm/smp_plat.h> > > -struct psci_operations psci_ops; > +extern void secondary_startup(void); > > static int (*invoke_psci_fn)(u32, u32, u32, u32); > > @@ -36,7 +38,11 @@ enum psci_function { > PSCI_FN_MAX, > }; > > -static u32 psci_function_id[PSCI_FN_MAX]; > +struct psci_function_desc { > + enum psci_function func; > + bool valid; > +}; > +static struct psci_function_desc psci_function_id[PSCI_FN_MAX]; > > #define PSCI_RET_SUCCESS 0 > #define PSCI_RET_EOPNOTSUPP -1 > @@ -116,7 +122,10 @@ static int psci_cpu_suspend(struct psci_power_state state, > int err; > u32 fn, power_state; > > - fn = psci_function_id[PSCI_FN_CPU_SUSPEND]; > + if (!psci_function_id[PSCI_FN_CPU_SUSPEND].valid) > + return -ENOSYS; > + > + fn = psci_function_id[PSCI_FN_CPU_SUSPEND].func; > power_state = psci_power_state_pack(state); > err = invoke_psci_fn(fn, power_state, entry_point, 0); > return psci_to_linux_errno(err); > @@ -127,7 +136,10 @@ static int psci_cpu_off(struct psci_power_state state) > int err; > u32 fn, power_state; > > - fn = psci_function_id[PSCI_FN_CPU_OFF]; > + if (!psci_function_id[PSCI_FN_CPU_OFF].valid) > + return -ENOSYS; > + > + fn = psci_function_id[PSCI_FN_CPU_OFF].func; > power_state = psci_power_state_pack(state); > err = invoke_psci_fn(fn, power_state, 0, 0); > return psci_to_linux_errno(err); > @@ -138,7 +150,10 @@ static int psci_cpu_on(unsigned long cpuid, unsigned long entry_point) > int err; > u32 fn; > > - fn = psci_function_id[PSCI_FN_CPU_ON]; > + if (!psci_function_id[PSCI_FN_CPU_ON].valid) > + return -ENOSYS; > + > + fn = psci_function_id[PSCI_FN_CPU_ON].func; > err = invoke_psci_fn(fn, cpuid, entry_point, 0); > return psci_to_linux_errno(err); > } > @@ -148,25 +163,64 @@ static int psci_migrate(unsigned long cpuid) > int err; > u32 fn; > > - fn = psci_function_id[PSCI_FN_MIGRATE]; > + if (!psci_function_id[PSCI_FN_MIGRATE].valid) > + return -ENOSYS; > + > + fn = psci_function_id[PSCI_FN_MIGRATE].func; > err = invoke_psci_fn(fn, cpuid, 0, 0); > return psci_to_linux_errno(err); > } > > +struct psci_operations psci_ops = { > + .cpu_suspend = psci_cpu_suspend, > + .cpu_off = psci_cpu_off, > + .cpu_on = psci_cpu_on, > + .migrate = psci_migrate, > +}; > + > +#ifdef CONFIG_SMP > +static void __init psci_smp_init_cpus(void) > +{ > +} > + > +static void __init psci_smp_prepare_cpus(unsigned int max_cpus) > +{ > +} > + > +static int __cpuinit psci_boot_secondary(unsigned int cpu, > + struct task_struct *idle) > +{ > + return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup)); > +} > + > +static void __cpuinit psci_secondary_init(unsigned int cpu) > +{ > + gic_secondary_init(0); > +} So here we end-up with a dependency between SMP, PSCI, and GIC. I really can't see that being a good idea, even I like the general direction of this series. M.
Hi Stefano, On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini wrote: > Check for the presence of PSCI before setting smp_ops, use PSCI if it is > available. > > This is useful because at least when running on Xen it's possible to have a > PSCI node for example on a Versatile Express or an Exynos5 machine. In these > cases the PSCI SMP calls should be the ones to be called. > > Remove virt_smp_ops and platsmp.c from mach-virt because they aren't needed > anymore. [...] > +struct psci_operations psci_ops = { > + .cpu_suspend = psci_cpu_suspend, > + .cpu_off = psci_cpu_off, > + .cpu_on = psci_cpu_on, > + .migrate = psci_migrate, > +}; > + > +#ifdef CONFIG_SMP > +static void __init psci_smp_init_cpus(void) > +{ > +} > + > +static void __init psci_smp_prepare_cpus(unsigned int max_cpus) > +{ > +} > + > +static int __cpuinit psci_boot_secondary(unsigned int cpu, > + struct task_struct *idle) > +{ > + return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup)); > +} > + > +static void __cpuinit psci_secondary_init(unsigned int cpu) > +{ > + gic_secondary_init(0); > +} > + > +struct smp_operations __initdata psci_smp_ops = { > + .smp_init_cpus = psci_smp_init_cpus, > + .smp_prepare_cpus = psci_smp_prepare_cpus, > + .smp_secondary_init = psci_secondary_init, > + .smp_boot_secondary = psci_boot_secondary, > +}; > +#endif As I said before, I don't agree with bolting these two interfaces together like this and, as it stands, I'm afraid I have to NAK this patch. A potential alternative is to have a set of virt_smp_ops, which have wrappers around the psci functions, but that requires agreement from Xen and KVM to implement the same PSCI interface, which feels unfair to me. I see what you're trying to do, but I can't go along with it. Sorry. Will
On 03/27/2013 07:50 AM, Stefano Stabellini wrote: > Check for the presence of PSCI before setting smp_ops, use PSCI if it is > available. > > This is useful because at least when running on Xen it's possible to have a > PSCI node for example on a Versatile Express or an Exynos5 machine. In these > cases the PSCI SMP calls should be the ones to be called. I have a similar patch in my tree. I thought I had sent it out, but looks like I didn't. I didn't make smp_ops default to this or change mach-virt, so we should go with yours. [...] > @@ -36,7 +38,11 @@ enum psci_function { > PSCI_FN_MAX, > }; > > -static u32 psci_function_id[PSCI_FN_MAX]; > +struct psci_function_desc { > + enum psci_function func; > + bool valid; Why can't you use a NULL function ptr or a 0 psci_function_id to indicate not valid? > +}; > +static struct psci_function_desc psci_function_id[PSCI_FN_MAX]; > > #define PSCI_RET_SUCCESS 0 > #define PSCI_RET_EOPNOTSUPP -1 > @@ -116,7 +122,10 @@ static int psci_cpu_suspend(struct psci_power_state state, > int err; > u32 fn, power_state; > > - fn = psci_function_id[PSCI_FN_CPU_SUSPEND]; > + if (!psci_function_id[PSCI_FN_CPU_SUSPEND].valid) > + return -ENOSYS; > + > + fn = psci_function_id[PSCI_FN_CPU_SUSPEND].func; > power_state = psci_power_state_pack(state); > err = invoke_psci_fn(fn, power_state, entry_point, 0); > return psci_to_linux_errno(err); > @@ -127,7 +136,10 @@ static int psci_cpu_off(struct psci_power_state state) > int err; > u32 fn, power_state; > > - fn = psci_function_id[PSCI_FN_CPU_OFF]; > + if (!psci_function_id[PSCI_FN_CPU_OFF].valid) > + return -ENOSYS; > + > + fn = psci_function_id[PSCI_FN_CPU_OFF].func; > power_state = psci_power_state_pack(state); > err = invoke_psci_fn(fn, power_state, 0, 0); > return psci_to_linux_errno(err); > @@ -138,7 +150,10 @@ static int psci_cpu_on(unsigned long cpuid, unsigned long entry_point) > int err; > u32 fn; > > - fn = psci_function_id[PSCI_FN_CPU_ON]; > + if (!psci_function_id[PSCI_FN_CPU_ON].valid) > + return -ENOSYS; > + > + fn = psci_function_id[PSCI_FN_CPU_ON].func; > err = invoke_psci_fn(fn, cpuid, entry_point, 0); > return psci_to_linux_errno(err); > } > @@ -148,25 +163,64 @@ static int psci_migrate(unsigned long cpuid) > int err; > u32 fn; > > - fn = psci_function_id[PSCI_FN_MIGRATE]; > + if (!psci_function_id[PSCI_FN_MIGRATE].valid) > + return -ENOSYS; > + > + fn = psci_function_id[PSCI_FN_MIGRATE].func; > err = invoke_psci_fn(fn, cpuid, 0, 0); > return psci_to_linux_errno(err); > } > > +struct psci_operations psci_ops = { > + .cpu_suspend = psci_cpu_suspend, > + .cpu_off = psci_cpu_off, > + .cpu_on = psci_cpu_on, > + .migrate = psci_migrate, > +}; > + > +#ifdef CONFIG_SMP > +static void __init psci_smp_init_cpus(void) > +{ > +} > + > +static void __init psci_smp_prepare_cpus(unsigned int max_cpus) > +{ > +} You can leave these 2 functions as NULL. > + > +static int __cpuinit psci_boot_secondary(unsigned int cpu, > + struct task_struct *idle) > +{ > + return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup)); > +} > + > +static void __cpuinit psci_secondary_init(unsigned int cpu) > +{ > + gic_secondary_init(0); > +} > + > +struct smp_operations __initdata psci_smp_ops = { > + .smp_init_cpus = psci_smp_init_cpus, > + .smp_prepare_cpus = psci_smp_prepare_cpus, > + .smp_secondary_init = psci_secondary_init, > + .smp_boot_secondary = psci_boot_secondary, You should also include hotplug. Here's what I had: #ifdef CONFIG_HOTPLUG_CPU void __ref psci_cpu_die(unsigned int cpu) { const struct psci_power_state ps = { .type = PSCI_POWER_STATE_TYPE_POWER_DOWN, }; if (psci_ops.cpu_off) psci_ops.cpu_off(ps); /* We should never return */ panic("psci: cpu %d failed to shutdown\n", cpu); } #else #define psci_cpu_die NULL #endif
On 03/27/2013 08:35 AM, Marc Zyngier wrote: > On 27/03/13 12:50, Stefano Stabellini wrote: >> Check for the presence of PSCI before setting smp_ops, use PSCI if it is >> available. >> >> This is useful because at least when running on Xen it's possible to have a >> PSCI node for example on a Versatile Express or an Exynos5 machine. In these >> cases the PSCI SMP calls should be the ones to be called. >> >> Remove virt_smp_ops and platsmp.c from mach-virt because they aren't needed >> anymore. [...] >> +static void __cpuinit psci_secondary_init(unsigned int cpu) >> +{ >> + gic_secondary_init(0); >> +} > > So here we end-up with a dependency between SMP, PSCI, and GIC. > I really can't see that being a good idea, even I like the general > direction of this series. > The GIC dependency should be removed with Catalin's series to use notifiers for GIC cpu interface init. Rob
On Wed, 27 Mar 2013, Will Deacon wrote: > Hi Stefano, > > On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini wrote: > > Check for the presence of PSCI before setting smp_ops, use PSCI if it is > > available. > > > > This is useful because at least when running on Xen it's possible to have a > > PSCI node for example on a Versatile Express or an Exynos5 machine. In these > > cases the PSCI SMP calls should be the ones to be called. > > > > Remove virt_smp_ops and platsmp.c from mach-virt because they aren't needed > > anymore. > > [...] > > > +struct psci_operations psci_ops = { > > + .cpu_suspend = psci_cpu_suspend, > > + .cpu_off = psci_cpu_off, > > + .cpu_on = psci_cpu_on, > > + .migrate = psci_migrate, > > +}; > > + > > +#ifdef CONFIG_SMP > > +static void __init psci_smp_init_cpus(void) > > +{ > > +} > > + > > +static void __init psci_smp_prepare_cpus(unsigned int max_cpus) > > +{ > > +} > > + > > +static int __cpuinit psci_boot_secondary(unsigned int cpu, > > + struct task_struct *idle) > > +{ > > + return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup)); > > +} > > + > > +static void __cpuinit psci_secondary_init(unsigned int cpu) > > +{ > > + gic_secondary_init(0); > > +} > > + > > +struct smp_operations __initdata psci_smp_ops = { > > + .smp_init_cpus = psci_smp_init_cpus, > > + .smp_prepare_cpus = psci_smp_prepare_cpus, > > + .smp_secondary_init = psci_secondary_init, > > + .smp_boot_secondary = psci_boot_secondary, > > +}; > > +#endif > > As I said before, I don't agree with bolting these two interfaces together > like this and, as it stands, I'm afraid I have to NAK this patch. > > A potential alternative is to have a set of virt_smp_ops, which have > wrappers around the psci functions, but that requires agreement from Xen and > KVM to implement the same PSCI interface, which feels unfair to me. > > I see what you're trying to do, but I can't go along with it. Sorry. OK, let's see if I can make this acceptable to you. Would you agree on a patch that moves virt_smp_ops out of mach-virt and renames them to psci_smp_ops (maybe to arch/arm/kernel/psci_smp_ops.c)? Would you agree on initializing psci from setup_arch, right after the call to arm_dt_init_cpu_maps()? Finally the most controversial point: would you agree on using psci_smp_ops by default if they are available? If not, would you at least agree on letting Xen overwrite the default machine smp_ops? We need one or the other for dom0 support.
On 03/27/2013 08:38 AM, Will Deacon wrote: > Hi Stefano, > > On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini wrote: >> Check for the presence of PSCI before setting smp_ops, use PSCI if it is >> available. >> >> This is useful because at least when running on Xen it's possible to have a >> PSCI node for example on a Versatile Express or an Exynos5 machine. In these >> cases the PSCI SMP calls should be the ones to be called. >> >> Remove virt_smp_ops and platsmp.c from mach-virt because they aren't needed >> anymore. > > [...] > >> +struct psci_operations psci_ops = { >> + .cpu_suspend = psci_cpu_suspend, >> + .cpu_off = psci_cpu_off, >> + .cpu_on = psci_cpu_on, >> + .migrate = psci_migrate, >> +}; >> + >> +#ifdef CONFIG_SMP >> +static void __init psci_smp_init_cpus(void) >> +{ >> +} >> + >> +static void __init psci_smp_prepare_cpus(unsigned int max_cpus) >> +{ >> +} >> + >> +static int __cpuinit psci_boot_secondary(unsigned int cpu, >> + struct task_struct *idle) >> +{ >> + return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup)); >> +} >> + >> +static void __cpuinit psci_secondary_init(unsigned int cpu) >> +{ >> + gic_secondary_init(0); >> +} >> + >> +struct smp_operations __initdata psci_smp_ops = { >> + .smp_init_cpus = psci_smp_init_cpus, >> + .smp_prepare_cpus = psci_smp_prepare_cpus, >> + .smp_secondary_init = psci_secondary_init, >> + .smp_boot_secondary = psci_boot_secondary, >> +}; >> +#endif > > As I said before, I don't agree with bolting these two interfaces together > like this and, as it stands, I'm afraid I have to NAK this patch. > > A potential alternative is to have a set of virt_smp_ops, which have > wrappers around the psci functions, but that requires agreement from Xen and > KVM to implement the same PSCI interface, which feels unfair to me. I need the same smp ops for highbank. By using mach-virt Xen is using the same interface as KVM. This patch does not change that, but rather allows other platforms to use the same smp ops as well. Isn't the whole point of PSCI to have a common interface? No one is making Xen use PSCI at all. It is a choice and since they are making that choice, why would the PSCI interface be different? Rob
On 03/27/2013 11:23 AM, Stefano Stabellini wrote: > On Wed, 27 Mar 2013, Will Deacon wrote: >> Hi Stefano, >> >> On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini wrote: >>> Check for the presence of PSCI before setting smp_ops, use PSCI if it is >>> available. >>> >>> This is useful because at least when running on Xen it's possible to have a >>> PSCI node for example on a Versatile Express or an Exynos5 machine. In these >>> cases the PSCI SMP calls should be the ones to be called. >>> >>> Remove virt_smp_ops and platsmp.c from mach-virt because they aren't needed >>> anymore. >> >> [...] >> >>> +struct psci_operations psci_ops = { >>> + .cpu_suspend = psci_cpu_suspend, >>> + .cpu_off = psci_cpu_off, >>> + .cpu_on = psci_cpu_on, >>> + .migrate = psci_migrate, >>> +}; >>> + >>> +#ifdef CONFIG_SMP >>> +static void __init psci_smp_init_cpus(void) >>> +{ >>> +} >>> + >>> +static void __init psci_smp_prepare_cpus(unsigned int max_cpus) >>> +{ >>> +} >>> + >>> +static int __cpuinit psci_boot_secondary(unsigned int cpu, >>> + struct task_struct *idle) >>> +{ >>> + return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup)); >>> +} >>> + >>> +static void __cpuinit psci_secondary_init(unsigned int cpu) >>> +{ >>> + gic_secondary_init(0); >>> +} >>> + >>> +struct smp_operations __initdata psci_smp_ops = { >>> + .smp_init_cpus = psci_smp_init_cpus, >>> + .smp_prepare_cpus = psci_smp_prepare_cpus, >>> + .smp_secondary_init = psci_secondary_init, >>> + .smp_boot_secondary = psci_boot_secondary, >>> +}; >>> +#endif >> >> As I said before, I don't agree with bolting these two interfaces together >> like this and, as it stands, I'm afraid I have to NAK this patch. >> >> A potential alternative is to have a set of virt_smp_ops, which have >> wrappers around the psci functions, but that requires agreement from Xen and >> KVM to implement the same PSCI interface, which feels unfair to me. >> >> I see what you're trying to do, but I can't go along with it. Sorry. > > OK, let's see if I can make this acceptable to you. > > > Would you agree on a patch that moves virt_smp_ops out of mach-virt and > renames them to psci_smp_ops (maybe to arch/arm/kernel/psci_smp_ops.c)? > > Would you agree on initializing psci from setup_arch, right after the > call to arm_dt_init_cpu_maps()? > > Finally the most controversial point: would you agree on using > psci_smp_ops by default if they are available? > If not, would you at least agree on letting Xen overwrite the default > machine smp_ops? > We need one or the other for dom0 support. It should not be *always* use PSCI smp ops if available, but use them only if the platform does not define its own smp ops. Rob
On Wed, Mar 27, 2013 at 04:33:58PM +0000, Rob Herring wrote: > On 03/27/2013 08:38 AM, Will Deacon wrote: > > On Wed, Mar 27, 2013 at 12:50:39PM +0000, Stefano Stabellini wrote: > >> +struct smp_operations __initdata psci_smp_ops = { > >> + .smp_init_cpus = psci_smp_init_cpus, > >> + .smp_prepare_cpus = psci_smp_prepare_cpus, > >> + .smp_secondary_init = psci_secondary_init, > >> + .smp_boot_secondary = psci_boot_secondary, > >> +}; > >> +#endif > > > > As I said before, I don't agree with bolting these two interfaces together > > like this and, as it stands, I'm afraid I have to NAK this patch. > > > > A potential alternative is to have a set of virt_smp_ops, which have > > wrappers around the psci functions, but that requires agreement from Xen and > > KVM to implement the same PSCI interface, which feels unfair to me. > > I need the same smp ops for highbank. By using mach-virt Xen is using > the same interface as KVM. This patch does not change that, but rather > allows other platforms to use the same smp ops as well. > > Isn't the whole point of PSCI to have a common interface? No one is > making Xen use PSCI at all. It is a choice and since they are making > that choice, why would the PSCI interface be different? The channel is common, sure, but I wouldn't expect the semantics of each call to be identical between firmware implementations (going back to my previous examples of CPU IDs and implementation-defined state parameters). If a platform happens to have an id-mapping from smp_operations to psci, then I still think there should be an indirection in there so that we have the flexibility to change the smp_operations if we wish and not give platforms the false impression that these two things are equivalent. Will
On Wed, Mar 27, 2013 at 04:23:15PM +0000, Stefano Stabellini wrote: > OK, let's see if I can make this acceptable to you. > > > Would you agree on a patch that moves virt_smp_ops out of mach-virt and > renames them to psci_smp_ops (maybe to arch/arm/kernel/psci_smp_ops.c)? Moving the code out of psci.c is certainly a good first step, yes. > Would you agree on initializing psci from setup_arch, right after the > call to arm_dt_init_cpu_maps()? Hmmm. An early_initcall runs before SMP is up, so why do you need this earlier than that? Is it because you don't want to set the SMP ops later on? > Finally the most controversial point: would you agree on using > psci_smp_ops by default if they are available? > If not, would you at least agree on letting Xen overwrite the default > machine smp_ops? > We need one or the other for dom0 support. Again, I think there needs to be a dummy layer between the smp_ops and PSCI, rather than assigning the things directly if we're going to use this as a default implementation. I still question whether default PSCI operations make any sense though... I understand that you're currently saying `yes, Xen can use the same firmware interface as KVM' but will that always be true? What happens when we want to run virtual machines on multi-cluster platforms, for example? Will KVM and Xen make sure that CPU affinities are described in the same way? What if one or the other decides to pass side-band information in the power_state parameters? In all of these cases, we'd have to split the code back up, so I don't see any long-term value in consolidating everything just because it might be possible today. The real problem you're trying to solve seems to stem from patching the smp_ops in your dom0 kernel. Can you elaborate a bit more on what's going on here please? How would having PSCI default smp_ops help you? Cheers, Will
On Wednesday 27 March 2013, Will Deacon wrote: > The channel is common, sure, but I wouldn't expect the semantics of each > call to be identical between firmware implementations (going back to my > previous examples of CPU IDs and implementation-defined state parameters). > > If a platform happens to have an id-mapping from smp_operations to psci, > then I still think there should be an indirection in there so that we have > the flexibility to change the smp_operations if we wish and not give > platforms the false impression that these two things are equivalent. I think the only reasonably implementation for psci is if we can assume that each callback with a specific property name has a well-defined behavior, and we should mandate that every platform that implements the callbacks we need for SMP actually implements them according to the spec. What would be the point of a standard psci interface if the specific implementation are not required to follow the same semantics? Arnd
On Wed, Mar 27, 2013 at 05:50:51PM +0000, Arnd Bergmann wrote: > On Wednesday 27 March 2013, Will Deacon wrote: > > The channel is common, sure, but I wouldn't expect the semantics of each > > call to be identical between firmware implementations (going back to my > > previous examples of CPU IDs and implementation-defined state parameters). > > > > If a platform happens to have an id-mapping from smp_operations to psci, > > then I still think there should be an indirection in there so that we have > > the flexibility to change the smp_operations if we wish and not give > > platforms the false impression that these two things are equivalent. > > I think the only reasonably implementation for psci is if we can assume > that each callback with a specific property name has a well-defined behavior, > and we should mandate that every platform that implements the callbacks > we need for SMP actually implements them according to the spec. > > What would be the point of a standard psci interface if the specific > implementation are not required to follow the same semantics? The interface *is* standard. The functions have well-defined headers and can be called in the same way between implementations. The difference is in the semantics of the parameters. For example: int cpu_off(u32 power_state); If you look at the power_state parameter, it's actually a struct (see struct psci_power_state) with a u16 id field. The current specification describes that field as `This is platform specific, the number is understood by the firmware, and used to program the power controller.'. So unless we get everybody to agree on the definition of that field, we can't blindly plug the interfaces together. Furthermore, there are other parameters like this and, as new functions are specified, I would expect them to grow. Will
On 03/27/2013 01:12 PM, Will Deacon wrote: > On Wed, Mar 27, 2013 at 05:50:51PM +0000, Arnd Bergmann wrote: >> On Wednesday 27 March 2013, Will Deacon wrote: >>> The channel is common, sure, but I wouldn't expect the semantics of each >>> call to be identical between firmware implementations (going back to my >>> previous examples of CPU IDs and implementation-defined state parameters). >>> >>> If a platform happens to have an id-mapping from smp_operations to psci, >>> then I still think there should be an indirection in there so that we have >>> the flexibility to change the smp_operations if we wish and not give >>> platforms the false impression that these two things are equivalent. >> >> I think the only reasonably implementation for psci is if we can assume >> that each callback with a specific property name has a well-defined behavior, >> and we should mandate that every platform that implements the callbacks >> we need for SMP actually implements them according to the spec. >> >> What would be the point of a standard psci interface if the specific >> implementation are not required to follow the same semantics? > > The interface *is* standard. The functions have well-defined headers and can > be called in the same way between implementations. The difference is in the > semantics of the parameters. For example: > > int cpu_off(u32 power_state); > > If you look at the power_state parameter, it's actually a struct (see struct > psci_power_state) with a u16 id field. The current specification describes > that field as `This is platform specific, the number is understood by the > firmware, and used to program the power controller.'. > > So unless we get everybody to agree on the definition of that field, we > can't blindly plug the interfaces together. Furthermore, there are other > parameters like this and, as new functions are specified, I would expect > them to grow. Which is why I've said I think the ID field is a bad idea. I've used it in my implementation, but only in the case of system level reset, power-off, and suspend. I don't see how it would be used otherwise. I guess you could define a value of 0 must be supported at a minimum and then default implementation would at least work to some level. Rob
On Wednesday 27 March 2013, Will Deacon wrote: > The interface is standard. The functions have well-defined headers and can > be called in the same way between implementations. The difference is in the > semantics of the parameters. For example: > > int cpu_off(u32 power_state); I think that is the opposite of well-defined :( > If you look at the power_state parameter, it's actually a struct (see struct > psci_power_state) with a u16 id field. The current specification describes > that field as `This is platform specific, the number is understood by the > firmware, and used to program the power controller.'. > > So unless we get everybody to agree on the definition of that field, we > can't blindly plug the interfaces together. Furthermore, there are other > parameters like this and, as new functions are specified, I would expect > them to grow. I think it's expected that there will be vendor specific extensions, but any interface that is part of the standard has to be completely specified there, anything else is pure madness. Perhaps we could extend the device tree binding to add the missing bits, and pass the values that you are supposed to pass there as properties of the node, but it would be much more logical to require the interface to be well-behaved in the first place. Arnd
On Wed, 27 Mar 2013, Will Deacon wrote: > On Wed, Mar 27, 2013 at 04:23:15PM +0000, Stefano Stabellini wrote: > > OK, let's see if I can make this acceptable to you. > > > > > > Would you agree on a patch that moves virt_smp_ops out of mach-virt and > > renames them to psci_smp_ops (maybe to arch/arm/kernel/psci_smp_ops.c)? > > Moving the code out of psci.c is certainly a good first step, yes. OK, I'll do that. > > Would you agree on initializing psci from setup_arch, right after the > > call to arm_dt_init_cpu_maps()? > > Hmmm. An early_initcall runs before SMP is up, so why do you need this > earlier than that? Is it because you don't want to set the SMP ops later on? Because I need to set the right smp_ops before any of the smp_ops functions are called. If I wait until early_initcall, the wrong smp_init_cpus is going to be called. > > Finally the most controversial point: would you agree on using > > psci_smp_ops by default if they are available? > > If not, would you at least agree on letting Xen overwrite the default > > machine smp_ops? > > We need one or the other for dom0 support. > > Again, I think there needs to be a dummy layer between the smp_ops and PSCI, > rather than assigning the things directly if we're going to use this as a > default implementation. I still question whether default PSCI operations make > any sense though... I understand that you're currently saying `yes, Xen can > use the same firmware interface as KVM' but will that always be true? What > happens when we want to run virtual machines on multi-cluster platforms, for > example? Will KVM and Xen make sure that CPU affinities are described in the > same way? What if one or the other decides to pass side-band information in > the power_state parameters? > > In all of these cases, we'd have to split the code back up, so I don't see > any long-term value in consolidating everything just because it might be > possible today. The real problem you're trying to solve seems to stem from > patching the smp_ops in your dom0 kernel. That is correct. Although having a clean generic solution would be preferable to me, if we cannot reach an agreement or if just cannot be done, an hack that patch smp_ops would also solve the Xen problem (see http://marc.info/?l=linux-arm-kernel&m=136440425214113&w=2). I distaste hacks as anybody else, so I would rather solve the issue in a different way. > Can you elaborate a bit more on > what's going on here please? How would having PSCI default smp_ops help you? As Arnd quickly explained (http://marc.info/?l=linux-arm-kernel&m=136440746215589&w=2), the hardware environment provided by Xen to Dom0 looks very similar to the native hardware but with a few important differences: - the amount of memory is probably lower, in fact Xen is going to edit the device tree and only tell Dom0 about the memory that Dom0 can actually see; - the number of cpus might be different, in fact Xen is going to edit the device tree and tell Dom0 about the vcpus that Dom0 can actually see; - not all the devices present might actually be exported to Dom0, in fact Xen is going to remove these devices from the device tree so that Dom0 won't try to access them; - the interface to bring up secondary cpus is different and based on PSCI, in fact Xen is going to add a PSCI node to the device tree so that Dom0 can use it. Oh wait, Dom0 is not going to use the PSCI interface even if the node is present on device tree because it's going to prefer the platform smp_ops instead. Fail. Xen can however add any nodes to device tree besides PSCI, it already adds a Xen hypervisor node, unfortunately the smp_ops are not detected via device tree at the moment so that won't help. So I guess what we really need is a way to export a set of firmware operations for cpu bring up via device tree. I thought that PSCI was exactly meant for this, but if it isn't, then we need to come up with something else.
On Thu, 28 Mar 2013, Stefano Stabellini wrote: > - the interface to bring up secondary cpus is different and based on > PSCI, in fact Xen is going to add a PSCI node to the device tree so that > Dom0 can use it. > > Oh wait, Dom0 is not going to use the PSCI interface even if the node is > present on device tree because it's going to prefer the platform smp_ops > instead. Waitaminute... I must have missed this part. Who said platform specific methods must be used in preference to PSCI? If DT does provide PSCI description, then PSCI should be used. Doing otherwise is senseless. If PSCI is not to be used, then it should not be present in DT. Nicolas
On 03/28/2013 09:51 AM, Nicolas Pitre wrote: > On Thu, 28 Mar 2013, Stefano Stabellini wrote: > >> - the interface to bring up secondary cpus is different and based on >> PSCI, in fact Xen is going to add a PSCI node to the device tree so that >> Dom0 can use it. >> >> Oh wait, Dom0 is not going to use the PSCI interface even if the node is >> present on device tree because it's going to prefer the platform smp_ops >> instead. > > Waitaminute... I must have missed this part. > > Who said platform specific methods must be used in preference to PSCI? I did. Specifically, I said the platform should be allowed to provide its own smp_ops. A platform may need to do addtional things on top of PSCI for example. > If DT does provide PSCI description, then PSCI should be used. Doing > otherwise is senseless. If PSCI is not to be used, then it should not > be present in DT. You can't assume the DT and kernel are in-sync. For example, I've added PSCI in the firmware and DTB (part of the firmware), but the highbank kernel may or may not use it depending if I convert it. Rob > > > Nicolas > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, 28 Mar 2013, Rob Herring wrote: > On 03/28/2013 09:51 AM, Nicolas Pitre wrote: > > On Thu, 28 Mar 2013, Stefano Stabellini wrote: > > > >> - the interface to bring up secondary cpus is different and based on > >> PSCI, in fact Xen is going to add a PSCI node to the device tree so that > >> Dom0 can use it. > >> > >> Oh wait, Dom0 is not going to use the PSCI interface even if the node is > >> present on device tree because it's going to prefer the platform smp_ops > >> instead. > > > > Waitaminute... I must have missed this part. > > > > Who said platform specific methods must be used in preference to PSCI? > > I did. Specifically, I said the platform should be allowed to provide > its own smp_ops. A platform may need to do addtional things on top of > PSCI for example. > > > If DT does provide PSCI description, then PSCI should be used. Doing > > otherwise is senseless. If PSCI is not to be used, then it should not > > be present in DT. > > You can't assume the DT and kernel are in-sync. For example, I've added > PSCI in the firmware and DTB (part of the firmware), but the highbank > kernel may or may not use it depending if I convert it. You are saying that we might want to run an old kernel, without PSCI support on a machine that exports a DT with PSCI? But in that case the platform smp_ops will just be chosen as usual. And if you have a new kernel with PSCI support on a machine that exports a DT with PSCI won't be OK to use the PSCI calls? The only problematic case would be if you actually need some platform specific calls to wrap around the PSCI firmare interface. But in that case what is the point of exporting the PSCI node on device tree anyway?
On Thu, 28 Mar 2013, Rob Herring wrote: > On 03/28/2013 09:51 AM, Nicolas Pitre wrote: > > On Thu, 28 Mar 2013, Stefano Stabellini wrote: > > > >> - the interface to bring up secondary cpus is different and based on > >> PSCI, in fact Xen is going to add a PSCI node to the device tree so that > >> Dom0 can use it. > >> > >> Oh wait, Dom0 is not going to use the PSCI interface even if the node is > >> present on device tree because it's going to prefer the platform smp_ops > >> instead. > > > > Waitaminute... I must have missed this part. > > > > Who said platform specific methods must be used in preference to PSCI? > > I did. Specifically, I said the platform should be allowed to provide > its own smp_ops. A platform may need to do addtional things on top of > PSCI for example. Then the platform should have its special hook that would override the default PSCI methods. But, by *default* the PSCI methods should be used if the related DT information is present. > > If DT does provide PSCI description, then PSCI should be used. Doing > > otherwise is senseless. If PSCI is not to be used, then it should not > > be present in DT. > > You can't assume the DT and kernel are in-sync. For example, I've added > PSCI in the firmware and DTB (part of the firmware), but the highbank > kernel may or may not use it depending if I convert it. If the kernel does not understand PSCI bindings in the DT, it naturally won't use PSCI, right? Conversely, if the firmware and therefore provided DT don't have PSCI, then the PSCI enabled kernel won't use PSCI either. So what is the problem? Nicolas
On Thu, Mar 28, 2013 at 03:39:42PM +0000, Nicolas Pitre wrote: > On Thu, 28 Mar 2013, Rob Herring wrote: > > > On 03/28/2013 09:51 AM, Nicolas Pitre wrote: > > > On Thu, 28 Mar 2013, Stefano Stabellini wrote: > > > > > >> - the interface to bring up secondary cpus is different and based on > > >> PSCI, in fact Xen is going to add a PSCI node to the device tree so that > > >> Dom0 can use it. > > >> > > >> Oh wait, Dom0 is not going to use the PSCI interface even if the node is > > >> present on device tree because it's going to prefer the platform smp_ops > > >> instead. > > > > > > Waitaminute... I must have missed this part. > > > > > > Who said platform specific methods must be used in preference to PSCI? > > > > I did. Specifically, I said the platform should be allowed to provide > > its own smp_ops. A platform may need to do addtional things on top of > > PSCI for example. > > Then the platform should have its special hook that would override the > default PSCI methods. But, by *default* the PSCI methods should be used > if the related DT information is present. I'm fine with a default PSCI-based implementation, providing that it's actually a layer between the smp ops and the psci code, not just glueing pointers together. KVM and Xen can then use the default implementation, but it does mean that they have to agree on that interface as it expands in the future. If Xen relies on the default ops in order to boot, then that's a good incentive not to deviate from them on the firmware side. Will
On Thu, 28 Mar 2013, Will Deacon wrote: > On Thu, Mar 28, 2013 at 03:39:42PM +0000, Nicolas Pitre wrote: > > On Thu, 28 Mar 2013, Rob Herring wrote: > > > > > On 03/28/2013 09:51 AM, Nicolas Pitre wrote: > > > > On Thu, 28 Mar 2013, Stefano Stabellini wrote: > > > > > > > >> - the interface to bring up secondary cpus is different and based on > > > >> PSCI, in fact Xen is going to add a PSCI node to the device tree so that > > > >> Dom0 can use it. > > > >> > > > >> Oh wait, Dom0 is not going to use the PSCI interface even if the node is > > > >> present on device tree because it's going to prefer the platform smp_ops > > > >> instead. > > > > > > > > Waitaminute... I must have missed this part. > > > > > > > > Who said platform specific methods must be used in preference to PSCI? > > > > > > I did. Specifically, I said the platform should be allowed to provide > > > its own smp_ops. A platform may need to do addtional things on top of > > > PSCI for example. > > > > Then the platform should have its special hook that would override the > > default PSCI methods. But, by *default* the PSCI methods should be used > > if the related DT information is present. > > I'm fine with a default PSCI-based implementation, providing that it's actually > a layer between the smp ops and the psci code, not just glueing pointers > together. Absolutely. Even in the MCPM case, the PSCI is wrapped into a MCPM backend while MCPM provides its own SMP ops methods. Nicolas
On Thu, 28 Mar 2013, Will Deacon wrote: > On Thu, Mar 28, 2013 at 03:39:42PM +0000, Nicolas Pitre wrote: > > On Thu, 28 Mar 2013, Rob Herring wrote: > > > > > On 03/28/2013 09:51 AM, Nicolas Pitre wrote: > > > > On Thu, 28 Mar 2013, Stefano Stabellini wrote: > > > > > > > >> - the interface to bring up secondary cpus is different and based on > > > >> PSCI, in fact Xen is going to add a PSCI node to the device tree so that > > > >> Dom0 can use it. > > > >> > > > >> Oh wait, Dom0 is not going to use the PSCI interface even if the node is > > > >> present on device tree because it's going to prefer the platform smp_ops > > > >> instead. > > > > > > > > Waitaminute... I must have missed this part. > > > > > > > > Who said platform specific methods must be used in preference to PSCI? > > > > > > I did. Specifically, I said the platform should be allowed to provide > > > its own smp_ops. A platform may need to do addtional things on top of > > > PSCI for example. > > > > Then the platform should have its special hook that would override the > > default PSCI methods. But, by *default* the PSCI methods should be used > > if the related DT information is present. > > I'm fine with a default PSCI-based implementation, providing that it's actually > a layer between the smp ops and the psci code, not just glueing pointers > together. OK. I'll rename virt_smp_ops and move it to its own file rather than psci.c and we'll take it from there. > KVM and Xen can then use the default implementation, but it does mean that > they have to agree on that interface as it expands in the future. If Xen > relies on the default ops in order to boot, then that's a good incentive not > to deviate from them on the firmware side. I am OK with that.
On 03/28/2013 10:39 AM, Nicolas Pitre wrote: > On Thu, 28 Mar 2013, Rob Herring wrote: > >> On 03/28/2013 09:51 AM, Nicolas Pitre wrote: >>> On Thu, 28 Mar 2013, Stefano Stabellini wrote: >>> >>>> - the interface to bring up secondary cpus is different and based on >>>> PSCI, in fact Xen is going to add a PSCI node to the device tree so that >>>> Dom0 can use it. >>>> >>>> Oh wait, Dom0 is not going to use the PSCI interface even if the node is >>>> present on device tree because it's going to prefer the platform smp_ops >>>> instead. >>> >>> Waitaminute... I must have missed this part. >>> >>> Who said platform specific methods must be used in preference to PSCI? >> >> I did. Specifically, I said the platform should be allowed to provide >> its own smp_ops. A platform may need to do addtional things on top of >> PSCI for example. > > Then the platform should have its special hook that would override the > default PSCI methods. But, by *default* the PSCI methods should be used > if the related DT information is present. Agreed. The special hook to override is setting mach desc smp_ops, right? >>> If DT does provide PSCI description, then PSCI should be used. Doing >>> otherwise is senseless. If PSCI is not to be used, then it should not >>> be present in DT. >> >> You can't assume the DT and kernel are in-sync. For example, I've added >> PSCI in the firmware and DTB (part of the firmware), but the highbank >> kernel may or may not use it depending if I convert it. > > If the kernel does not understand PSCI bindings in the DT, it naturally > won't use PSCI, right? Conversely, if the firmware and therefore > provided DT don't have PSCI, then the PSCI enabled kernel won't use PSCI > either. So what is the problem? I'm distinguishing the kernel in general is enabled for PSCI and a platform is enabled. The kernel may have PSCI smp_ops and the DTB may have PSCI data, but that alone should not make a platform use the default PSCI smp_ops. The platform has to make the decision and it cannot be just based on the platform's dtb having PSCI data. I have firmware (dtb is part of the firmware) with PSCI support and older firmware without. Old/existing kernels are fine on both firmware versions and don't use PSCI. New kernels with default PSCI ops should continue to work with both versions. When/If I convert highbank to use PSCI in the kernel, only then will new kernels require the new firmware version. Or perhaps I need to support both in the kernel for a while before ripping out non PSCI code. There is enough lag in distro kernels that I don't think this is necessary. Rob
On Thu, 28 Mar 2013, Rob Herring wrote: > On 03/28/2013 10:39 AM, Nicolas Pitre wrote: > > On Thu, 28 Mar 2013, Rob Herring wrote: > > > >> On 03/28/2013 09:51 AM, Nicolas Pitre wrote: > >>> On Thu, 28 Mar 2013, Stefano Stabellini wrote: > >>> > >>>> - the interface to bring up secondary cpus is different and based on > >>>> PSCI, in fact Xen is going to add a PSCI node to the device tree so that > >>>> Dom0 can use it. > >>>> > >>>> Oh wait, Dom0 is not going to use the PSCI interface even if the node is > >>>> present on device tree because it's going to prefer the platform smp_ops > >>>> instead. > >>> > >>> Waitaminute... I must have missed this part. > >>> > >>> Who said platform specific methods must be used in preference to PSCI? > >> > >> I did. Specifically, I said the platform should be allowed to provide > >> its own smp_ops. A platform may need to do addtional things on top of > >> PSCI for example. > > > > Then the platform should have its special hook that would override the > > default PSCI methods. But, by *default* the PSCI methods should be used > > if the related DT information is present. > > Agreed. The special hook to override is setting mach desc smp_ops, right? If you consider the mach smp_ops a platform specific override, then again PSCI and providing a PSCI node on DT doesn't solve the Xen problem at all. See above: Xen adds a PSCI node to DT, and Linux still does not use it. > >>> If DT does provide PSCI description, then PSCI should be used. Doing > >>> otherwise is senseless. If PSCI is not to be used, then it should not > >>> be present in DT. > >> > >> You can't assume the DT and kernel are in-sync. For example, I've added > >> PSCI in the firmware and DTB (part of the firmware), but the highbank > >> kernel may or may not use it depending if I convert it. > > > > If the kernel does not understand PSCI bindings in the DT, it naturally > > won't use PSCI, right? Conversely, if the firmware and therefore > > provided DT don't have PSCI, then the PSCI enabled kernel won't use PSCI > > either. So what is the problem? > > I'm distinguishing the kernel in general is enabled for PSCI and a > platform is enabled. The kernel may have PSCI smp_ops and the DTB may > have PSCI data, but that alone should not make a platform use the > default PSCI smp_ops. The platform has to make the decision and it > cannot be just based on the platform's dtb having PSCI data. I can see how this would give greater flexibility to firmware developers, but on the other hand it would limit the flexibility of the kernel. In fact, unfortunately, it is diametrically the opposite of what Xen needs. I would kindly ask the maintainers to let me know what direction I should take to move forward.
On 03/29/2013 08:22 AM, Stefano Stabellini wrote: > On Thu, 28 Mar 2013, Rob Herring wrote: >> On 03/28/2013 10:39 AM, Nicolas Pitre wrote: >>> On Thu, 28 Mar 2013, Rob Herring wrote: >>> >>>> On 03/28/2013 09:51 AM, Nicolas Pitre wrote: >>>>> On Thu, 28 Mar 2013, Stefano Stabellini wrote: >>>>> >>>>>> - the interface to bring up secondary cpus is different and based on >>>>>> PSCI, in fact Xen is going to add a PSCI node to the device tree so that >>>>>> Dom0 can use it. >>>>>> >>>>>> Oh wait, Dom0 is not going to use the PSCI interface even if the node is >>>>>> present on device tree because it's going to prefer the platform smp_ops >>>>>> instead. >>>>> >>>>> Waitaminute... I must have missed this part. >>>>> >>>>> Who said platform specific methods must be used in preference to PSCI? >>>> >>>> I did. Specifically, I said the platform should be allowed to provide >>>> its own smp_ops. A platform may need to do addtional things on top of >>>> PSCI for example. >>> >>> Then the platform should have its special hook that would override the >>> default PSCI methods. But, by *default* the PSCI methods should be used >>> if the related DT information is present. >> >> Agreed. The special hook to override is setting mach desc smp_ops, right? > > If you consider the mach smp_ops a platform specific override, then > again PSCI and providing a PSCI node on DT doesn't solve the Xen problem > at all. > > See above: Xen adds a PSCI node to DT, and Linux still does not use it. Okay, I see. I wasn't distinguishing Dom0 vs DomU cases. Is this really the only issue with having a platform run in Dom0? We expect all platforms to work without any modifications? I would think for more complex platforms there would be some other work needed. How is Xen going to really do physical cpu power management if a platform does not provide PSCI firmware? Are you going to pull all the platform specific code we have in the kernel now into Xen? If you make PSCI firmware a requirement for Xen, then you would only be modifying existing PSCI data to the DTB and the platform would be converted to use PSCI already. >>>>> If DT does provide PSCI description, then PSCI should be used. Doing >>>>> otherwise is senseless. If PSCI is not to be used, then it should not >>>>> be present in DT. >>>> >>>> You can't assume the DT and kernel are in-sync. For example, I've added >>>> PSCI in the firmware and DTB (part of the firmware), but the highbank >>>> kernel may or may not use it depending if I convert it. >>> >>> If the kernel does not understand PSCI bindings in the DT, it naturally >>> won't use PSCI, right? Conversely, if the firmware and therefore >>> provided DT don't have PSCI, then the PSCI enabled kernel won't use PSCI >>> either. So what is the problem? >> >> I'm distinguishing the kernel in general is enabled for PSCI and a >> platform is enabled. The kernel may have PSCI smp_ops and the DTB may >> have PSCI data, but that alone should not make a platform use the >> default PSCI smp_ops. The platform has to make the decision and it >> cannot be just based on the platform's dtb having PSCI data. > > I can see how this would give greater flexibility to firmware > developers, but on the other hand it would limit the flexibility of the > kernel. It limits the flexibility of the kernel too. If PSCI is present in the DTB, then the kernel must use it and the platform has no say? That's not flexible. > > In fact, unfortunately, it is diametrically the opposite of what Xen > needs. > > I would kindly ask the maintainers to let me know what direction I > should take to move forward. My argument is somewhat academic. I fully expect to convert highbank over to PSCI for 3.10 assuming this patch gets sorted out in time. So it is not really an issue for me. Adding Nico's smp_init function could give the platform flexibility later if needed. We're only talking about the behavior of a small portion of the patch, so I would go ahead with implementing the rest of the feedback. Rob
On Fri, 29 Mar 2013, Rob Herring wrote: > On 03/29/2013 08:22 AM, Stefano Stabellini wrote: > > On Thu, 28 Mar 2013, Rob Herring wrote: > >> On 03/28/2013 10:39 AM, Nicolas Pitre wrote: > >>> On Thu, 28 Mar 2013, Rob Herring wrote: > >>> > >>>> On 03/28/2013 09:51 AM, Nicolas Pitre wrote: > >>>>> On Thu, 28 Mar 2013, Stefano Stabellini wrote: > >>>>> > >>>>>> - the interface to bring up secondary cpus is different and based on > >>>>>> PSCI, in fact Xen is going to add a PSCI node to the device tree so that > >>>>>> Dom0 can use it. > >>>>>> > >>>>>> Oh wait, Dom0 is not going to use the PSCI interface even if the node is > >>>>>> present on device tree because it's going to prefer the platform smp_ops > >>>>>> instead. > >>>>> > >>>>> Waitaminute... I must have missed this part. > >>>>> > >>>>> Who said platform specific methods must be used in preference to PSCI? > >>>> > >>>> I did. Specifically, I said the platform should be allowed to provide > >>>> its own smp_ops. A platform may need to do addtional things on top of > >>>> PSCI for example. > >>> > >>> Then the platform should have its special hook that would override the > >>> default PSCI methods. But, by *default* the PSCI methods should be used > >>> if the related DT information is present. > >> > >> Agreed. The special hook to override is setting mach desc smp_ops, right? > > > > If you consider the mach smp_ops a platform specific override, then > > again PSCI and providing a PSCI node on DT doesn't solve the Xen problem > > at all. > > > > See above: Xen adds a PSCI node to DT, and Linux still does not use it. > > Okay, I see. I wasn't distinguishing Dom0 vs DomU cases. Is this really > the only issue with having a platform run in Dom0? We expect all > platforms to work without any modifications? I would think for more > complex platforms there would be some other work needed. No, I think that's all we need. At least I fail to see the need for something else at the moment :-) > How is Xen going to really do physical cpu power management if a > platform does not provide PSCI firmware? Are you going to pull all the > platform specific code we have in the kernel now into Xen? If you make > PSCI firmware a requirement for Xen, then you would only be modifying > existing PSCI data to the DTB and the platform would be converted to use > PSCI already. That is a good question. Realistically there are only few platforms that support ARMv7 virtualization extensions today, so we could have those platform specific functions in Xen. I am hopeful that in the future the new platforms that will support ARMv7 virtualization extensions and the coming ARMv8 platforms will also support PSCI. Otherwise yes, Xen will have to know about platform specific power management. > >>>>> If DT does provide PSCI description, then PSCI should be used. Doing > >>>>> otherwise is senseless. If PSCI is not to be used, then it should not > >>>>> be present in DT. > >>>> > >>>> You can't assume the DT and kernel are in-sync. For example, I've added > >>>> PSCI in the firmware and DTB (part of the firmware), but the highbank > >>>> kernel may or may not use it depending if I convert it. > >>> > >>> If the kernel does not understand PSCI bindings in the DT, it naturally > >>> won't use PSCI, right? Conversely, if the firmware and therefore > >>> provided DT don't have PSCI, then the PSCI enabled kernel won't use PSCI > >>> either. So what is the problem? > >> > >> I'm distinguishing the kernel in general is enabled for PSCI and a > >> platform is enabled. The kernel may have PSCI smp_ops and the DTB may > >> have PSCI data, but that alone should not make a platform use the > >> default PSCI smp_ops. The platform has to make the decision and it > >> cannot be just based on the platform's dtb having PSCI data. > > > > I can see how this would give greater flexibility to firmware > > developers, but on the other hand it would limit the flexibility of the > > kernel. > > It limits the flexibility of the kernel too. If PSCI is present in the > DTB, then the kernel must use it and the platform has no say? That's not > flexible. I am OK with having an override, but the override can't be hardcoded in the kernel sources. We could have a kernel command line parameter to disable PSCI for example. Something like no_psci. > > In fact, unfortunately, it is diametrically the opposite of what Xen > > needs. > > > > I would kindly ask the maintainers to let me know what direction I > > should take to move forward. > > My argument is somewhat academic. I fully expect to convert highbank > over to PSCI for 3.10 assuming this patch gets sorted out in time. So it > is not really an issue for me. Adding Nico's smp_init function could > give the platform flexibility later if needed. > > We're only talking about the behavior of a small portion of the patch, > so I would go ahead with implementing the rest of the feedback. OK, I'll do that. But I have to highlight that without those two lines in setup_arch we'll have broken SMP in Dom0.
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index ce0dbe7..ddef231 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -32,5 +32,14 @@ struct psci_operations { }; extern struct psci_operations psci_ops; +extern struct smp_operations psci_smp_ops; + +#ifdef CONFIG_ARM_PSCI +int psci_init(void); +bool psci_smp_available(void); +#else +static inline int psci_init(void) { return -ENODEV; } +static inline bool psci_smp_available(void) { return false; } +#endif #endif /* __ASM_ARM_PSCI_H */ diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c index 3653164..90f0839 100644 --- a/arch/arm/kernel/psci.c +++ b/arch/arm/kernel/psci.c @@ -16,6 +16,7 @@ #define pr_fmt(fmt) "psci: " fmt #include <linux/init.h> +#include <linux/irqchip/arm-gic.h> #include <linux/of.h> #include <asm/compiler.h> @@ -23,8 +24,9 @@ #include <asm/opcodes-sec.h> #include <asm/opcodes-virt.h> #include <asm/psci.h> +#include <asm/smp_plat.h> -struct psci_operations psci_ops; +extern void secondary_startup(void); static int (*invoke_psci_fn)(u32, u32, u32, u32); @@ -36,7 +38,11 @@ enum psci_function { PSCI_FN_MAX, }; -static u32 psci_function_id[PSCI_FN_MAX]; +struct psci_function_desc { + enum psci_function func; + bool valid; +}; +static struct psci_function_desc psci_function_id[PSCI_FN_MAX]; #define PSCI_RET_SUCCESS 0 #define PSCI_RET_EOPNOTSUPP -1 @@ -116,7 +122,10 @@ static int psci_cpu_suspend(struct psci_power_state state, int err; u32 fn, power_state; - fn = psci_function_id[PSCI_FN_CPU_SUSPEND]; + if (!psci_function_id[PSCI_FN_CPU_SUSPEND].valid) + return -ENOSYS; + + fn = psci_function_id[PSCI_FN_CPU_SUSPEND].func; power_state = psci_power_state_pack(state); err = invoke_psci_fn(fn, power_state, entry_point, 0); return psci_to_linux_errno(err); @@ -127,7 +136,10 @@ static int psci_cpu_off(struct psci_power_state state) int err; u32 fn, power_state; - fn = psci_function_id[PSCI_FN_CPU_OFF]; + if (!psci_function_id[PSCI_FN_CPU_OFF].valid) + return -ENOSYS; + + fn = psci_function_id[PSCI_FN_CPU_OFF].func; power_state = psci_power_state_pack(state); err = invoke_psci_fn(fn, power_state, 0, 0); return psci_to_linux_errno(err); @@ -138,7 +150,10 @@ static int psci_cpu_on(unsigned long cpuid, unsigned long entry_point) int err; u32 fn; - fn = psci_function_id[PSCI_FN_CPU_ON]; + if (!psci_function_id[PSCI_FN_CPU_ON].valid) + return -ENOSYS; + + fn = psci_function_id[PSCI_FN_CPU_ON].func; err = invoke_psci_fn(fn, cpuid, entry_point, 0); return psci_to_linux_errno(err); } @@ -148,25 +163,64 @@ static int psci_migrate(unsigned long cpuid) int err; u32 fn; - fn = psci_function_id[PSCI_FN_MIGRATE]; + if (!psci_function_id[PSCI_FN_MIGRATE].valid) + return -ENOSYS; + + fn = psci_function_id[PSCI_FN_MIGRATE].func; err = invoke_psci_fn(fn, cpuid, 0, 0); return psci_to_linux_errno(err); } +struct psci_operations psci_ops = { + .cpu_suspend = psci_cpu_suspend, + .cpu_off = psci_cpu_off, + .cpu_on = psci_cpu_on, + .migrate = psci_migrate, +}; + +#ifdef CONFIG_SMP +static void __init psci_smp_init_cpus(void) +{ +} + +static void __init psci_smp_prepare_cpus(unsigned int max_cpus) +{ +} + +static int __cpuinit psci_boot_secondary(unsigned int cpu, + struct task_struct *idle) +{ + return psci_cpu_on(cpu_logical_map(cpu), __pa(secondary_startup)); +} + +static void __cpuinit psci_secondary_init(unsigned int cpu) +{ + gic_secondary_init(0); +} + +struct smp_operations __initdata psci_smp_ops = { + .smp_init_cpus = psci_smp_init_cpus, + .smp_prepare_cpus = psci_smp_prepare_cpus, + .smp_secondary_init = psci_secondary_init, + .smp_boot_secondary = psci_boot_secondary, +}; +#endif + static const struct of_device_id psci_of_match[] __initconst = { { .compatible = "arm,psci", }, {}, }; -static int __init psci_init(void) +int __init psci_init(void) { struct device_node *np; const char *method; u32 id; + int rc = -EINVAL; np = of_find_matching_node(NULL, psci_of_match); if (!np) - return 0; + return -ENODEV; pr_info("probing function IDs from device-tree\n"); @@ -185,27 +239,34 @@ static int __init psci_init(void) } if (!of_property_read_u32(np, "cpu_suspend", &id)) { - psci_function_id[PSCI_FN_CPU_SUSPEND] = id; - psci_ops.cpu_suspend = psci_cpu_suspend; + psci_function_id[PSCI_FN_CPU_SUSPEND].func = id; + psci_function_id[PSCI_FN_CPU_SUSPEND].valid = true; } if (!of_property_read_u32(np, "cpu_off", &id)) { - psci_function_id[PSCI_FN_CPU_OFF] = id; - psci_ops.cpu_off = psci_cpu_off; + psci_function_id[PSCI_FN_CPU_OFF].func = id; + psci_function_id[PSCI_FN_CPU_OFF].valid = true; } if (!of_property_read_u32(np, "cpu_on", &id)) { - psci_function_id[PSCI_FN_CPU_ON] = id; - psci_ops.cpu_on = psci_cpu_on; + psci_function_id[PSCI_FN_CPU_ON].func = id; + psci_function_id[PSCI_FN_CPU_ON].valid = true; } if (!of_property_read_u32(np, "migrate", &id)) { - psci_function_id[PSCI_FN_MIGRATE] = id; - psci_ops.migrate = psci_migrate; + psci_function_id[PSCI_FN_MIGRATE].func = id; + psci_function_id[PSCI_FN_MIGRATE].valid = true; } + rc = 0; + out_put_node: of_node_put(np); - return 0; + return rc; +} + +bool __init psci_smp_available(void) +{ + /* is cpu_on available at least? */ + return psci_function_id[PSCI_FN_CPU_ON].valid; } -early_initcall(psci_init); diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index 3f6cbb2..c7e50dd 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -36,6 +36,7 @@ #include <asm/cputype.h> #include <asm/elf.h> #include <asm/procinfo.h> +#include <asm/psci.h> #include <asm/sections.h> #include <asm/setup.h> #include <asm/smp_plat.h> @@ -766,9 +767,13 @@ void __init setup_arch(char **cmdline_p) unflatten_device_tree(); arm_dt_init_cpu_maps(); + psci_init(); #ifdef CONFIG_SMP if (is_smp()) { - smp_set_ops(mdesc->smp); + if (psci_smp_available()) + smp_set_ops(&psci_smp_ops); + else + smp_set_ops(mdesc->smp); smp_init_cpus(); } #endif diff --git a/arch/arm/mach-virt/Makefile b/arch/arm/mach-virt/Makefile index 042afc1..7ddbfa6 100644 --- a/arch/arm/mach-virt/Makefile +++ b/arch/arm/mach-virt/Makefile @@ -3,4 +3,3 @@ # 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 deleted file mode 100644 index 8badaab..0000000 --- a/arch/arm/mach-virt/platsmp.c +++ /dev/null @@ -1,58 +0,0 @@ -/* - * Dummy Virtual Machine - does what it says on the tin. - * - * Copyright (C) 2012 ARM Ltd - * Author: Will Deacon <will.deacon@arm.com> - * - * 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 <linux/irqchip/arm-gic.h> - -#include <asm/psci.h> -#include <asm/smp_plat.h> - -extern void secondary_startup(void); - -static void __init virt_smp_init_cpus(void) -{ -} - -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; -} - -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 528c05e..c417752 100644 --- a/arch/arm/mach-virt/virt.c +++ b/arch/arm/mach-virt/virt.c @@ -44,12 +44,9 @@ static const char *virt_dt_match[] = { NULL }; -extern struct smp_operations virt_smp_ops; - DT_MACHINE_START(VIRT, "Dummy Virtual Machine") .init_irq = irqchip_init, .init_time = virt_timer_init, .init_machine = virt_init, - .smp = smp_ops(virt_smp_ops), .dt_compat = virt_dt_match, MACHINE_END
Check for the presence of PSCI before setting smp_ops, use PSCI if it is available. This is useful because at least when running on Xen it's possible to have a PSCI node for example on a Versatile Express or an Exynos5 machine. In these cases the PSCI SMP calls should be the ones to be called. Remove virt_smp_ops and platsmp.c from mach-virt because they aren't needed anymore. This patch was originally part of this series: http://marc.info/?l=linux-arm-kernel&m=136430903110734&w=2 I am keeping it separate now since it is the only non-obvious change and it is not Xen related. Changes in v3: - move the call to psci_init to setup_arch; - export psci_smp_ops from psci.h; - introduce psci_smp_available; - introduce stub functions for psci_init and psci_smp_available ifndef CONFIG_ARM_PSCI; - only compile psci_smp functions ifdef CONFIG_SMP. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> CC: will.deacon@arm.com CC: arnd@arndb.de CC: marc.zyngier@arm.com CC: linux@arm.linux.org.uk CC: nico@linaro.org --- arch/arm/include/asm/psci.h | 9 ++++ arch/arm/kernel/psci.c | 97 ++++++++++++++++++++++++++++++++++-------- arch/arm/kernel/setup.c | 7 +++- arch/arm/mach-virt/Makefile | 1 - arch/arm/mach-virt/platsmp.c | 58 ------------------------- arch/arm/mach-virt/virt.c | 3 - 6 files changed, 94 insertions(+), 81 deletions(-) delete mode 100644 arch/arm/mach-virt/platsmp.c