| Message ID | 1445611610-12871-1-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 at 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; > } >
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
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
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
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
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
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
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; }
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(-)