linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] powernv: Move IDLE_STATE_ENTER_SEQ macro to cpuidle.h
@ 2016-02-25  4:54 Mahesh J Salgaonkar
  2016-02-25  4:55 ` [PATCH 2/3] powernv: Rename pSeries to powenv from machine_check_pSeries_early Mahesh J Salgaonkar
  2016-02-25  4:55 ` [PATCH 3/3] powernv: Fix MCE handler to avoid trashing CR0/CR1 registers Mahesh J Salgaonkar
  0 siblings, 2 replies; 5+ messages in thread
From: Mahesh J Salgaonkar @ 2016-02-25  4:54 UTC (permalink / raw)
  To: linuxppc-dev, Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
  Cc: Shreyas B. Prabhu, Vaidyanathan Srinivasan

From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>

Move IDLE_STATE_ENTER_SEQ macro to cpuidle.h so that MCE handler changes
in subsequent patch can use it.

No functionality change.

Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/cpuidle.h |   14 ++++++++++++++
 arch/powerpc/kernel/idle_power7.S  |   12 ------------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/cpuidle.h b/arch/powerpc/include/asm/cpuidle.h
index d2f99ca..5894ad6 100644
--- a/arch/powerpc/include/asm/cpuidle.h
+++ b/arch/powerpc/include/asm/cpuidle.h
@@ -17,4 +17,18 @@ extern u32 pnv_fastsleep_workaround_at_exit[];
 
 #endif
 
+/* Idle state entry routines */
+#ifdef	CONFIG_PPC_P7_NAP
+#define	IDLE_STATE_ENTER_SEQ(IDLE_INST)				\
+	/* Magic NAP/SLEEP/WINKLE mode enter sequence */	\
+	std	r0,0(r1);					\
+	ptesync;						\
+	ld	r0,0(r1);					\
+1:	cmp	cr0,r0,r0;					\
+	bne	1b;						\
+	IDLE_INST;						\
+	b	.
+#endif /* CONFIG_PPC_P7_NAP */
+
+
 #endif
diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S
index cedc4ef..cd43c90 100644
--- a/arch/powerpc/kernel/idle_power7.S
+++ b/arch/powerpc/kernel/idle_power7.S
@@ -37,18 +37,6 @@
 #define _WORT	GPR10
 #define _WORC	GPR11
 
-/* Idle state entry routines */
-
-#define	IDLE_STATE_ENTER_SEQ(IDLE_INST)				\
-	/* Magic NAP/SLEEP/WINKLE mode enter sequence */	\
-	std	r0,0(r1);					\
-	ptesync;						\
-	ld	r0,0(r1);					\
-1:	cmp	cr0,r0,r0;					\
-	bne	1b;						\
-	IDLE_INST;						\
-	b	.
-
 	.text
 
 /*

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/3] powernv: Rename pSeries to powenv from machine_check_pSeries_early.
  2016-02-25  4:54 [PATCH 1/3] powernv: Move IDLE_STATE_ENTER_SEQ macro to cpuidle.h Mahesh J Salgaonkar
@ 2016-02-25  4:55 ` Mahesh J Salgaonkar
  2016-02-25  4:55 ` [PATCH 3/3] powernv: Fix MCE handler to avoid trashing CR0/CR1 registers Mahesh J Salgaonkar
  1 sibling, 0 replies; 5+ messages in thread
From: Mahesh J Salgaonkar @ 2016-02-25  4:55 UTC (permalink / raw)
  To: linuxppc-dev, Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
  Cc: Shreyas B. Prabhu, Vaidyanathan Srinivasan

From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>

The routine machine_check_pSeries_early() is only used on powernv, not
pseries. Hence rename machine_check_pSeries_early to
machine_check_powernv_early.

Reported-by: Paul Mackerras <paulus@samba.org>
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/exceptions-64s.S |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 0607ae9..d4c99f0 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -189,7 +189,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
 #endif /* CONFIG_PPC_P7_NAP */
 	EXCEPTION_PROLOG_0(PACA_EXMC)
 BEGIN_FTR_SECTION
-	b	machine_check_pSeries_early
+	b	machine_check_powernv_early
 FTR_SECTION_ELSE
 	b	machine_check_pSeries_0
 ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
