[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[Lorenzo Pieralisi]

The PSCI specifications [1] and the SMC calling convention mandate
that unimplemented functions ids must return NOT_SUPPORTED (0xffffffff)
if a function id is called but it is not implemented.

Consequently, PSCI 1.0 function ids that require the 1.0 PSCI_FEATURES
call to be initialized:

CPU_SUSPEND (psci_init_cpu_suspend())
SYSTEM_SUSPEND (psci_init_system_suspend())

call the PSCI_FEATURES function id independently of the detected
PSCI firmware version, since, if the PSCI_FEATURES function id is not
implemented, it must return NOT_SUPPORTED according to the PSCI
specifications, causing the initialization functions to fail as expected.

Some existing PSCI implementations (ie Qemu PSCI emulation), do not
comply with the SMC calling convention and fail if function ids that are
not implemented are called from the OS, causing boot failures.

To solve this issue, this patch adds code that checks the PSCI firmware
version before calling PSCI 1.0 initialization functions so that the
OS makes sure that it is calling 1.0 functions only if the firmware
version detected is 1.0 or greater, therefore avoiding PSCI calls
that are bound to fail and might cause system boot failures owing
to non-compliant PSCI firmware implementations.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Olof Johansson <olof@lixom.net>
Cc: Mark Rutland <mark.rutland@arm.com>
---
Arnd, Kevin, Olof,

this applies to current arm-soc drivers/psci branch, and solves the
issue Kevin detected through kernelci with Qemu emulation:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/380304.html

Tested on:

- Juno host
- AMD Seattle host
- kvmtool arm64 guest (on Juno arm64 defconfig host)
- Qemu x86 host (aarch64 emulation)

A run on kernelci and consequent tested-by tags would be much appreciated,
thanks for spotting this and for your help.

Thanks,
Lorenzo

 drivers/firmware/psci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 492db42..d24f35d 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -347,9 +347,10 @@ static int __init psci_probe(void)
 
 	psci_init_migrate();
 
-	psci_init_cpu_suspend();
-
-	psci_init_system_suspend();
+	if (PSCI_VERSION_MAJOR(ver) >= 1) {
+		psci_init_cpu_suspend();
+		psci_init_system_suspend();
+	}
 
 	return 0;
 }
-- 
2.5.1

[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[Sudeep Holla]

On 23/10/15 15:46, Lorenzo Pieralisi wrote:
> The PSCI specifications [1] and the SMC calling convention mandate
> that unimplemented functions ids must return NOT_SUPPORTED (0xffffffff)
> if a function id is called but it is not implemented.
>
> Consequently, PSCI 1.0 function ids that require the 1.0 PSCI_FEATURES
> call to be initialized:
>
> CPU_SUSPEND (psci_init_cpu_suspend())
> SYSTEM_SUSPEND (psci_init_system_suspend())
>
> call the PSCI_FEATURES function id independently of the detected
> PSCI firmware version, since, if the PSCI_FEATURES function id is not
> implemented, it must return NOT_SUPPORTED according to the PSCI
> specifications, causing the initialization functions to fail as expected.
>
> Some existing PSCI implementations (ie Qemu PSCI emulation), do not
> comply with the SMC calling convention and fail if function ids that are
> not implemented are called from the OS, causing boot failures.
>
> To solve this issue, this patch adds code that checks the PSCI firmware
> version before calling PSCI 1.0 initialization functions so that the
> OS makes sure that it is calling 1.0 functions only if the firmware
> version detected is 1.0 or greater, therefore avoiding PSCI calls
> that are bound to fail and might cause system boot failures owing
> to non-compliant PSCI firmware implementations.
>
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Looks good to me(however have alternate thought, see below)
Acked-by: Sudeep Holla <sudeep.holla@arm.com>

> Cc: Olof Johansson <olof@lixom.net>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
> Arnd, Kevin, Olof,
>
> this applies to current arm-soc drivers/psci branch, and solves the
> issue Kevin detected through kernelci with Qemu emulation:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/380304.html
>
> Tested on:
>
> - Juno host
> - AMD Seattle host
> - kvmtool arm64 guest (on Juno arm64 defconfig host)
> - Qemu x86 host (aarch64 emulation)
>
> A run on kernelci and consequent tested-by tags would be much appreciated,
> thanks for spotting this and for your help.
>
> Thanks,
> Lorenzo
>
>   drivers/firmware/psci.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index 492db42..d24f35d 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -347,9 +347,10 @@ static int __init psci_probe(void)
>
>   	psci_init_migrate();
>
> -	psci_init_cpu_suspend();
> -
> -	psci_init_system_suspend();
> +	if (PSCI_VERSION_MAJOR(ver) >= 1) {

Alternatively we can just add this check in psci_features function and
return PSCI_RET_NOT_SUPPORTED if PSCI_VERSION_MAJOR(ver) < 1. New
callers of psci_features are also protected and need not add checks@
call sites. Thoughts ?

I am fine with this patch as is and don't have a strong opinion.

> +		psci_init_cpu_suspend();
> +		psci_init_system_suspend();
> +	}
>
>   	return 0;
>   }
>

