LinuxLists.cc - [PATCH v3] [RFC] arm: use PSCI if available

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 14:55:18

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 16:20:37

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 16:23:34

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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.

2013-03-27 16:34:04

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 16:35:52

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 17:06:25

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 17:10:33

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

On Wed, 27 Mar 2013, Rob Herring wrote:
> On 03/27/2013 11:23 AM, Stefano Stabellini wrote:
> > 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.

Well, that is the one additional problem that we have on Xen.

On x86 Xen replaces a lot of core native function calls with its own
implementations (see paravirt_ops).
On ARM we only need *one* set of calls: the smp_ops calls.

So if we don't want to give priority to PSCI over the platform smp_ops,
then we need a simple workaround just for Xen in common code like the
one appended below.
Not pretty, but at least small:

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 3f6cbb2..08cf7e0 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -43,6 +43,8 @@
#include <asm/cacheflush.h>
#include <asm/cachetype.h>
#include <asm/tlbflush.h>
+#include <xen/xen.h>
+#include <asm/xen/hypervisor.h>

#include <asm/prom.h>
#include <asm/mach/arch.h>
@@ -766,9 +768,13 @@ void __init setup_arch(char **cmdline_p)
unflatten_device_tree();

arm_dt_init_cpu_maps();
+ xen_early_init();
#ifdef CONFIG_SMP
if (is_smp()) {
- smp_set_ops(mdesc->smp);
+ if (xen_domain())
+ smp_set_ops(&xen_smp_ops);
+ else
+ smp_set_ops(mdesc->smp);
smp_init_cpus();
}
#endif

2013-03-27 17:23:29

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 17:24:56

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

On Wed, 27 Mar 2013, Stefano Stabellini wrote:

> On Wed, 27 Mar 2013, Rob Herring wrote:
> > On 03/27/2013 11:23 AM, Stefano Stabellini wrote:
> > > 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.
>
> Well, that is the one additional problem that we have on Xen.
>
> On x86 Xen replaces a lot of core native function calls with its own
> implementations (see paravirt_ops).
> On ARM we only need *one* set of calls: the smp_ops calls.
>
> So if we don't want to give priority to PSCI over the platform smp_ops,
> then we need a simple workaround just for Xen in common code like the
> one appended below.
> Not pretty, but at least small:
[...]

What about the patch below that I'm carying in my MCPM branch
which has been posted here already:

From: Jon Medhurst <[email protected]>
Date: Thu, 13 Dec 2012 13:23:13 +0000
Subject: [PATCH] ARM: Enable selection of SMP operations at boot time

Add a new 'smp_init' hook to machine_desc so platforms can specify a
function to be used to setup smp ops instead of having a statically
defined value.

Signed-off-by: Jon Medhurst <[email protected]>
Signed-off-by: Nicolas Pitre <[email protected]>
Reviewed-by: Santosh Shilimkar <[email protected]>

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 308ad7d6f9..c01bf53b85 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -16,8 +16,10 @@ struct pt_regs;
struct smp_operations;
#ifdef CONFIG_SMP
#define smp_ops(ops) (&(ops))
+#define smp_init_ops(ops) (&(ops))
#else
#define smp_ops(ops) (struct smp_operations *)NULL
+#define smp_init_ops(ops) (void (*)(void))NULL
#endif