@@ -443,7 +443,7 @@ denorm_exception_hv:
 
 	.align	7
 	/* moved from 0x200 */
-machine_check_pSeries_early:
+machine_check_powernv_early:
 BEGIN_FTR_SECTION
 	EXCEPTION_PROLOG_1(PACA_EXMC, NOTEST, 0x200)
 	/*

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 3/3] powernv: Fix MCE handler to avoid trashing CR0/CR1 registers.
  2016-02-25  4:54 [PATCH 1/3] powernv: Move IDLE_STATE_ENTER_SEQ macro to cpuidle.h Mahesh J Salgaonkar
  2016-02-25  4:55 ` [PATCH 2/3] powernv: Rename pSeries to powenv from machine_check_pSeries_early Mahesh J Salgaonkar
@ 2016-02-25  4:55 ` Mahesh J Salgaonkar
  2016-02-25  7:05   ` Shreyas B Prabhu
  1 sibling, 1 reply; 5+ messages in thread
From: Mahesh J Salgaonkar @ 2016-02-25  4:55 UTC (permalink / raw)
  To: linuxppc-dev, Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
  Cc: Shreyas B. Prabhu, Vaidyanathan Srinivasan

From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>

The current implementation of MCE early handling modifies CR0/1 registers
without saving its old values. Fix this by moving early check for
powersaving mode to machine_check_handle_early().

The power architecture 2.06 or later allows the possibility of getting
machine check while in nap/sleep/winkle. The last bit of HSPRG0 is set
to 1, if thread is woken up from winkle. Hence, clear the last bit of
HSPRG0 (r13) before MCE handler starts using it as paca pointer.

Also, the current code always puts the thread into nap state irrespective
of whatever idle state it woke up from. Fix that by looking at
paca->thread_idle_state and put the thread back into same state where it
came from.

Reported-by: Paul Mackerras <paulus@samba.org>
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/exceptions-64s.S |   66 ++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index d4c99f0..7fa71e7 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -164,29 +164,14 @@ machine_check_pSeries_1:
 	 * vector
 	 */
 	SET_SCRATCH0(r13)		/* save r13 */
-#ifdef CONFIG_PPC_P7_NAP
-BEGIN_FTR_SECTION
-	/* Running native on arch 2.06 or later, check if we are
-	 * waking up from nap. We only handle no state loss and
-	 * supervisor state loss. We do -not- handle hypervisor
-	 * state loss at this time.
+	/*
+	 * Running native on arch 2.06 or later, we may wakeup from winkle
+	 * inside machine check. If yes, then last bit of HSPGR0 would be set
+	 * to 1. Hence clear it unconditionally.
 	 */
-	mfspr	r13,SPRN_SRR1
-	rlwinm.	r13,r13,47-31,30,31
-	OPT_GET_SPR(r13, SPRN_CFAR, CPU_FTR_CFAR)
-	beq	9f
-
-	mfspr	r13,SPRN_SRR1
-	rlwinm.	r13,r13,47-31,30,31
-	/* waking up from powersave (nap) state */
-	cmpwi	cr1,r13,2
-	/* Total loss of HV state is fatal. let's just stay stuck here */
-	OPT_GET_SPR(r13, SPRN_CFAR, CPU_FTR_CFAR)
-	bgt	cr1,.
-9:
-	OPT_SET_SPR(r13, SPRN_CFAR, CPU_FTR_CFAR)
-END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
-#endif /* CONFIG_PPC_P7_NAP */
+	GET_PACA(r13)
+	clrrdi	r13,r13,1
+	SET_PACA(r13)
 	EXCEPTION_PROLOG_0(PACA_EXMC)
 BEGIN_FTR_SECTION
 	b	machine_check_powernv_early
@@ -1362,25 +1347,52 @@ machine_check_handle_early:
 	 * Check if thread was in power saving mode. We come here when any
 	 * of the following is true:
 	 * a. thread wasn't in power saving mode
-	 * b. thread was in power saving mode with no state loss or
-	 *    supervisor state loss
+	 * b. thread was in power saving mode with no state loss,
+	 *    supervisor state loss or hypervisor state loss.
 	 *