-- 
Regards,
Sudeep

[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[Lorenzo Pieralisi]

On Fri, Oct 23, 2015 at 03:58:43PM +0100, Sudeep Holla wrote:

[...]

> >@@ -347,9 +347,10 @@ static int __init psci_probe(void)
> >
> >  	psci_init_migrate();
> >
> >-	psci_init_cpu_suspend();
> >-
> >-	psci_init_system_suspend();
> >+	if (PSCI_VERSION_MAJOR(ver) >= 1) {
> 
> Alternatively we can just add this check in psci_features function and
> return PSCI_RET_NOT_SUPPORTED if PSCI_VERSION_MAJOR(ver) < 1. New
> callers of psci_features are also protected and need not add checks
> at call sites. Thoughts ?

The resulting behaviour given the current code would be equivalent,
but I think you have a point, we can always refactor this code later
as a clean-up, I just prevented calling initialization functions
that have 0 chances to succeed on pre 1.0 firmware anyway (but do have
have a chance to fail as the breakage showed).

Thanks,
Lorenzo

[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[Kevin Hilman]

Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:

> The PSCI specifications [1] and the SMC calling convention mandate
> that unimplemented functions ids must return NOT_SUPPORTED (0xffffffff)
> if a function id is called but it is not implemented.
>
> Consequently, PSCI 1.0 function ids that require the 1.0 PSCI_FEATURES
> call to be initialized:
>
> CPU_SUSPEND (psci_init_cpu_suspend())
> SYSTEM_SUSPEND (psci_init_system_suspend())
>
> call the PSCI_FEATURES function id independently of the detected
> PSCI firmware version, since, if the PSCI_FEATURES function id is not
> implemented, it must return NOT_SUPPORTED according to the PSCI
> specifications, causing the initialization functions to fail as expected.
>
> Some existing PSCI implementations (ie Qemu PSCI emulation), do not
> comply with the SMC calling convention and fail if function ids that are
> not implemented are called from the OS, causing boot failures.
>
> To solve this issue, this patch adds code that checks the PSCI firmware
> version before calling PSCI 1.0 initialization functions so that the
> OS makes sure that it is calling 1.0 functions only if the firmware
> version detected is 1.0 or greater, therefore avoiding PSCI calls
> that are bound to fail and might cause system boot failures owing
> to non-compliant PSCI firmware implementations.
>
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
> Arnd, Kevin, Olof,
>
> this applies to current arm-soc drivers/psci branch, and solves the
> issue Kevin detected through kernelci with Qemu emulation:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/380304.html
>
> Tested on:
>
> - Juno host
> - AMD Seattle host
> - kvmtool arm64 guest (on Juno arm64 defconfig host)
> - Qemu x86 host (aarch64 emulation)
>
> A run on kernelci and consequent tested-by tags would be much appreciated,
> thanks for spotting this and for your help.

I tested manually on my local qemu that was failing without this patch,
and it works:

Tested-by: Kevin Hilman <khilman@linaro.org>

We'll add this onto arm-soc drivers/psci and then it will have a spin through
kernelci.

Thanks for the quick fix,

Kevin

[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[Lorenzo Pieralisi]

On Fri, Oct 23, 2015 at 08:21:44AM -0700, Kevin Hilman wrote:
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:

[...]

> > Tested on:
> >
> > - Juno host
> > - AMD Seattle host
> > - kvmtool arm64 guest (on Juno arm64 defconfig host)
> > - Qemu x86 host (aarch64 emulation)
> >
> > A run on kernelci and consequent tested-by tags would be much appreciated,
> > thanks for spotting this and for your help.
> 
> I tested manually on my local qemu that was failing without this patch,
> and it works:
> 
> Tested-by: Kevin Hilman <khilman@linaro.org>
> 
> We'll add this onto arm-soc drivers/psci and then it will have a spin through
> kernelci.

Thank you very much !

Lorenzo

[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[Kevin Hilman]

On Fri, Oct 23, 2015 at 8:21 AM, Kevin Hilman <khilman@kernel.org> wrote:
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
>
>> The PSCI specifications [1] and the SMC calling convention mandate
>> that unimplemented functions ids must return NOT_SUPPORTED (0xffffffff)
>> if a function id is called but it is not implemented.
>>
>> Consequently, PSCI 1.0 function ids that require the 1.0 PSCI_FEATURES
>> call to be initialized:
>>
>> CPU_SUSPEND (psci_init_cpu_suspend())
>> SYSTEM_SUSPEND (psci_init_system_suspend())
>>
>> call the PSCI_FEATURES function id independently of the detected
>> PSCI firmware version, since, if the PSCI_FEATURES function id is not
>> implemented, it must return NOT_SUPPORTED according to the PSCI
>> specifications, causing the initialization functions to fail as expected.
>>
>> Some existing PSCI implementations (ie Qemu PSCI emulation), do not
>> comply with the SMC calling convention and fail if function ids that are
>> not implemented are called from the OS, causing boot failures.
>>
>> To solve this issue, this patch adds code that checks the PSCI firmware
>> version before calling PSCI 1.0 initialization functions so that the
>> OS makes sure that it is calling 1.0 functions only if the firmware
>> version detected is 1.0 or greater, therefore avoiding PSCI calls
>> that are bound to fail and might cause system boot failures owing
>> to non-compliant PSCI firmware implementations.
>>
>> [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
>>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Kevin Hilman <khilman@kernel.org>
>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>> Cc: Olof Johansson <olof@lixom.net>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> ---
>> Arnd, Kevin, Olof,
>>
>> this applies to current arm-soc drivers/psci branch, and solves the
>> issue Kevin detected through kernelci with Qemu emulation:
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/380304.html
>>
>> Tested on:
>>
>> - Juno host
>> - AMD Seattle host
>> - kvmtool arm64 guest (on Juno arm64 defconfig host)
>> - Qemu x86 host (aarch64 emulation)
>>
>> A run on kernelci and consequent tested-by tags would be much appreciated,
>> thanks for spotting this and for your help.
>
> I tested manually on my local qemu that was failing without this patch,
> and it works:
>
> Tested-by: Kevin Hilman <khilman@linaro.org>
>
> We'll add this onto arm-soc drivers/psci and then it will have a spin through
> kernelci.

FYI... This has now been merged into arm-soc/for-next and had a spin
through kernelci.org, and the arm64 qemu results are passing:
http://kernelci.org/boot/all/job/arm-soc/kernel/v4.3-rc5-571-g9b588fdb667c/

Thanks,

Kevin

[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[Olof Johansson]

On Fri, Oct 23, 2015 at 03:46:50PM +0100, Lorenzo Pieralisi wrote:
> The PSCI specifications [1] and the SMC calling convention mandate
> that unimplemented functions ids must return NOT_SUPPORTED (0xffffffff)
> if a function id is called but it is not implemented.
> 
> Consequently, PSCI 1.0 function ids that require the 1.0 PSCI_FEATURES
> call to be initialized:
> 
> CPU_SUSPEND (psci_init_cpu_suspend())
> SYSTEM_SUSPEND (psci_init_system_suspend())
> 
> call the PSCI_FEATURES function id independently of the detected
> PSCI firmware version, since, if the PSCI_FEATURES function id is not
> implemented, it must return NOT_SUPPORTED according to the PSCI
> specifications, causing the initialization functions to fail as expected.
> 
> Some existing PSCI implementations (ie Qemu PSCI emulation), do not
> comply with the SMC calling convention and fail if function ids that are
> not implemented are called from the OS, causing boot failures.
> 
> To solve this issue, this patch adds code that checks the PSCI firmware
> version before calling PSCI 1.0 initialization functions so that the
> OS makes sure that it is calling 1.0 functions only if the firmware
> version detected is 1.0 or greater, therefore avoiding PSCI calls
> that are bound to fail and might cause system boot failures owing
> to non-compliant PSCI firmware implementations.
> 
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
> Arnd, Kevin, Olof,
> 
> this applies to current arm-soc drivers/psci branch, and solves the
> issue Kevin detected through kernelci with Qemu emulation:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/380304.html

Bummer. It'd been nice to catch this before merging, especially since
the branch had been around a while.

Anyway, I applied this to a drivers/psci2 branch that builds on top of the
previous one. Please use psci2 as the base for other trees, so that mainline
won't regress in case they get merged before our stuff.


-Olof

[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[Lorenzo Pieralisi]

On Fri, Oct 23, 2015 at 09:59:02AM -0700, Olof Johansson wrote:
> On Fri, Oct 23, 2015 at 03:46:50PM +0100, Lorenzo Pieralisi wrote:
> > The PSCI specifications [1] and the SMC calling convention mandate
> > that unimplemented functions ids must return NOT_SUPPORTED (0xffffffff)
> > if a function id is called but it is not implemented.
> > 
> > Consequently, PSCI 1.0 function ids that require the 1.0 PSCI_FEATURES
> > call to be initialized:
> > 
> > CPU_SUSPEND (psci_init_cpu_suspend())
> > SYSTEM_SUSPEND (psci_init_system_suspend())
> > 
> > call the PSCI_FEATURES function id independently of the detected
> > PSCI firmware version, since, if the PSCI_FEATURES function id is not
> > implemented, it must return NOT_SUPPORTED according to the PSCI
> > specifications, causing the initialization functions to fail as expected.
> > 
> > Some existing PSCI implementations (ie Qemu PSCI emulation), do not
> > comply with the SMC calling convention and fail if function ids that are
> > not implemented are called from the OS, causing boot failures.
> > 
> > To solve this issue, this patch adds code that checks the PSCI firmware
> > version before calling PSCI 1.0 initialization functions so that the
> > OS makes sure that it is calling 1.0 functions only if the firmware
> > version detected is 1.0 or greater, therefore avoiding PSCI calls
> > that are bound to fail and might cause system boot failures owing
> > to non-compliant PSCI firmware implementations.
> > 
> > [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Kevin Hilman <khilman@kernel.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Olof Johansson <olof@lixom.net>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > ---
> > Arnd, Kevin, Olof,
> > 
> > this applies to current arm-soc drivers/psci branch, and solves the
> > issue Kevin detected through kernelci with Qemu emulation:
> > 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/380304.html
> 
> Bummer. It'd been nice to catch this before merging, especially since
> the branch had been around a while.

Agreed, sorry about that, I will talk to Kevin to make sure I am able
to run kernelci tests on branches aimed at upstreaming from now onwards.

> Anyway, I applied this to a drivers/psci2 branch that builds on top of the
> previous one. Please use psci2 as the base for other trees, so that mainline
> won't regress in case they get merged before our stuff.

Thanks, I will talk to Daniel for the PSCI CPUidle patches, but I think
they will have to wait, let's merge this drivers/psci2 branch and I will
rebase the patches going via other trees at -rc1 for the next merge window.

Thanks a lot,
Lorenzo