struct machine_desc {
@@ -41,6 +43,7 @@ struct machine_desc {
unsigned char reserve_lp2 :1; /* never has lp2 */
char restart_mode; /* default restart mode */
struct smp_operations *smp; /* SMP operations */
+ void (*smp_init)(void);
void (*fixup)(struct tag *, char **,
struct meminfo *);
void (*reserve)(void);/* reserve mem blocks */
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 3f6cbb2e3e..41edca8582 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -768,7 +768,10 @@ void __init setup_arch(char **cmdline_p)
arm_dt_init_cpu_maps();
#ifdef CONFIG_SMP
if (is_smp()) {
- smp_set_ops(mdesc->smp);
+ if(mdesc->smp_init)
+ (*mdesc->smp_init)();
+ else
+ smp_set_ops(mdesc->smp);
smp_init_cpus();
}
#endif

2013-03-27 17:45:34

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

On 03/27/2013 12:10 PM, Stefano Stabellini wrote:
> On Wed, 27 Mar 2013, Rob Herring wrote:
>> On 03/27/2013 11:23 AM, Stefano Stabellini wrote:
>>> 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.
>
> Well, that is the one additional problem that we have on Xen.
>
> On x86 Xen replaces a lot of core native function calls with its own
> implementations (see paravirt_ops).
> On ARM we only need *one* set of calls: the smp_ops calls.
>
> So if we don't want to give priority to PSCI over the platform smp_ops,
> then we need a simple workaround just for Xen in common code like the
> one appended below.
> Not pretty, but at least small:
>
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 3f6cbb2..08cf7e0 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -43,6 +43,8 @@
> #include <asm/cacheflush.h>
> #include <asm/cachetype.h>
> #include <asm/tlbflush.h>
> +#include <xen/xen.h>
> +#include <asm/xen/hypervisor.h>
>
> #include <asm/prom.h>
> #include <asm/mach/arch.h>
> @@ -766,9 +768,13 @@ void __init setup_arch(char **cmdline_p)
> unflatten_device_tree();
>
> arm_dt_init_cpu_maps();
> + xen_early_init();
> #ifdef CONFIG_SMP
> if (is_smp()) {
> - smp_set_ops(mdesc->smp);
> + if (xen_domain())
> + smp_set_ops(&xen_smp_ops);
> + else
> + smp_set_ops(mdesc->smp);

No, I was thinking in the case of Xen and mach-virt, you would not set
mdesc->smp. So you would have something like this:

if (mdesc->smp)
smp_set_ops(mdesc->smp);
else
smp_set_ops(&psci_smp_ops);

Rob

2013-03-27 17:51:09

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 18:03:52

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

On Wednesday 27 March 2013, Rob Herring wrote:
> No, I was thinking in the case of Xen and mach-virt, you would not set
> mdesc->smp. So you would have something like this:
>
> if (mdesc->smp)
> smp_set_ops(mdesc->smp);
> else
> smp_set_ops(&psci_smp_ops);

The case that Stefano is interested in if obviously other platforms
that can either run as Dom0 under Xen with psci_smp_ops or natively
with their own smp_ops. A similar case would be a platform that
may implement psci using smc when run in secure mode but provide
its own smp_ops when run natively.

In both cases, it would be simpler to use psci if available but
fall back to mdesc->smp otherwise.

Arnd

2013-03-27 18:12:33

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 18:14:54

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

On Wed, 27 Mar 2013, Arnd Bergmann wrote:
> On Wednesday 27 March 2013, Rob Herring wrote:
> > No, I was thinking in the case of Xen and mach-virt, you would not set
> > mdesc->smp. So you would have something like this:
> >
> > if (mdesc->smp)
> > smp_set_ops(mdesc->smp);
> > else
> > smp_set_ops(&psci_smp_ops);
>
> The case that Stefano is interested in if obviously other platforms
> that can either run as Dom0 under Xen with psci_smp_ops or natively
> with their own smp_ops. A similar case would be a platform that
> may implement psci using smc when run in secure mode but provide
> its own smp_ops when run natively.

That's correct.

> In both cases, it would be simpler to use psci if available but
> fall back to mdesc->smp otherwise.

I agree.

2013-03-27 18:22:22

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

On Wed, 27 Mar 2013, Nicolas Pitre wrote:
> On Wed, 27 Mar 2013, Stefano Stabellini wrote:
>
> > On Wed, 27 Mar 2013, Rob Herring wrote:
> > > On 03/27/2013 11:23 AM, Stefano Stabellini wrote:
> > > > 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.
> >
> > Well, that is the one additional problem that we have on Xen.
> >
> > On x86 Xen replaces a lot of core native function calls with its own
> > implementations (see paravirt_ops).
> > On ARM we only need *one* set of calls: the smp_ops calls.
> >
> > So if we don't want to give priority to PSCI over the platform smp_ops,
> > then we need a simple workaround just for Xen in common code like the
> > one appended below.
> > Not pretty, but at least small:
> [...]
>
> What about the patch below that I'm carying in my MCPM branch
> which has been posted here already:

It's a step in the right direction but it still wouldn't solve the
problem.
Let me describe the scenario again:

- we have a versatile express machine with Xen running on it (or an
exynos5, etc.);
- Xen boots Linux as Dom0, passing a versatile express device tree with
the Xen hypervisor node added to it;
- Linux is booting, using the versatile express device tree for hardware
discovery;
- Linux can access the hardware described in the device tree because Xen
remapped the MMIO regions appropriately;
- Linux needs to bring secondary cpus up, but Xen only exports a PSCI
interface, so the native versatile express smp_ops don't work;
- Linux needs to detect that PSCI is available (or Xen is available with
its set of xen_smp_ops) and preferable on this platform and uses it
instead of the versatile express smp_ops.

> From: Jon Medhurst <[email protected]>
> Date: Thu, 13 Dec 2012 13:23:13 +0000
> Subject: [PATCH] ARM: Enable selection of SMP operations at boot time
>
> Add a new 'smp_init' hook to machine_desc so platforms can specify a
> function to be used to setup smp ops instead of having a statically
> defined value.
>
> Signed-off-by: Jon Medhurst <[email protected]>
> Signed-off-by: Nicolas Pitre <[email protected]>
> Reviewed-by: Santosh Shilimkar <[email protected]>
>
> diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
> index 308ad7d6f9..c01bf53b85 100644
> --- a/arch/arm/include/asm/mach/arch.h
> +++ b/arch/arm/include/asm/mach/arch.h
> @@ -16,8 +16,10 @@ struct pt_regs;
> struct smp_operations;
> #ifdef CONFIG_SMP
> #define smp_ops(ops) (&(ops))
> +#define smp_init_ops(ops) (&(ops))
> #else
> #define smp_ops(ops) (struct smp_operations *)NULL
> +#define smp_init_ops(ops) (void (*)(void))NULL
> #endif
>
> struct machine_desc {
> @@ -41,6 +43,7 @@ struct machine_desc {
> unsigned char reserve_lp2 :1; /* never has lp2 */
> char restart_mode; /* default restart mode */
> struct smp_operations *smp; /* SMP operations */
> + void (*smp_init)(void);
> void (*fixup)(struct tag *, char **,
> struct meminfo *);
> void (*reserve)(void);/* reserve mem blocks */
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 3f6cbb2e3e..41edca8582 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -768,7 +768,10 @@ void __init setup_arch(char **cmdline_p)
> arm_dt_init_cpu_maps();
> #ifdef CONFIG_SMP
> if (is_smp()) {
> - smp_set_ops(mdesc->smp);
> + if(mdesc->smp_init)
> + (*mdesc->smp_init)();
> + else
> + smp_set_ops(mdesc->smp);
> smp_init_cpus();
> }
> #endif
>

2013-03-27 19:10:32

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-27 19:14:25

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-28 12:48:26

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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.

2013-03-28 14:51:15

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-28 15:04:09

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2013-03-28 15:36:56

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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?

2013-03-28 15:39:47

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-28 16:00:56

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-28 16:06:56

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-28 16:20:19

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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.

2013-03-28 18:38:24

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-29 13:22:45

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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.

2013-03-29 13:54:14

[permalink] [raw]

Subject: Re: [PATCH v3] [RFC] arm: use PSCI if available

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

2013-03-29 14:47:44