-	 * Go back to nap again if (b) is true.
+	 * Go back to nap/sleep/winkle mode again if (b) is true.
 	 */
 	rlwinm.	r11,r12,47-31,30,31	/* Was it in power saving mode? */
 	beq	4f			/* No, it wasn;t */
 	/* Thread was in power saving mode. Go back to nap again. */
 	cmpwi	r11,2
-	bne	3f
-	/* Supervisor state loss */
+	blt	3f
+	/* Supervisor/Hypervisor state loss loss */
 	li	r0,1
 	stb	r0,PACA_NAPSTATELOST(r13)
 3:	bl	machine_check_queue_event
 	MACHINE_CHECK_HANDLER_WINDUP
 	GET_PACA(r13)
+	/*
+	 * Check what idle state this CPU was in and go back to same mode
+	 * again.
+	 */
+	lbz	r3,PACA_THREAD_IDLE_STATE(r13)
+	cmpwi	r3,PNV_THREAD_NAP
+	bgt	1f
 	ld	r1,PACAR1(r13)
 	li	r3,PNV_THREAD_NAP
 	b	power7_enter_nap_mode
+	/* No return */
+1:
+	cmpwi	r3,PNV_THREAD_SLEEP
+	bgt	2f
+	IDLE_STATE_ENTER_SEQ(PPC_SLEEP)
+	/* No return */
+
+2:
+	/*
+	 * Go back to winkle. Please note that this thread was woken up in
+	 * machine check from winkle and have not restored the per-subcore
+	 * state. Hence before going back to winkle, set last bit of HSPGR0
+	 * to 1. This will make sure that if this thread gets woken up
+	 * again at reset vector 0x100 then it will get chance to restore
+	 * the subcore state.
+	 */
+	ori	r13,r13,1
+	SET_PACA(r13)
+	IDLE_STATE_ENTER_SEQ(PPC_WINKLE)
+	/* No return */
 4:
 #endif
 	/*

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/3] powernv: Fix MCE handler to avoid trashing CR0/CR1 registers.
  2016-02-25  4:55 ` [PATCH 3/3] powernv: Fix MCE handler to avoid trashing CR0/CR1 registers Mahesh J Salgaonkar
@ 2016-02-25  7:05   ` Shreyas B Prabhu
  2016-02-25 11:24     ` Mahesh Jagannath Salgaonkar
  0 siblings, 1 reply; 5+ messages in thread
From: Shreyas B Prabhu @ 2016-02-25  7:05 UTC (permalink / raw)
  To: Mahesh J Salgaonkar, linuxppc-dev, Benjamin Herrenschmidt,
	Paul Mackerras, Michael Ellerman



On 02/25/2016 10:25 AM, Mahesh J Salgaonkar wrote:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> 
> The current implementation of MCE early handling modifies CR0/1 registers
> without saving its old values. Fix this by moving early check for
> powersaving mode to machine_check_handle_early().
> 
> The power architecture 2.06 or later allows the possibility of getting
> machine check while in nap/sleep/winkle. The last bit of HSPRG0 is set
> to 1, if thread is woken up from winkle. Hence, clear the last bit of
> HSPRG0 (r13) before MCE handler starts using it as paca pointer.
> 
> Also, the current code always puts the thread into nap state irrespective
> of whatever idle state it woke up from. Fix that by looking at
> paca->thread_idle_state and put the thread back into same state where it
> came from.
> 
> Reported-by: Paul Mackerras <paulus@samba.org>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/exceptions-64s.S |   66 ++++++++++++++++++++--------------
>  1 file changed, 39 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index d4c99f0..7fa71e7 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -164,29 +164,14 @@ machine_check_pSeries_1:
>  	 * vector
>  	 */
>  	SET_SCRATCH0(r13)		/* save r13 */
> -#ifdef CONFIG_PPC_P7_NAP
> -BEGIN_FTR_SECTION
> -	/* Running native on arch 2.06 or later, check if we are
> -	 * waking up from nap. We only handle no state loss and
> -	 * supervisor state loss. We do -not- handle hypervisor
> -	 * state loss at this time.
> +	/*
> +	 * Running native on arch 2.06 or later, we may wakeup from winkle
> +	 * inside machine check. If yes, then last bit of HSPGR0 would be set
> +	 * to 1. Hence clear it unconditionally.
>  	 */
> -	mfspr	r13,SPRN_SRR1
> -	rlwinm.	r13,r13,47-31,30,31
> -	OPT_GET_SPR(r13, SPRN_CFAR, CPU_FTR_CFAR)
> -	beq	9f
> -
> -	mfspr	r13,SPRN_SRR1
> -	rlwinm.	r13,r13,47-31,30,31
> -	/* waking up from powersave (nap) state */
> -	cmpwi	cr1,r13,2
> -	/* Total loss of HV state is fatal. let's just stay stuck here */
> -	OPT_GET_SPR(r13, SPRN_CFAR, CPU_FTR_CFAR)
> -	bgt	cr1,.
> -9:
> -	OPT_SET_SPR(r13, SPRN_CFAR, CPU_FTR_CFAR)
> -END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
> -#endif /* CONFIG_PPC_P7_NAP */
> +	GET_PACA(r13)
> +	clrrdi	r13,r13,1
> +	SET_PACA(r13)
>  	EXCEPTION_PROLOG_0(PACA_EXMC)
>  BEGIN_FTR_SECTION
>  	b	machine_check_powernv_early
> @@ -1362,25 +1347,52 @@ machine_check_handle_early:
>  	 * Check if thread was in power saving mode. We come here when any
>  	 * of the following is true:
>  	 * a. thread wasn't in power saving mode
> -	 * b. thread was in power saving mode with no state loss or
> -	 *    supervisor state loss
> +	 * b. thread was in power saving mode with no state loss,
> +	 *    supervisor state loss or hypervisor state loss.
>  	 *
> -	 * Go back to nap again if (b) is true.
> +	 * Go back to nap/sleep/winkle mode again if (b) is true.
>  	 */
>  	rlwinm.	r11,r12,47-31,30,31	/* Was it in power saving mode? */
>  	beq	4f			/* No, it wasn;t */
>  	/* Thread was in power saving mode. Go back to nap again. */
>  	cmpwi	r11,2
> -	bne	3f
> -	/* Supervisor state loss */
> +	blt	3f
> +	/* Supervisor/Hypervisor state loss loss */

