drivers: psci: make PSCI 1.0 functions initialization version dependent

PATCH New Lorenzo Pieralisi 4 participants 8 messages Oct 23, 2015 – Oct 26, 2015

Share & tools

Lorenzo Pieralisi

author

2015-10-23 14:46 UTC #0 lore raw

PATCH 1/1 drivers: psci: make PSCI 1.0 functions initialization version dependent

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 <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Olof Johansson <[email protected]>

Cc Mark Rutland <[email protected]>

---
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(-)

drivers/firmware/psci.c +4 -3

index 492db42..d24f35d 100644
@@ -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;
 }

Signature · 1 line

2.5.1

Sudeep Holla

2015-10-23 14:58 UTC #1 lore raw

PATCH 1/1 drivers: psci: make PSCI 1.0 functions initialization version dependent

Quoted 26 lines · 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.

Quoted 6 lines

[1] http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
Signed-off-by Lorenzo Pieralisi <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Sudeep Holla <[email protected]>

Looks good to me(however have alternate thought, see below)

Acked-by Sudeep Holla <[email protected]>

Quoted 31 lines

Cc Olof Johansson <[email protected]>
Cc Mark Rutland <[email protected]>
---
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(-)
drivers/firmware/psci.c +0 -0
index 492db42..d24f35d 100644

drivers/firmware/psci.c static int __init psci_probe(void) · line 350

Quoted 7 lines

drivers/firmware/psci.c +1 -3

@@ -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.

Quoted 7 lines

drivers/firmware/psci.c +3 -0

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

Signature · 2 lines

Regards,
Sudeep

Lorenzo Pieralisi

author

2015-10-23 16:00 UTC #2 lore raw

PATCH 1/1 drivers: psci: make PSCI 1.0 functions initialization version dependent

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

[...]

Quoted 4 lines

drivers/firmware/psci.c +0 -0

@@ -347,9 +347,10 @@ static int __init psci_probe(void)
 
 	psci_init_migrate();

Quoted 9 lines

drivers/firmware/psci.c static int __init psci_probe(void) · line 350
Quoted 4 lines
drivers/firmware/psci.c +1 -3
-	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

Kevin Hilman

2015-10-23 15:21 UTC #3 lore raw

PATCH 1/1 drivers: psci: make PSCI 1.0 functions initialization version dependent

In reply to Lorenzo Pieralisi

Quoted 44 lines · 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 <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Sudeep Holla <[email protected]>
Cc Olof Johansson <[email protected]>
Cc Mark Rutland <[email protected]>
---
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:

Quoted 7 lines

- 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 <[email protected]>

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

Thanks for the quick fix,

Kevin

Lorenzo Pieralisi

author

2015-10-23 16:02 UTC #4 lore raw

PATCH 1/1 drivers: psci: make PSCI 1.0 functions initialization version dependent

Quoted 1 line · Kevin Hilman

Lorenzo Pieralisi <[email protected]> writes:

[...]

Quoted 10 lines

Quoted 9 lines
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.

Quoted 7 lines

I tested manually on my local qemu that was failing without this patch, and it works:
Tested-by Kevin Hilman <[email protected]>
We'll add this onto arm-soc drivers/psci and then it will have a spin through kernelci.

Thank you very much !

Lorenzo

Kevin Hilman

2015-10-23 19:09 UTC #5 lore raw

PATCH 1/1 drivers: psci: make PSCI 1.0 functions initialization version dependent

In reply to Kevin Hilman

Quoted 54 lines · Kevin Hilman

Quoted 44 lines · 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 <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Sudeep Holla <[email protected]>
Cc Olof Johansson <[email protected]>
Cc Mark Rutland <[email protected]>
---
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:
Quoted 7 lines
- 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.

Quoted 7 lines

I tested manually on my local qemu that was failing without this patch, and it works:
Tested-by Kevin Hilman <[email protected]>
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

Olof Johansson

2015-10-23 16:59 UTC #6 lore raw

PATCH 1/1 drivers: psci: make PSCI 1.0 functions initialization version dependent

In reply to Lorenzo Pieralisi

Quoted 34 lines · 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 <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Sudeep Holla <[email protected]>
Cc Olof Johansson <[email protected]>
Cc Mark Rutland <[email protected]>

Quoted 7 lines

---
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

Lorenzo Pieralisi

author

2015-10-26 10:02 UTC #7 lore raw

PATCH 1/1 drivers: psci: make PSCI 1.0 functions initialization version dependent

Quoted 38 lines · Olof Johansson

Quoted 37 lines · 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 <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Sudeep Holla <[email protected]>
Cc Olof Johansson <[email protected]>
Cc Mark Rutland <[email protected]>
---
Arnd, Kevin, Olof,

Quoted 7 lines

Quoted 4 lines
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.

Quoted 3 lines

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