| Message ID | 1364308875-26484-6-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 26 March 2013, 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. Very nice, I had a similar idea but had not gotten around to write a patch. This fits in nicely with my plans to make all fields of machine_desc optional. > void __init smp_set_ops(struct smp_operations *ops) > { > - if (ops) > + int rc = -ENODEV; > +#ifdef CONFIG_ARM_PSCI > + rc = psci_init(&smp_ops); > +#endif > + if (rc && ops) > smp_ops = *ops; > }; Could you move this into the caller, i.e. setup_arch() so we call smp_set_ops either for psci_smp_ops or for machine_desc->smp? Arnd
Hi Stefano, On Tue, Mar 26, 2013 at 02:41:15PM +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 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, > +}; Whilst I like the idea of this, I don't think things will pan out this nicely in practice. There will almost always be a level of indirection required between the internal Linux SMP operations and the expectations of the PSCI firmware, whether this is in CPU numbering or other, platform-specific fields in various parameters. Tying these two things together like this confuses the layering in my opinion and will likely lead to potentially subtle breakages if platforms start trying to adopt this. If this can indeed work for the virtual platforms (Xen and KVM), then I think it would be better expressed using `virt' smp_ops, which map directly to PSCI, rather than putting them here. Even then, it's tying KVM and Xen together on the firmware side of things... Will
On Tue, 26 Mar 2013, Arnd Bergmann wrote: > On Tuesday 26 March 2013, 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. > > Very nice, I had a similar idea but had not gotten around to write a patch. > This fits in nicely with my plans to make all fields of machine_desc optional. > > > void __init smp_set_ops(struct smp_operations *ops) > > { > > - if (ops) > > + int rc = -ENODEV; > > +#ifdef CONFIG_ARM_PSCI > > + rc = psci_init(&smp_ops); > > +#endif > > + if (rc && ops) > > smp_ops = *ops; > > }; > > Could you move this into the caller, i.e. setup_arch() so we call smp_set_ops > either for psci_smp_ops or for machine_desc->smp? Sure, I can do that.
On Tue, 26 Mar 2013, Will Deacon wrote: > Hi Stefano, > > On Tue, Mar 26, 2013 at 02:41:15PM +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 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, > > +}; > > Whilst I like the idea of this, I don't think things will pan out this > nicely in practice. There will almost always be a level of indirection > required between the internal Linux SMP operations and the expectations of > the PSCI firmware, whether this is in CPU numbering or other, > platform-specific fields in various parameters. > > Tying these two things together like this confuses the layering in my > opinion and will likely lead to potentially subtle breakages if platforms > start trying to adopt this. What you are saying is that psci could either be used directly, like we are doing, or it could just be the base of some higher level platform specific smp_ops. Honestly I think that psci is already high level enough that I would worry if somebody started to wrap it around something else. However we still support that use case just fine: they can just avoid having a psci node on device tree and just keep using their machine specific smp_ops. It's up to them really. They can even base the implementation of their smp_ops on the current psci code, in order to facilitate that I could get rid of psci_ops (which initialization is based on device tree) and export the psci_cpu_* functions instead, so that they can be called directly by other smp_ops. > If this can indeed work for the virtual platforms (Xen and KVM), then I > think it would be better expressed using `virt' smp_ops, which map directly > to PSCI, rather than putting them here. Even then, it's tying KVM and Xen > together on the firmware side of things... Keep in mind that dom0 on Xen boots as a native machine (versatile express or exynos5 for example) with a Xen hypervisor node on it. We would need to find a way to override the default machine smp_ops with a set of xen_smp_ops early at boot. I don't like this option very much, I think is fragile.
On Tue, Mar 26, 2013 at 03:25:55PM +0000, Stefano Stabellini wrote: > On Tue, 26 Mar 2013, Will Deacon wrote: > > On Tue, Mar 26, 2013 at 02:41:15PM +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, > > > +}; > > > > Whilst I like the idea of this, I don't think things will pan out this > > nicely in practice. There will almost always be a level of indirection > > required between the internal Linux SMP operations and the expectations of > > the PSCI firmware, whether this is in CPU numbering or other, > > platform-specific fields in various parameters. > > > > Tying these two things together like this confuses the layering in my > > opinion and will likely lead to potentially subtle breakages if platforms > > start trying to adopt this. > > What you are saying is that psci could either be used directly, like we > are doing, or it could just be the base of some higher level platform > specific smp_ops. > > Honestly I think that psci is already high level enough that I would > worry if somebody started to wrap it around something else. I don't agree. PSCI is a low-level firmware interface, which will naturally have implementation-specific parts to it. For example, many of the CPU power functions have platform-specific state ID parameters which we can't just ignore. Furthermore, the method by which a CPU is identified needn't match the value in our logical map. The purpose of the PSCI code in Linux is to provide a basic abstraction on top of this interface, so that platforms can incorporate them into higher-level power management functions, which themselves might be plumbed into smp_operations structures. > However we still support that use case just fine: they can just avoid > having a psci node on device tree and just keep using their machine > specific smp_ops. It's up to them really. Why get rid of the node? That's there to initialise the PSCI backend accordingly and shouldn't have anything to do with SMP. > They can even base the implementation of their smp_ops on the current > psci code, in order to facilitate that I could get rid of psci_ops > (which initialization is based on device tree) and export the psci_cpu_* > functions instead, so that they can be called directly by other smp_ops. Again, I think this destroys the layering. The whole point is that the PSCI functions are called from within something that understands precisely how to talk to the firmware and what it is capable of. > > If this can indeed work for the virtual platforms (Xen and KVM), then I > > think it would be better expressed using `virt' smp_ops, which map directly > > to PSCI, rather than putting them here. Even then, it's tying KVM and Xen > > together on the firmware side of things... > > Keep in mind that dom0 on Xen boots as a native machine (versatile > express or exynos5 for example) with a Xen hypervisor node on it. We > would need to find a way to override the default machine smp_ops with > a set of xen_smp_ops early at boot. > I don't like this option very much, I think is fragile. Why can't dom0 use whatever smp ops the native machine would use? Will
On Tuesday 26 March 2013, Will Deacon wrote: > > They can even base the implementation of their smp_ops on the current > > psci code, in order to facilitate that I could get rid of psci_ops > > (which initialization is based on device tree) and export the psci_cpu_* > > functions instead, so that they can be called directly by other smp_ops. > > Again, I think this destroys the layering. The whole point is that the PSCI > functions are called from within something that understands precisely how to > talk to the firmware and what it is capable of. Right, we probably the psci smp ops to be separate from the rest of the psci code, but I also think that Stefano is right that we should let any platform use the psci smp ops if possible, rather than having to implement their own. > > > If this can indeed work for the virtual platforms (Xen and KVM), then I > > > think it would be better expressed using `virt' smp_ops, which map directly > > > to PSCI, rather than putting them here. Even then, it's tying KVM and Xen > > > together on the firmware side of things... > > > > Keep in mind that dom0 on Xen boots as a native machine (versatile > > express or exynos5 for example) with a Xen hypervisor node on it. We > > would need to find a way to override the default machine smp_ops with > > a set of xen_smp_ops early at boot. > > I don't like this option very much, I think is fragile. > > Why can't dom0 use whatever smp ops the native machine would use? The part that I'm most interested in is making it possible for a platform to kill off its native smp ops in the kernel by implementing the psci ops. I think it's a good strategy to use psci by default if both platform and psci implementations are available. Arnd
On Tue, 26 Mar 2013, Arnd Bergmann wrote: > On Tuesday 26 March 2013, Will Deacon wrote: > > > They can even base the implementation of their smp_ops on the current > > > psci code, in order to facilitate that I could get rid of psci_ops > > > (which initialization is based on device tree) and export the psci_cpu_* > > > functions instead, so that they can be called directly by other smp_ops. > > > > Again, I think this destroys the layering. The whole point is that the PSCI > > functions are called from within something that understands precisely how to > > talk to the firmware and what it is capable of. > > Right, we probably the psci smp ops to be separate from the rest of the psci > code, but I also think that Stefano is right that we should let any platform > use the psci smp ops if possible, rather than having to implement their own. [...] > The part that I'm most interested in is making it possible for a platform > to kill off its native smp ops in the kernel by implementing the psci > ops. I think it's a good strategy to use psci by default if both > platform and psci implementations are available. I fully agree, and Xen would use it as is. If the psci node on DT only signifies the presence of psci, not that it should be used for smp_ops, then we need another DT node or property to say "this machine uses smp_psci_ops".
On Tue, 26 Mar 2013, Will Deacon wrote: > On Tue, Mar 26, 2013 at 03:25:55PM +0000, Stefano Stabellini wrote: > > On Tue, 26 Mar 2013, Will Deacon wrote: > > > On Tue, Mar 26, 2013 at 02:41:15PM +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, > > > > +}; > > > > > > Whilst I like the idea of this, I don't think things will pan out this > > > nicely in practice. There will almost always be a level of indirection > > > required between the internal Linux SMP operations and the expectations of > > > the PSCI firmware, whether this is in CPU numbering or other, > > > platform-specific fields in various parameters. > > > > > > Tying these two things together like this confuses the layering in my > > > opinion and will likely lead to potentially subtle breakages if platforms > > > start trying to adopt this. > > > > What you are saying is that psci could either be used directly, like we > > are doing, or it could just be the base of some higher level platform > > specific smp_ops. > > > > Honestly I think that psci is already high level enough that I would > > worry if somebody started to wrap it around something else. > > I don't agree. PSCI is a low-level firmware interface, which will naturally > have implementation-specific parts to it. For example, many of the CPU power > functions have platform-specific state ID parameters which we can't just > ignore. Furthermore, the method by which a CPU is identified needn't match > the value in our logical map. The purpose of the PSCI code in Linux is to > provide a basic abstraction on top of this interface, so that platforms can > incorporate them into higher-level power management functions, which > themselves might be plumbed into smp_operations structures. Absolutely. PSCI is _not_ a Linux API. It is a firmware API. And remember that Linux has no stable API by design. So it is best to keep PSCI as one possible way to talk to the firmware, but a flexible shim layer (flexible as in "we can change its interface whenever we want to") around PSCI to provide a Linux API which also encompass all possible low-level implementations alternatives is a better idea. Nicolas
On Tue, 26 Mar 2013, Arnd Bergmann wrote: > On Tuesday 26 March 2013, Will Deacon wrote: > > > They can even base the implementation of their smp_ops on the current > > > psci code, in order to facilitate that I could get rid of psci_ops > > > (which initialization is based on device tree) and export the psci_cpu_* > > > functions instead, so that they can be called directly by other smp_ops. > > > > Again, I think this destroys the layering. The whole point is that the PSCI > > functions are called from within something that understands precisely how to > > talk to the firmware and what it is capable of. > > Right, we probably the psci smp ops to be separate from the rest of the psci > code, but I also think that Stefano is right that we should let any platform > use the psci smp ops if possible, rather than having to implement their own. Oh absolutely. It is always best to use an existing standard. But PSCI probably won't be the only firmware interface standard. It therefore shouldn't be used as the Linux internal interface model. Nicolas
On Tue, 26 Mar 2013, Nicolas Pitre wrote: > On Tue, 26 Mar 2013, Arnd Bergmann wrote: > > > On Tuesday 26 March 2013, Will Deacon wrote: > > > > They can even base the implementation of their smp_ops on the current > > > > psci code, in order to facilitate that I could get rid of psci_ops > > > > (which initialization is based on device tree) and export the psci_cpu_* > > > > functions instead, so that they can be called directly by other smp_ops. > > > > > > Again, I think this destroys the layering. The whole point is that the PSCI > > > functions are called from within something that understands precisely how to > > > talk to the firmware and what it is capable of. > > > > Right, we probably the psci smp ops to be separate from the rest of the psci > > code, but I also think that Stefano is right that we should let any platform > > use the psci smp ops if possible, rather than having to implement their own. > > Oh absolutely. It is always best to use an existing standard. But PSCI > probably won't be the only firmware interface standard. It therefore > shouldn't be used as the Linux internal interface model. I am not proposing to use PSCI as an interal Linux API. I am proposing to use a set of PSCI based smp_ops (instead of the ones that come with machine_desc, if any) if a PSCI node is available on device tree. smp_ops remains the internal Linux API. I am also saying that we should let people reuse the PSCI functions in their own machine-specific smp_ops, if they want to.
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index ce0dbe7..e64ea84 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -32,5 +32,6 @@ struct psci_operations { }; extern struct psci_operations psci_ops; +int psci_init(struct smp_operations *ops); #endif /* __ASM_ARM_PSCI_H */ diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c index 3653164..6f66d93 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,7 +24,9 @@ #include <asm/opcodes-sec.h> #include <asm/opcodes-virt.h> #include <asm/psci.h> +#include <asm/smp_plat.h> +extern void secondary_startup(void); struct psci_operations psci_ops; static int (*invoke_psci_fn)(u32, u32, u32, u32); @@ -153,20 +156,50 @@ static int psci_migrate(unsigned long cpuid) return psci_to_linux_errno(err); } +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) +{ + if (psci_ops.cpu_on) + return psci_ops.cpu_on(cpu_logical_map(cpu), + __pa(secondary_startup)); + return -ENODEV; +} + +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, +}; + static const struct of_device_id psci_of_match[] __initconst = { { .compatible = "arm,psci", }, {}, }; -static int __init psci_init(void) +int __init psci_init(struct smp_operations *ops) { 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"); @@ -203,9 +236,10 @@ static int __init psci_init(void) psci_function_id[PSCI_FN_MIGRATE] = id; psci_ops.migrate = psci_migrate; } + *ops = psci_smp_ops; + rc = 0; out_put_node: of_node_put(np); - return 0; + return rc; } -early_initcall(psci_init); diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index 31644f1..09eda3c 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -38,6 +38,7 @@ #include <asm/pgtable.h> #include <asm/pgalloc.h> #include <asm/processor.h> +#include <asm/psci.h> #include <asm/sections.h> #include <asm/tlbflush.h> #include <asm/ptrace.h> @@ -74,7 +75,11 @@ static struct smp_operations smp_ops; void __init smp_set_ops(struct smp_operations *ops) { - if (ops) + int rc = -ENODEV; +#ifdef CONFIG_ARM_PSCI + rc = psci_init(&smp_ops); +#endif + if (rc && ops) smp_ops = *ops; }; 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. 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 | 1 + arch/arm/kernel/psci.c | 42 +++++++++++++++++++++++++++--- arch/arm/kernel/smp.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, 45 insertions(+), 67 deletions(-) delete mode 100644 arch/arm/mach-virt/platsmp.c