loss repeated twice in comment
>  	li	r0,1
>  	stb	r0,PACA_NAPSTATELOST(r13)
>  3:	bl	machine_check_queue_event
>  	MACHINE_CHECK_HANDLER_WINDUP
>  	GET_PACA(r13)
> +	/*
> +	 * Check what idle state this CPU was in and go back to same mode
> +	 * again.
> +	 */
> +	lbz	r3,PACA_THREAD_IDLE_STATE(r13)
> +	cmpwi	r3,PNV_THREAD_NAP
> +	bgt	1f
>  	ld	r1,PACAR1(r13)
>  	li	r3,PNV_THREAD_NAP
>  	b	power7_enter_nap_mode

You could call IDLE_STATE_ENTER_SEQ(PPC_NAP) here to keep it consistent
with what you do for sleep and winkle below. power7_enter_nap_mode is
only setting couple of PACA flags which are anyway set in your case.

Also what is the MSR at this point? I don't foresee any issue as long as
we are in real mode. That said, ISA says SF, HV and ME bits should be 1,
RI can be 0,1 and rest have to be 0 before entering low power mode.
> +	/* No return */
> +1:
> +	cmpwi	r3,PNV_THREAD_SLEEP
> +	bgt	2f
> +	IDLE_STATE_ENTER_SEQ(PPC_SLEEP)
> +	/* No return */
> +
> +2:
> +	/*
> +	 * Go back to winkle. Please note that this thread was woken up in
> +	 * machine check from winkle and have not restored the per-subcore
> +	 * state. Hence before going back to winkle, set last bit of HSPGR0
> +	 * to 1. This will make sure that if this thread gets woken up
> +	 * again at reset vector 0x100 then it will get chance to restore
> +	 * the subcore state.
> +	 */
> +	ori	r13,r13,1
> +	SET_PACA(r13)

Note PACA_THREAD_IDLE_STATE(r13) showing PNV_THREAD_WINKLE doesn't
necessarily mean we woke up from winkle. It only means we requested for
it. So here is possible you are setting last bit of HSPRG0 unnecessarily
in some cases. I don't believe this has any harmful effects though. It
will only result unnecessarily restoring SLB and few SPRs next time the
thread wakes up in reset vector.

