powerpc/64s: fix idle wakeup potential to clobber registers

Submitted by Nicholas Piggin on March 17, 2017, 5:13 a.m.

Details

Message ID 20170317051320.7069-1-npiggin@gmail.com
State Accepted
Headers show

Commit Message

Nicholas Piggin March 17, 2017, 5:13 a.m.
We concluded there may be a window where the idle wakeup code could
get to pnv_wakeup_tb_loss (which clobbers non-volatile GPRs), but the
hardware may set SRR1[46:47] to 01b (no state loss) which would
result in the wakeup code failing to restore non-volatile GPRs.

I was not able to trigger this condition with trivial tests on
real hardware or simulator, but the ISA (at least 2.07) seems to
allow for it, and Gautham says that it can happen if there is an
exception pending when the sleep/winkle instruction is executed.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/idle_book3s.S | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

Gautham R. Shenoy March 17, 2017, 9:47 a.m.
Hi ,


On Fri, Mar 17, 2017 at 03:13:20PM +1000, Nicholas Piggin wrote:
> We concluded there may be a window where the idle wakeup code could
> get to pnv_wakeup_tb_loss (which clobbers non-volatile GPRs), but the
> hardware may set SRR1[46:47] to 01b (no state loss) which would
> result in the wakeup code failing to restore non-volatile GPRs.
> 
> I was not able to trigger this condition with trivial tests on
> real hardware or simulator, but the ISA (at least 2.07) seems to
> allow for it, and Gautham says that it can happen if there is an
> exception pending when the sleep/winkle instruction is executed.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

This fix should go into stable v4.8,v4.9 and v4.10.

Prior to commit 83289f909a72 ("powerpc/powernv: Rename idle_power7.S
to idle_book3s.S"), pnv_wakeup_tb_loss was explicitly restoring all
the GPRs to the saved value. So we are good for all the previous
kernel versions.

> ---
>  arch/powerpc/kernel/idle_book3s.S | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> index 995728736677..6fd08219248d 100644
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -449,9 +449,23 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
>  _GLOBAL(pnv_wakeup_tb_loss)
>  	ld	r1,PACAR1(r13)
>  	/*
> -	 * Before entering any idle state, the NVGPRs are saved in the stack
> -	 * and they are restored before switching to the process context. Hence
> -	 * until they are restored, they are free to be used.
> +	 * Before entering any idle state, the NVGPRs are saved in the stack.
> +	 * If there was a state loss, or PACA_NAPSTATELOST was set, then the
> +	 * NVGPRs are restored. If we are here, it is likely that state is lost,
> +	 * but not guaranteed -- neither ISA207 nor ISA300 tests to reach
> +	 * here are the same as the test to restore NVGPRS:
> +	 * PACA_THREAD_IDLE_STATE test for ISA207, PSSCR test for ISA300,
> +	 * and SRR1 test for restoring NVGPRs.
> +	 *
> +	 * We are about to clobber NVGPRs now, so set NAPSTATELOST to
> +	 * guarantee they will always be restored. This might be tightened
> +	 * with careful reading of specs (particularly for ISA300) but this
> +	 * is already a slow wakeup path and it's simpler to be safe.
> +	 */
> +	li	r0,1
> +	stb	r0,PACA_NAPSTATELOST(r13)
> +
> +	/*
>  	 *
>  	 * Save SRR1 and LR in NVGPRs as they might be clobbered in
>  	 * opal_call() (called in CHECK_HMI_INTERRUPT). SRR1 is required
> -- 
> 2.11.0
> 
--
Thanks and Regards
gautham.
Michael Ellerman March 21, 2017, 11:33 a.m.
On Fri, 2017-03-17 at 05:13:20 UTC, Nicholas Piggin wrote:
> We concluded there may be a window where the idle wakeup code could
> get to pnv_wakeup_tb_loss (which clobbers non-volatile GPRs), but the
> hardware may set SRR1[46:47] to 01b (no state loss) which would
> result in the wakeup code failing to restore non-volatile GPRs.
> 
> I was not able to trigger this condition with trivial tests on
> real hardware or simulator, but the ISA (at least 2.07) seems to
> allow for it, and Gautham says that it can happen if there is an
> exception pending when the sleep/winkle instruction is executed.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/6d98ce0be541d4a3cfbb52cd75072c

cheers

Patch hide | download patch | download mbox

diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 995728736677..6fd08219248d 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -449,9 +449,23 @@  END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
 _GLOBAL(pnv_wakeup_tb_loss)
 	ld	r1,PACAR1(r13)
 	/*
-	 * Before entering any idle state, the NVGPRs are saved in the stack
-	 * and they are restored before switching to the process context. Hence
-	 * until they are restored, they are free to be used.
+	 * Before entering any idle state, the NVGPRs are saved in the stack.
+	 * If there was a state loss, or PACA_NAPSTATELOST was set, then the
+	 * NVGPRs are restored. If we are here, it is likely that state is lost,
+	 * but not guaranteed -- neither ISA207 nor ISA300 tests to reach
+	 * here are the same as the test to restore NVGPRS:
+	 * PACA_THREAD_IDLE_STATE test for ISA207, PSSCR test for ISA300,
+	 * and SRR1 test for restoring NVGPRs.
+	 *
+	 * We are about to clobber NVGPRs now, so set NAPSTATELOST to
+	 * guarantee they will always be restored. This might be tightened
+	 * with careful reading of specs (particularly for ISA300) but this
+	 * is already a slow wakeup path and it's simpler to be safe.
+	 */
+	li	r0,1
+	stb	r0,PACA_NAPSTATELOST(r13)
+
+	/*
 	 *
 	 * Save SRR1 and LR in NVGPRs as they might be clobbered in
 	 * opal_call() (called in CHECK_HMI_INTERRUPT). SRR1 is required