> +	IDLE_STATE_ENTER_SEQ(PPC_WINKLE)
> +	/* No return */
>  4:
>  #endif
>  	/*
> 
Overall this patch looks good to me.

Reviewed-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/3] powernv: Fix MCE handler to avoid trashing CR0/CR1 registers.
  2016-02-25  7:05   ` Shreyas B Prabhu
@ 2016-02-25 11:24     ` Mahesh Jagannath Salgaonkar
  0 siblings, 0 replies; 5+ messages in thread
From: Mahesh Jagannath Salgaonkar @ 2016-02-25 11:24 UTC (permalink / raw)
  To: Shreyas B Prabhu, linuxppc-dev, Benjamin Herrenschmidt,
	Paul Mackerras, Michael Ellerman

On 02/25/2016 12:35 PM, Shreyas B Prabhu wrote:
> 
> 
> On 02/25/2016 10:25 AM, Mahesh J Salgaonkar wrote:
>> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>>
>> The current implementation of MCE early handling modifies CR0/1 registers
>> without saving its old values. Fix this by moving early check for
>> powersaving mode to machine_check_handle_early().
>>
>> The power architecture 2.06 or later allows the possibility of getting
>> machine check while in nap/sleep/winkle. The last bit of HSPRG0 is set
>> to 1, if thread is woken up from winkle. Hence, clear the last bit of
>> HSPRG0 (r13) before MCE handler starts using it as paca pointer.
>>
>> Also, the current code always puts the thread into nap state irrespective
>> of whatever idle state it woke up from. Fix that by looking at
>> paca->thread_idle_state and put the thread back into same state where it
>> came from.
>>
>> Reported-by: Paul Mackerras <paulus@samba.org>
>> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/kernel/exceptions-64s.S |   66 ++++++++++++++++++++--------------
>>  1 file changed, 39 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index d4c99f0..7fa71e7 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -164,29 +164,14 @@ machine_check_pSeries_1:
>>  	 * vector
>>  	 */
>>  	SET_SCRATCH0(r13)		/* save r13 */
>> -#ifdef CONFIG_PPC_P7_NAP
>> -BEGIN_FTR_SECTION
>> -	/* Running native on arch 2.06 or later, check if we are
>> -	 * waking up from nap. We only handle no state loss and
>> -	 * supervisor state loss. We do -not- handle hypervisor
>> -	 * state loss at this time.
>> +	/*
>> +	 * Running native on arch 2.06 or later, we may wakeup from winkle
>> +	 * inside machine check. If yes, then last bit of HSPGR0 would be set
>> +	 * to 1. Hence clear it unconditionally.
>>  	 */
>> -	mfspr	r13,SPRN_SRR1
>> -	rlwinm.	r13,r13,47-31,30,31
>> -	OPT_GET_SPR(r13, SPRN_CFAR, CPU_FTR_CFAR)
>> -	beq	9f
>> -
>> -	mfspr	r13,SPRN_SRR1
>> -	rlwinm.	r13,r13,47-31,30,31
>> -	/* waking up from powersave (nap) state */
>> -	cmpwi	cr1,r13,2
>> -	/* Total loss of HV state is fatal. let's just stay stuck here */
>> -	OPT_GET_SPR(r13, SPRN_CFAR, CPU_FTR_CFAR)
>> -	bgt	cr1,.
>> -9:
>> -	OPT_SET_SPR(r13, SPRN_CFAR, CPU_FTR_CFAR)
>> -END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
>> -#endif /* CONFIG_PPC_P7_NAP */
>> +	GET_PACA(r13)
>> +	clrrdi	r13,r13,1
>> +	SET_PACA(r13)
>>  	EXCEPTION_PROLOG_0(PACA_EXMC)
>>  BEGIN_FTR_SECTION
>>  	b	machine_check_powernv_early
>> @@ -1362,25 +1347,52 @@ machine_check_handle_early:
>>  	 * Check if thread was in power saving mode. We come here when any
>>  	 * of the following is true:
>>  	 * a. thread wasn't in power saving mode
>> -	 * b. thread was in power saving mode with no state loss or
>> -	 *    supervisor state loss
>> +	 * b. thread was in power saving mode with no state loss,
>> +	 *    supervisor state loss or hypervisor state loss.
>>  	 *
>> -	 * Go back to nap again if (b) is true.
>> +	 * Go back to nap/sleep/winkle mode again if (b) is true.
>>  	 */
>>  	rlwinm.	r11,r12,47-31,30,31	/* Was it in power saving mode? */
>>  	beq	4f			/* No, it wasn;t */
>>  	/* Thread was in power saving mode. Go back to nap again. */
>>  	cmpwi	r11,2
>> -	bne	3f
>> -	/* Supervisor state loss */
>> +	blt	3f
>> +	/* Supervisor/Hypervisor state loss loss */
> 
> loss repeated twice in comment
>>  	li	r0,1
>>  	stb	r0,PACA_NAPSTATELOST(r13)
>>  3:	bl	machine_check_queue_event
>>  	MACHINE_CHECK_HANDLER_WINDUP
>>  	GET_PACA(r13)
>> +	/*
>> +	 * Check what idle state this CPU was in and go back to same mode
>> +	 * again.
>> +	 */
>> +	lbz	r3,PACA_THREAD_IDLE_STATE(r13)
>> +	cmpwi	r3,PNV_THREAD_NAP
>> +	bgt	1f
>>  	ld	r1,PACAR1(r13)
>>  	li	r3,PNV_THREAD_NAP
>>  	b	power7_enter_nap_mode
> 
> You could call IDLE_STATE_ENTER_SEQ(PPC_NAP) here to keep it consistent
> with what you do for sleep and winkle below. power7_enter_nap_mode is
> only setting couple of PACA flags which are anyway set in your case.

Yup I could do that. Will fix it in v2.

> 
> Also what is the MSR at this point? I don't foresee any issue as long as
> we are in real mode. That said, ISA says SF, HV and ME bits should be 1,
> RI can be 0,1 and rest have to be 0 before entering low power mode.

Machine check interrupts are taken in real mode and by the time we come
here ME bit is already set to 1. So we are good.

>> +	/* No return */
>> +1:
>> +	cmpwi	r3,PNV_THREAD_SLEEP
>> +	bgt	2f
>> +	IDLE_STATE_ENTER_SEQ(PPC_SLEEP)
>> +	/* No return */
>> +
>> +2:
>> +	/*
>> +	 * Go back to winkle. Please note that this thread was woken up in
>> +	 * machine check from winkle and have not restored the per-subcore
>> +	 * state. Hence before going back to winkle, set last bit of HSPGR0
>> +	 * to 1. This will make sure that if this thread gets woken up
>> +	 * again at reset vector 0x100 then it will get chance to restore
>> +	 * the subcore state.
>> +	 */
>> +	ori	r13,r13,1
>> +	SET_PACA(r13)
> 
> Note PACA_THREAD_IDLE_STATE(r13) showing PNV_THREAD_WINKLE doesn't
> necessarily mean we woke up from winkle. It only means we requested for
> it. So here is possible you are setting last bit of HSPRG0 unnecessarily
> in some cases. 

Agree. But unlike reset vector, MCE can come with valid values in
CR[0-7] and we can't afford to trash them. For checking whether r13 has
its last bit set or not, it will end up trashing CR0 at least.

I don't believe this has any harmful effects though. It
> will only result unnecessarily restoring SLB and few SPRs next time the
> thread wakes up in reset vector.

I think we should be ok with that.

> 
>> +	IDLE_STATE_ENTER_SEQ(PPC_WINKLE)
>> +	/* No return */
>>  4:
>>  #endif
>>  	/*
>> 
> Overall this patch looks good to me.
> 
> Reviewed-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> 

Thanks for your review.

-Mahesh.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-02-25 11:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-25  4:54 [PATCH 1/3] powernv: Move IDLE_STATE_ENTER_SEQ macro to cpuidle.h Mahesh J Salgaonkar
2016-02-25  4:55 ` [PATCH 2/3] powernv: Rename pSeries to powenv from machine_check_pSeries_early Mahesh J Salgaonkar
2016-02-25  4:55 ` [PATCH 3/3] powernv: Fix MCE handler to avoid trashing CR0/CR1 registers Mahesh J Salgaonkar
2016-02-25  7:05   ` Shreyas B Prabhu
2016-02-25 11:24     ` Mahesh Jagannath Salgaonkar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).