[PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Nicholas Piggin]

This patch does a couple of things. First of all, powernv immediately
explodes when running a relocated kernel, because the system reset
exception for handling sleeps does not do correct relocated branches.

Secondly, the sleep handling code trashes the condition and cfar
registers, which we would like to preserve for debugging purposes (for
non-sleep case exception).

This patch changes the exception to use the standard format that saves
registers before any tests or branches are made. It adds the test for
idle-wakeup as an "extra" to break out of the normal exception path.
Then it branches to a relocated idle handler that calls the various
idle handling functions.

After this patch, POWER8 CPU simulator now boots powernv kernel that is
running at non-zero.

Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/exception-64s.h | 16 ++++++++++
 arch/powerpc/kernel/exceptions-64s.S     | 50 ++++++++++++++++++--------------
 2 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index 2e4e7d8..84d49b1 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h
@@ -93,6 +93,10 @@
 	ld	reg,PACAKBASE(r13);	/* get high part of &label */	\
 	ori	reg,reg,(FIXED_SYMBOL_ABS_ADDR(label))@l;
 
+#define __LOAD_HANDLER(reg, label)					\
+	ld	reg,PACAKBASE(r13);					\
+	ori	reg,reg,(ABS_ADDR(label))@l;
+
 /* Exception register prefixes */
 #define EXC_HV	H
 #define EXC_STD
@@ -208,6 +212,18 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
 #define kvmppc_interrupt kvmppc_interrupt_pr
 #endif
 
+#ifdef CONFIG_RELOCATABLE
+#define BRANCH_TO_COMMON(reg, label)					\
+	__LOAD_HANDLER(reg, label);					\
+	mtctr	reg;							\
+	bctr
+
+#else
+#define BRANCH_TO_COMMON(reg, label)					\
+	b	label
+
+#endif
+
 #define __KVM_HANDLER_PROLOG(area, n)					\
 	BEGIN_FTR_SECTION_NESTED(947)					\
 	ld	r10,area+EX_CFAR(r13);					\
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 08992f8..e680e84 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -95,19 +95,35 @@ __start_interrupts:
 /* No virt vectors corresponding with 0x0..0x100 */
 EXC_VIRT_NONE(0x4000, 0x4100)
 
-EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
-	SET_SCRATCH0(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/sleep/winkle.
+	/*
+	 * If running native on arch 2.06 or later, check if we are waking up
+	 * from nap/sleep/winkle, and branch to idle handler.
 	 */
-	mfspr	r13,SPRN_SRR1
-	rlwinm.	r13,r13,47-31,30,31
-	beq	9f
+#define IDLETEST(n)							\
+	BEGIN_FTR_SECTION ;						\
+	mfspr	r10,SPRN_SRR1 ;						\
+	rlwinm.	r10,r10,47-31,30,31 ;					\
+	beq-	1f ;							\
+	cmpwi	cr3,r10,2 ;						\
+	BRANCH_TO_COMMON(r10, system_reset_idle_common) ;		\
+1:									\
+	END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
+#else
+#define IDLETEST NOTEST
+#endif
 
-	cmpwi	cr3,r13,2
-	GET_PACA(r13)
+EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
+	SET_SCRATCH0(r13)
+	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
+				 IDLETEST, 0x100)
+
+EXC_REAL_END(system_reset, 0x100, 0x200)
+EXC_VIRT_NONE(0x4100, 0x4200)
+
+#ifdef CONFIG_PPC_P7_NAP
+EXC_COMMON_BEGIN(system_reset_idle_common)
 	bl	pnv_restore_hyp_resource
 
 	li	r0,PNV_THREAD_RUNNING
@@ -130,14 +146,8 @@ BEGIN_FTR_SECTION
 	blt	cr3,2f
 	b	pnv_wakeup_loss
 2:	b	pnv_wakeup_noloss
+#endif
 
-9:
-END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
-#endif /* CONFIG_PPC_P7_NAP */
-	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
-				 NOTEST, 0x100)
-EXC_REAL_END(system_reset, 0x100, 0x200)
-EXC_VIRT_NONE(0x4100, 0x4200)
 EXC_COMMON(system_reset_common, 0x100, system_reset_exception)
 
 #ifdef CONFIG_PPC_PSERIES
@@ -817,10 +827,8 @@ EXC_VIRT(trap_0b, 0x4b00, 0x4c00, 0xb00)
 TRAMP_KVM(PACA_EXGEN, 0xb00)
 EXC_COMMON(trap_0b_common, 0xb00, unknown_exception)
 
-
-#define LOAD_SYSCALL_HANDLER(reg)				\
-	ld	reg,PACAKBASE(r13);				\
-	ori	reg,reg,(ABS_ADDR(system_call_common))@l;
+#define LOAD_SYSCALL_HANDLER(reg)					\
+	__LOAD_HANDLER(reg, system_call_common)
 
 /* Syscall routine is used twice, in reloc-off and reloc-on paths */
 #define SYSCALL_PSERIES_1 					\
-- 
2.9.3

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Gautham R Shenoy]

Hi Nick,

[Updated Shreyas's current e-mail address:
	 "Shreyas B. Prabhu" <shreyasbp@gmail.com>]

On Thu, Oct 13, 2016 at 01:17:14PM +1100, Nicholas Piggin wrote:
> This patch does a couple of things. First of all, powernv immediately
> explodes when running a relocated kernel, because the system reset
> exception for handling sleeps does not do correct relocated branches.
> 
> Secondly, the sleep handling code trashes the condition and cfar
> registers, which we would like to preserve for debugging purposes (for
> non-sleep case exception).
> 
> This patch changes the exception to use the standard format that saves
> registers before any tests or branches are made. It adds the test for
> idle-wakeup as an "extra" to break out of the normal exception path.
> Then it branches to a relocated idle handler that calls the various
> idle handling functions.
> 
> After this patch, POWER8 CPU simulator now boots powernv kernel that is
> running at non-zero.

This patch looks good to me. I have tested this on POWER8 and verified
that we are correctly waking up from nap, sleep and winkle.

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

> 
> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/include/asm/exception-64s.h | 16 ++++++++++
>  arch/powerpc/kernel/exceptions-64s.S     | 50 ++++++++++++++++++--------------
>  2 files changed, 45 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index 2e4e7d8..84d49b1 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -93,6 +93,10 @@
>  	ld	reg,PACAKBASE(r13);	/* get high part of &label */	\
>  	ori	reg,reg,(FIXED_SYMBOL_ABS_ADDR(label))@l;
> 
> +#define __LOAD_HANDLER(reg, label)					\
> +	ld	reg,PACAKBASE(r13);					\
> +	ori	reg,reg,(ABS_ADDR(label))@l;
> +
>  /* Exception register prefixes */
>  #define EXC_HV	H
>  #define EXC_STD
> @@ -208,6 +212,18 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
>  #define kvmppc_interrupt kvmppc_interrupt_pr
>  #endif
> 
> +#ifdef CONFIG_RELOCATABLE
> +#define BRANCH_TO_COMMON(reg, label)					\
> +	__LOAD_HANDLER(reg, label);					\
> +	mtctr	reg;							\
> +	bctr
> +
> +#else
> +#define BRANCH_TO_COMMON(reg, label)					\
> +	b	label
> +
> +#endif
> +
>  #define __KVM_HANDLER_PROLOG(area, n)					\
>  	BEGIN_FTR_SECTION_NESTED(947)					\
>  	ld	r10,area+EX_CFAR(r13);					\
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 08992f8..e680e84 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -95,19 +95,35 @@ __start_interrupts:
>  /* No virt vectors corresponding with 0x0..0x100 */
>  EXC_VIRT_NONE(0x4000, 0x4100)
> 
> -EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> -	SET_SCRATCH0(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/sleep/winkle.
> +	/*
> +	 * If running native on arch 2.06 or later, check if we are waking up
> +	 * from nap/sleep/winkle, and branch to idle handler.
>  	 */
> -	mfspr	r13,SPRN_SRR1
> -	rlwinm.	r13,r13,47-31,30,31
> -	beq	9f
> +#define IDLETEST(n)							\
> +	BEGIN_FTR_SECTION ;						\
> +	mfspr	r10,SPRN_SRR1 ;						\
> +	rlwinm.	r10,r10,47-31,30,31 ;					\
> +	beq-	1f ;							\
> +	cmpwi	cr3,r10,2 ;						\
> +	BRANCH_TO_COMMON(r10, system_reset_idle_common) ;		\
> +1:									\
> +	END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
> +#else
> +#define IDLETEST NOTEST
> +#endif
> 
> -	cmpwi	cr3,r13,2
> -	GET_PACA(r13)
> +EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> +	SET_SCRATCH0(r13)
> +	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
> +				 IDLETEST, 0x100)
> +
> +EXC_REAL_END(system_reset, 0x100, 0x200)
> +EXC_VIRT_NONE(0x4100, 0x4200)
> +
> +#ifdef CONFIG_PPC_P7_NAP
> +EXC_COMMON_BEGIN(system_reset_idle_common)
>  	bl	pnv_restore_hyp_resource
> 
>  	li	r0,PNV_THREAD_RUNNING
> @@ -130,14 +146,8 @@ BEGIN_FTR_SECTION
>  	blt	cr3,2f
>  	b	pnv_wakeup_loss
>  2:	b	pnv_wakeup_noloss
> +#endif
> 
> -9:
> -END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
> -#endif /* CONFIG_PPC_P7_NAP */
> -	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
> -				 NOTEST, 0x100)
> -EXC_REAL_END(system_reset, 0x100, 0x200)
> -EXC_VIRT_NONE(0x4100, 0x4200)
>  EXC_COMMON(system_reset_common, 0x100, system_reset_exception)
> 
>  #ifdef CONFIG_PPC_PSERIES
> @@ -817,10 +827,8 @@ EXC_VIRT(trap_0b, 0x4b00, 0x4c00, 0xb00)
>  TRAMP_KVM(PACA_EXGEN, 0xb00)
>  EXC_COMMON(trap_0b_common, 0xb00, unknown_exception)
> 
> -
> -#define LOAD_SYSCALL_HANDLER(reg)				\
> -	ld	reg,PACAKBASE(r13);				\
> -	ori	reg,reg,(ABS_ADDR(system_call_common))@l;
> +#define LOAD_SYSCALL_HANDLER(reg)					\
> +	__LOAD_HANDLER(reg, system_call_common)
> 
>  /* Syscall routine is used twice, in reloc-off and reloc-on paths */
>  #define SYSCALL_PSERIES_1 					\
> -- 
> 2.9.3
> 

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Balbir Singh]

On 13/10/16 13:17, Nicholas Piggin wrote:
> This patch does a couple of things. First of all, powernv immediately
> explodes when running a relocated kernel, because the system reset
> exception for handling sleeps does not do correct relocated branches.
> 
> Secondly, the sleep handling code trashes the condition and cfar
> registers, which we would like to preserve for debugging purposes (for
> non-sleep case exception).

Agreed, that is a good side, we also save the PPR in r9 and set the
PPR to HMT_MEDIUM

> 
> This patch changes the exception to use the standard format that saves
> registers before any tests or branches are made. It adds the test for
> idle-wakeup as an "extra" to break out of the normal exception path.
> Then it branches to a relocated idle handler that calls the various
> idle handling functions.
> 
> After this patch, POWER8 CPU simulator now boots powernv kernel that is
> running at non-zero.
> 

Yep, a much required fixup. I had done a version before your changes

> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/include/asm/exception-64s.h | 16 ++++++++++
>  arch/powerpc/kernel/exceptions-64s.S     | 50 ++++++++++++++++++--------------
>  2 files changed, 45 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index 2e4e7d8..84d49b1 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -93,6 +93,10 @@
>  	ld	reg,PACAKBASE(r13);	/* get high part of &label */	\
>  	ori	reg,reg,(FIXED_SYMBOL_ABS_ADDR(label))@l;
>  
> +#define __LOAD_HANDLER(reg, label)					\

I wonder if it is a good idea to trap

.if (reg == 13);
.error "Don't use r13";
.abort;
.endif;

> +	ld	reg,PACAKBASE(r13);					\
> +	ori	reg,reg,(ABS_ADDR(label))@l;
> +
>  /* Exception register prefixes */
>  #define EXC_HV	H
>  #define EXC_STD
> @@ -208,6 +212,18 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
>  #define kvmppc_interrupt kvmppc_interrupt_pr
>  #endif
>  
> +#ifdef CONFIG_RELOCATABLE
> +#define BRANCH_TO_COMMON(reg, label)					\
> +	__LOAD_HANDLER(reg, label);					\
> +	mtctr	reg;							\
> +	bctr
> +
> +#else
> +#define BRANCH_TO_COMMON(reg, label)					\
> +	b	label
> +
> +#endif
> +
>  #define __KVM_HANDLER_PROLOG(area, n)					\
>  	BEGIN_FTR_SECTION_NESTED(947)					\
>  	ld	r10,area+EX_CFAR(r13);					\
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 08992f8..e680e84 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -95,19 +95,35 @@ __start_interrupts:
>  /* No virt vectors corresponding with 0x0..0x100 */
>  EXC_VIRT_NONE(0x4000, 0x4100)
>  
> -EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> -	SET_SCRATCH0(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/sleep/winkle.
> +	/*
> +	 * If running native on arch 2.06 or later, check if we are waking up
> +	 * from nap/sleep/winkle, and branch to idle handler.
>  	 */
> -	mfspr	r13,SPRN_SRR1
> -	rlwinm.	r13,r13,47-31,30,31
> -	beq	9f
> +#define IDLETEST(n)							\
> +	BEGIN_FTR_SECTION ;						\
> +	mfspr	r10,SPRN_SRR1 ;						\
> +	rlwinm.	r10,r10,47-31,30,31 ;					\
> +	beq-	1f ;							\
> +	cmpwi	cr3,r10,2 ;						\
> +	BRANCH_TO_COMMON(r10, system_reset_idle_common) ;		\
> +1:									\
> +	END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
> +#else
> +#define IDLETEST NOTEST
> +#endif
>  
> -	cmpwi	cr3,r13,2
> -	GET_PACA(r13)
> +EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> +	SET_SCRATCH0(r13)
> +	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
> +				 IDLETEST, 0x100)
> +
> +EXC_REAL_END(system_reset, 0x100, 0x200)
> +EXC_VIRT_NONE(0x4100, 0x4200)
> +
> +#ifdef CONFIG_PPC_P7_NAP
> +EXC_COMMON_BEGIN(system_reset_idle_common)
>  	bl	pnv_restore_hyp_resource
>  
>  	li	r0,PNV_THREAD_RUNNING
> @@ -130,14 +146,8 @@ BEGIN_FTR_SECTION
>  	blt	cr3,2f
>  	b	pnv_wakeup_loss
>  2:	b	pnv_wakeup_noloss
> +#endif
>  
> -9:
> -END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
> -#endif /* CONFIG_PPC_P7_NAP */
> -	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
> -				 NOTEST, 0x100)
> -EXC_REAL_END(system_reset, 0x100, 0x200)
> -EXC_VIRT_NONE(0x4100, 0x4200)
>  EXC_COMMON(system_reset_common, 0x100, system_reset_exception)
>  
>  #ifdef CONFIG_PPC_PSERIES
> @@ -817,10 +827,8 @@ EXC_VIRT(trap_0b, 0x4b00, 0x4c00, 0xb00)
>  TRAMP_KVM(PACA_EXGEN, 0xb00)
>  EXC_COMMON(trap_0b_common, 0xb00, unknown_exception)
>  
> -
> -#define LOAD_SYSCALL_HANDLER(reg)				\
> -	ld	reg,PACAKBASE(r13);				\
> -	ori	reg,reg,(ABS_ADDR(system_call_common))@l;
> +#define LOAD_SYSCALL_HANDLER(reg)					\
> +	__LOAD_HANDLER(reg, system_call_common)
>  
>  /* Syscall routine is used twice, in reloc-off and reloc-on paths */
>  #define SYSCALL_PSERIES_1 					\
> 

Acked-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Nicholas Piggin]

Thanks Balbir and Gautham for testing and reviewing.

On Thu, 13 Oct 2016 22:54:32 +1100
Balbir Singh <bsingharora@gmail.com> wrote:

> On 13/10/16 13:17, Nicholas Piggin wrote:
> > This patch does a couple of things. First of all, powernv immediately
> > explodes when running a relocated kernel, because the system reset
> > exception for handling sleeps does not do correct relocated branches.
> > 
> > Secondly, the sleep handling code trashes the condition and cfar
> > registers, which we would like to preserve for debugging purposes (for
> > non-sleep case exception).  
> 
> Agreed, that is a good side, we also save the PPR in r9 and set the
> PPR to HMT_MEDIUM
> 
> > 
> > This patch changes the exception to use the standard format that saves
> > registers before any tests or branches are made. It adds the test for
> > idle-wakeup as an "extra" to break out of the normal exception path.
> > Then it branches to a relocated idle handler that calls the various
> > idle handling functions.
> > 
> > After this patch, POWER8 CPU simulator now boots powernv kernel that is
> > running at non-zero.
> >   
> 
> Yep, a much required fixup. I had done a version before your changes

Yeah, I didn't mean to push ahead of your patch -- I was halfway through
writing the register save patch when I realized yours did not get merged
yet. Credit to you for originally finding and fixing it.

> 
> > Cc: Balbir Singh <bsingharora@gmail.com>
> > Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> > Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  arch/powerpc/include/asm/exception-64s.h | 16 ++++++++++
> >  arch/powerpc/kernel/exceptions-64s.S     | 50 ++++++++++++++++++--------------
> >  2 files changed, 45 insertions(+), 21 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> > index 2e4e7d8..84d49b1 100644
> > --- a/arch/powerpc/include/asm/exception-64s.h
> > +++ b/arch/powerpc/include/asm/exception-64s.h
> > @@ -93,6 +93,10 @@
> >  	ld	reg,PACAKBASE(r13);	/* get high part of &label */	\
> >  	ori	reg,reg,(FIXED_SYMBOL_ABS_ADDR(label))@l;
> >  
> > +#define __LOAD_HANDLER(reg, label)					\  
> 
> I wonder if it is a good idea to trap
> 
> .if (reg == 13);
> .error "Don't use r13";
> .abort;
> .endif;

Well... I guess checking is always nice, but r13 is taboo for anything but paca
for virtually all assembly in the kernel. So I don't know if the benefit is very
large.

Now a scripted test to check the objdump might be nice. You could whitelist the few
places that set r13, and then give errors for any other code that sets it.
 
Thanks,
Nick

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Balbir Singh]

On 14/10/16 14:16, Nicholas Piggin wrote:
> Thanks Balbir and Gautham for testing and reviewing.
> 
> On Thu, 13 Oct 2016 22:54:32 +1100
> Balbir Singh <bsingharora@gmail.com> wrote:
> 
>> On 13/10/16 13:17, Nicholas Piggin wrote:
>>> This patch does a couple of things. First of all, powernv immediately
>>> explodes when running a relocated kernel, because the system reset
>>> exception for handling sleeps does not do correct relocated branches.
>>>
>>> Secondly, the sleep handling code trashes the condition and cfar
>>> registers, which we would like to preserve for debugging purposes (for
>>> non-sleep case exception).  
>>
>> Agreed, that is a good side, we also save the PPR in r9 and set the
>> PPR to HMT_MEDIUM
>>
>>>
>>> This patch changes the exception to use the standard format that saves
>>> registers before any tests or branches are made. It adds the test for
>>> idle-wakeup as an "extra" to break out of the normal exception path.
>>> Then it branches to a relocated idle handler that calls the various
>>> idle handling functions.
>>>
>>> After this patch, POWER8 CPU simulator now boots powernv kernel that is
>>> running at non-zero.
>>>   
>>
>> Yep, a much required fixup. I had done a version before your changes
> 
> Yeah, I didn't mean to push ahead of your patch -- I was halfway through
> writing the register save patch when I realized yours did not get merged
> yet. Credit to you for originally finding and fixing it.
> 

No problems with that

>>
>>> Cc: Balbir Singh <bsingharora@gmail.com>
>>> Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
>>> Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>  arch/powerpc/include/asm/exception-64s.h | 16 ++++++++++
>>>  arch/powerpc/kernel/exceptions-64s.S     | 50 ++++++++++++++++++--------------
>>>  2 files changed, 45 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
>>> index 2e4e7d8..84d49b1 100644
>>> --- a/arch/powerpc/include/asm/exception-64s.h
>>> +++ b/arch/powerpc/include/asm/exception-64s.h
>>> @@ -93,6 +93,10 @@
>>>  	ld	reg,PACAKBASE(r13);	/* get high part of &label */	\
>>>  	ori	reg,reg,(FIXED_SYMBOL_ABS_ADDR(label))@l;
>>>  
>>> +#define __LOAD_HANDLER(reg, label)					\  
>>
>> I wonder if it is a good idea to trap
>>
>> .if (reg == 13);
>> .error "Don't use r13";
>> .abort;
>> .endif;
> 
> Well... I guess checking is always nice, but r13 is taboo for anything but paca
> for virtually all assembly in the kernel. So I don't know if the benefit is very
> large.
> 

The thing with macros that take arguments is that there is no indication whats
OK and whats not. You are right r13 is taboo, but we could probably still use
r13 and do a GET_PACA() later, like we did earlier, so lets drop this

> Now a scripted test to check the objdump might be nice. You could whitelist the few
> places that set r13, and then give errors for any other code that sets it.
>  

Good idea

Balbir Singh

Re: powerpc/64s: relocation, register save fixes for system reset interrupt

[Michael Ellerman]

On Thu, 2016-13-10 at 02:17:14 UTC, Nicholas Piggin wrote:
> This patch does a couple of things. First of all, powernv immediately
> explodes when running a relocated kernel, because the system reset
> exception for handling sleeps does not do correct relocated branches.
> 
> Secondly, the sleep handling code trashes the condition and cfar
> registers, which we would like to preserve for debugging purposes (for
> non-sleep case exception).
> 
> This patch changes the exception to use the standard format that saves
> registers before any tests or branches are made. It adds the test for
> idle-wakeup as an "extra" to break out of the normal exception path.
> Then it branches to a relocated idle handler that calls the various
> idle handling functions.
> 
> After this patch, POWER8 CPU simulator now boots powernv kernel that is
> running at non-zero.
> 
> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Acked-by: Balbir Singh <bsingharora@gmail.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/fb479e44a9e240a23c2d208c2ace23

cheers

Re: powerpc/64s: relocation, register save fixes for system reset interrupt

[Balbir Singh]

On 27/10/16 22:38, Michael Ellerman wrote:
> On Thu, 2016-13-10 at 02:17:14 UTC, Nicholas Piggin wrote:
>> This patch does a couple of things. First of all, powernv immediately
>> explodes when running a relocated kernel, because the system reset
>> exception for handling sleeps does not do correct relocated branches.
>>
>> Secondly, the sleep handling code trashes the condition and cfar
>> registers, which we would like to preserve for debugging purposes (for
>> non-sleep case exception).
>>
>> This patch changes the exception to use the standard format that saves
>> registers before any tests or branches are made. It adds the test for
>> idle-wakeup as an "extra" to break out of the normal exception path.
>> Then it branches to a relocated idle handler that calls the various
>> idle handling functions.
>>
>> After this patch, POWER8 CPU simulator now boots powernv kernel that is
>> running at non-zero.
>>
>> Cc: Balbir Singh <bsingharora@gmail.com>
>> Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
>> Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
>> Acked-by: Balbir Singh <bsingharora@gmail.com>
> 
> Applied to powerpc fixes, thanks.
> 
> https://git.kernel.org/powerpc/c/fb479e44a9e240a23c2d208c2ace23
> 
BTW, this cannot be directly marked back to stable
(ref: http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=fb479e44a9e240a23c2d208c2ace23542a47f41c)

For stable, we might need

https://marc.info/?l=linuxppc-embedded&m=146967183624811&w=4

Balbir Singh

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Mahesh Jagannath Salgaonkar]

On 10/13/2016 07:47 AM, Nicholas Piggin wrote:
> This patch does a couple of things. First of all, powernv immediately
> explodes when running a relocated kernel, because the system reset
> exception for handling sleeps does not do correct relocated branches.
> 
> Secondly, the sleep handling code trashes the condition and cfar
> registers, which we would like to preserve for debugging purposes (for
> non-sleep case exception).
> 
> This patch changes the exception to use the standard format that saves
> registers before any tests or branches are made. It adds the test for
> idle-wakeup as an "extra" to break out of the normal exception path.
> Then it branches to a relocated idle handler that calls the various
> idle handling functions.
> 
> After this patch, POWER8 CPU simulator now boots powernv kernel that is
> running at non-zero.
> 
> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/include/asm/exception-64s.h | 16 ++++++++++
>  arch/powerpc/kernel/exceptions-64s.S     | 50 ++++++++++++++++++--------------
>  2 files changed, 45 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index 2e4e7d8..84d49b1 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -93,6 +93,10 @@
>  	ld	reg,PACAKBASE(r13);	/* get high part of &label */	\
>  	ori	reg,reg,(FIXED_SYMBOL_ABS_ADDR(label))@l;
> 
> +#define __LOAD_HANDLER(reg, label)					\
> +	ld	reg,PACAKBASE(r13);					\
> +	ori	reg,reg,(ABS_ADDR(label))@l;
> +
>  /* Exception register prefixes */
>  #define EXC_HV	H
>  #define EXC_STD
> @@ -208,6 +212,18 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
>  #define kvmppc_interrupt kvmppc_interrupt_pr
>  #endif
> 
> +#ifdef CONFIG_RELOCATABLE
> +#define BRANCH_TO_COMMON(reg, label)					\
> +	__LOAD_HANDLER(reg, label);					\
> +	mtctr	reg;							\
> +	bctr
> +
> +#else
> +#define BRANCH_TO_COMMON(reg, label)					\
> +	b	label
> +
> +#endif
> +
>  #define __KVM_HANDLER_PROLOG(area, n)					\
>  	BEGIN_FTR_SECTION_NESTED(947)					\
>  	ld	r10,area+EX_CFAR(r13);					\
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 08992f8..e680e84 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -95,19 +95,35 @@ __start_interrupts:
>  /* No virt vectors corresponding with 0x0..0x100 */
>  EXC_VIRT_NONE(0x4000, 0x4100)
> 
> -EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> -	SET_SCRATCH0(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/sleep/winkle.
> +	/*
> +	 * If running native on arch 2.06 or later, check if we are waking up
> +	 * from nap/sleep/winkle, and branch to idle handler.
>  	 */
> -	mfspr	r13,SPRN_SRR1
> -	rlwinm.	r13,r13,47-31,30,31
> -	beq	9f
> +#define IDLETEST(n)							\
> +	BEGIN_FTR_SECTION ;						\
> +	mfspr	r10,SPRN_SRR1 ;						\
> +	rlwinm.	r10,r10,47-31,30,31 ;					\
> +	beq-	1f ;							\
> +	cmpwi	cr3,r10,2 ;						\
> +	BRANCH_TO_COMMON(r10, system_reset_idle_common) ;		\
> +1:									\
> +	END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
> +#else
> +#define IDLETEST NOTEST
> +#endif
> 
> -	cmpwi	cr3,r13,2
> -	GET_PACA(r13)
> +EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> +	SET_SCRATCH0(r13)
> +	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
> +				 IDLETEST, 0x100)

Very sorry for late review. On arch 2.07 and less if we wakeup from
winkle then last bit of HSPGR0 would be set to 1. Hence before we access
paca we need to fix it by clearing that bit and that is done in
pnv_restore_hyp_resource(). But with this patch, we would end up there
after going through EXCEPTION_PROLOG_PSERIES(). This macro gets the paca
using GET_PACA(r13) and all the EXCEPTION_PROLOG_* starts
using/accessing r13/paca without fixing it. Wouldn't this break things
badly on arch 2.07 and less ? Am I missing anything ?

Thanks,
-Mahesh.

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Nicholas Piggin]

On Wed, 2 Nov 2016 11:34:59 +0530
Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com> wrote:

> On 10/13/2016 07:47 AM, Nicholas Piggin wrote:
> > This patch does a couple of things. First of all, powernv immediately
> > explodes when running a relocated kernel, because the system reset
> > exception for handling sleeps does not do correct relocated branches.
> > 
> > Secondly, the sleep handling code trashes the condition and cfar
> > registers, which we would like to preserve for debugging purposes (for
> > non-sleep case exception).
> > 
> > This patch changes the exception to use the standard format that saves
> > registers before any tests or branches are made. It adds the test for
> > idle-wakeup as an "extra" to break out of the normal exception path.
> > Then it branches to a relocated idle handler that calls the various
> > idle handling functions.
> > 
> > After this patch, POWER8 CPU simulator now boots powernv kernel that is
> > running at non-zero.
> > 
> > Cc: Balbir Singh <bsingharora@gmail.com>
> > Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> > Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  arch/powerpc/include/asm/exception-64s.h | 16 ++++++++++
> >  arch/powerpc/kernel/exceptions-64s.S     | 50 ++++++++++++++++++--------------
> >  2 files changed, 45 insertions(+), 21 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> > index 2e4e7d8..84d49b1 100644
> > --- a/arch/powerpc/include/asm/exception-64s.h
> > +++ b/arch/powerpc/include/asm/exception-64s.h
> > @@ -93,6 +93,10 @@
> >  	ld	reg,PACAKBASE(r13);	/* get high part of &label */	\
> >  	ori	reg,reg,(FIXED_SYMBOL_ABS_ADDR(label))@l;
> > 
> > +#define __LOAD_HANDLER(reg, label)					\
> > +	ld	reg,PACAKBASE(r13);					\
> > +	ori	reg,reg,(ABS_ADDR(label))@l;
> > +
> >  /* Exception register prefixes */
> >  #define EXC_HV	H
> >  #define EXC_STD
> > @@ -208,6 +212,18 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
> >  #define kvmppc_interrupt kvmppc_interrupt_pr
> >  #endif
> > 
> > +#ifdef CONFIG_RELOCATABLE
> > +#define BRANCH_TO_COMMON(reg, label)					\
> > +	__LOAD_HANDLER(reg, label);					\
> > +	mtctr	reg;							\
> > +	bctr
> > +
> > +#else
> > +#define BRANCH_TO_COMMON(reg, label)					\
> > +	b	label
> > +
> > +#endif
> > +
> >  #define __KVM_HANDLER_PROLOG(area, n)					\
> >  	BEGIN_FTR_SECTION_NESTED(947)					\
> >  	ld	r10,area+EX_CFAR(r13);					\
> > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> > index 08992f8..e680e84 100644
> > --- a/arch/powerpc/kernel/exceptions-64s.S
> > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > @@ -95,19 +95,35 @@ __start_interrupts:
> >  /* No virt vectors corresponding with 0x0..0x100 */
> >  EXC_VIRT_NONE(0x4000, 0x4100)
> > 
> > -EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> > -	SET_SCRATCH0(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/sleep/winkle.
> > +	/*
> > +	 * If running native on arch 2.06 or later, check if we are waking up
> > +	 * from nap/sleep/winkle, and branch to idle handler.
> >  	 */
> > -	mfspr	r13,SPRN_SRR1
> > -	rlwinm.	r13,r13,47-31,30,31
> > -	beq	9f
> > +#define IDLETEST(n)							\
> > +	BEGIN_FTR_SECTION ;						\
> > +	mfspr	r10,SPRN_SRR1 ;						\
> > +	rlwinm.	r10,r10,47-31,30,31 ;					\
> > +	beq-	1f ;							\
> > +	cmpwi	cr3,r10,2 ;						\
> > +	BRANCH_TO_COMMON(r10, system_reset_idle_common) ;		\
> > +1:									\
> > +	END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
> > +#else
> > +#define IDLETEST NOTEST
> > +#endif
> > 
> > -	cmpwi	cr3,r13,2
> > -	GET_PACA(r13)
> > +EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> > +	SET_SCRATCH0(r13)
> > +	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
> > +				 IDLETEST, 0x100)  
> 
> Very sorry for late review. On arch 2.07 and less if we wakeup from
> winkle then last bit of HSPGR0 would be set to 1. Hence before we access
> paca we need to fix it by clearing that bit and that is done in
> pnv_restore_hyp_resource(). But with this patch, we would end up there
> after going through EXCEPTION_PROLOG_PSERIES(). This macro gets the paca
> using GET_PACA(r13) and all the EXCEPTION_PROLOG_* starts
> using/accessing r13/paca without fixing it. Wouldn't this break things
> badly on arch 2.07 and less ? Am I missing anything ?

Arg, that's a stupid bug :( Thanks for catching it.

Would something like the following do the trick, do you think? I obviously
was not reaching winkle state in my testing.

Thanks,
Nick

---
 arch/powerpc/include/asm/exception-64s.h | 13 +++++++++++--
 arch/powerpc/kernel/exceptions-64s.S     | 11 ++++++++---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index 84d49b1..3ce4366 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h
@@ -158,14 +158,17 @@ BEGIN_FTR_SECTION_NESTED(943)						\
 	std	ra,offset(r13);						\
 END_FTR_SECTION_NESTED(ftr,ftr,943)
 
-#define EXCEPTION_PROLOG_0(area)					\
-	GET_PACA(r13);							\
+#define EXCEPTION_PROLOG_0_PACA(area)					\
 	std	r9,area+EX_R9(r13);	/* save r9 */			\
 	OPT_GET_SPR(r9, SPRN_PPR, CPU_FTR_HAS_PPR);			\
 	HMT_MEDIUM;							\
 	std	r10,area+EX_R10(r13);	/* save r10 - r12 */		\
 	OPT_GET_SPR(r10, SPRN_CFAR, CPU_FTR_CFAR)
 
+#define EXCEPTION_PROLOG_0(area)					\
+	GET_PACA(r13);							\
+	EXCEPTION_PROLOG_0_PACA(area)
+
 #define __EXCEPTION_PROLOG_1(area, extra, vec)				\
 	OPT_SAVE_REG_TO_PACA(area+EX_PPR, r9, CPU_FTR_HAS_PPR);		\
 	OPT_SAVE_REG_TO_PACA(area+EX_CFAR, r10, CPU_FTR_CFAR);		\
@@ -196,6 +199,12 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
 	EXCEPTION_PROLOG_1(area, extra, vec);				\
 	EXCEPTION_PROLOG_PSERIES_1(label, h);
 
+/* Have the PACA in r13 already */
+#define EXCEPTION_PROLOG_PSERIES_PACA(area, label, h, extra, vec)	\
+	EXCEPTION_PROLOG_0_PACA(area);					\
+	EXCEPTION_PROLOG_1(area, extra, vec);				\
+	EXCEPTION_PROLOG_PSERIES_1(label, h);
+
 #define __KVMTEST(h, n)							\
 	lbz	r10,HSTATE_IN_GUEST(r13);				\
 	cmpwi	r10,0;							\
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 08ba447..1ba82ea 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -116,7 +116,9 @@ EXC_VIRT_NONE(0x4000, 0x4100)
 
 EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
 	SET_SCRATCH0(r13)
-	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
+	GET_PACA(r13)
+	clrrdi	r13,r13,1 /* Last bit of HSPRG0 is set if waking from winkle */
+	EXCEPTION_PROLOG_PSERIES_PACA(PACA_EXGEN, system_reset_common, EXC_STD,
 				 IDLETEST, 0x100)
 
 EXC_REAL_END(system_reset, 0x100, 0x200)
@@ -124,6 +126,9 @@ EXC_VIRT_NONE(0x4100, 0x4200)
 
 #ifdef CONFIG_PPC_P7_NAP
 EXC_COMMON_BEGIN(system_reset_idle_common)
+BEGIN_FTR_SECTION
+	GET_PACA(r13) /* Restore HSPRG0 to get the winkle bit in r13 */
+END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
 	bl	pnv_restore_hyp_resource
 
 	li	r0,PNV_THREAD_RUNNING
@@ -169,7 +174,7 @@ EXC_REAL_BEGIN(machine_check, 0x200, 0x300)
 	SET_SCRATCH0(r13)		/* save r13 */
 	/*
 	 * 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
+	 * inside machine check. If yes, then last bit of HSPRG0 would be set
 	 * to 1. Hence clear it unconditionally.
 	 */
 	GET_PACA(r13)
@@ -388,7 +393,7 @@ EXC_COMMON_BEGIN(machine_check_handle_early)
 	/*
 	 * 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
+	 * state. Hence before going back to winkle, set last bit of HSPRG0
 	 * 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.
-- 
2.9.3

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Mahesh J Salgaonkar]

On 2016-11-02 17:57:01 Wed, Nicholas Piggin wrote:
> On Wed, 2 Nov 2016 11:34:59 +0530
> Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com> wrote:
> 
> > On 10/13/2016 07:47 AM, Nicholas Piggin wrote:
> > > This patch does a couple of things. First of all, powernv immediately
> > > explodes when running a relocated kernel, because the system reset
> > > exception for handling sleeps does not do correct relocated branches.
> > > 
> > > Secondly, the sleep handling code trashes the condition and cfar
> > > registers, which we would like to preserve for debugging purposes (for
> > > non-sleep case exception).
> > > 
> > > This patch changes the exception to use the standard format that saves
> > > registers before any tests or branches are made. It adds the test for
> > > idle-wakeup as an "extra" to break out of the normal exception path.
> > > Then it branches to a relocated idle handler that calls the various
> > > idle handling functions.
> > > 
> > > After this patch, POWER8 CPU simulator now boots powernv kernel that is
> > > running at non-zero.
> > > 
> > > Cc: Balbir Singh <bsingharora@gmail.com>
> > > Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> > > Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > ---
> > >  arch/powerpc/include/asm/exception-64s.h | 16 ++++++++++
> > >  arch/powerpc/kernel/exceptions-64s.S     | 50 ++++++++++++++++++--------------
> > >  2 files changed, 45 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> > > index 2e4e7d8..84d49b1 100644
> > > --- a/arch/powerpc/include/asm/exception-64s.h
> > > +++ b/arch/powerpc/include/asm/exception-64s.h
> > > @@ -93,6 +93,10 @@
> > >  	ld	reg,PACAKBASE(r13);	/* get high part of &label */	\
> > >  	ori	reg,reg,(FIXED_SYMBOL_ABS_ADDR(label))@l;
> > > 
> > > +#define __LOAD_HANDLER(reg, label)					\
> > > +	ld	reg,PACAKBASE(r13);					\
> > > +	ori	reg,reg,(ABS_ADDR(label))@l;
> > > +
> > >  /* Exception register prefixes */
> > >  #define EXC_HV	H
> > >  #define EXC_STD
> > > @@ -208,6 +212,18 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
> > >  #define kvmppc_interrupt kvmppc_interrupt_pr
> > >  #endif
> > > 
> > > +#ifdef CONFIG_RELOCATABLE
> > > +#define BRANCH_TO_COMMON(reg, label)					\
> > > +	__LOAD_HANDLER(reg, label);					\
> > > +	mtctr	reg;							\
> > > +	bctr
> > > +
> > > +#else
> > > +#define BRANCH_TO_COMMON(reg, label)					\
> > > +	b	label
> > > +
> > > +#endif
> > > +
> > >  #define __KVM_HANDLER_PROLOG(area, n)					\
> > >  	BEGIN_FTR_SECTION_NESTED(947)					\
> > >  	ld	r10,area+EX_CFAR(r13);					\
> > > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> > > index 08992f8..e680e84 100644
> > > --- a/arch/powerpc/kernel/exceptions-64s.S
> > > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > > @@ -95,19 +95,35 @@ __start_interrupts:
> > >  /* No virt vectors corresponding with 0x0..0x100 */
> > >  EXC_VIRT_NONE(0x4000, 0x4100)
> > > 
> > > -EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> > > -	SET_SCRATCH0(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/sleep/winkle.
> > > +	/*
> > > +	 * If running native on arch 2.06 or later, check if we are waking up
> > > +	 * from nap/sleep/winkle, and branch to idle handler.
> > >  	 */
> > > -	mfspr	r13,SPRN_SRR1
> > > -	rlwinm.	r13,r13,47-31,30,31
> > > -	beq	9f
> > > +#define IDLETEST(n)							\
> > > +	BEGIN_FTR_SECTION ;						\
> > > +	mfspr	r10,SPRN_SRR1 ;						\
> > > +	rlwinm.	r10,r10,47-31,30,31 ;					\
> > > +	beq-	1f ;							\
> > > +	cmpwi	cr3,r10,2 ;						\
> > > +	BRANCH_TO_COMMON(r10, system_reset_idle_common) ;		\
> > > +1:									\
> > > +	END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
> > > +#else
> > > +#define IDLETEST NOTEST
> > > +#endif
> > > 
> > > -	cmpwi	cr3,r13,2
> > > -	GET_PACA(r13)
> > > +EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> > > +	SET_SCRATCH0(r13)
> > > +	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
> > > +				 IDLETEST, 0x100)  
> > 
> > Very sorry for late review. On arch 2.07 and less if we wakeup from
> > winkle then last bit of HSPGR0 would be set to 1. Hence before we access
> > paca we need to fix it by clearing that bit and that is done in
> > pnv_restore_hyp_resource(). But with this patch, we would end up there
> > after going through EXCEPTION_PROLOG_PSERIES(). This macro gets the paca
> > using GET_PACA(r13) and all the EXCEPTION_PROLOG_* starts
> > using/accessing r13/paca without fixing it. Wouldn't this break things
> > badly on arch 2.07 and less ? Am I missing anything ?
> 
> Arg, that's a stupid bug :( Thanks for catching it.
> 
> Would something like the following do the trick, do you think? I obviously
> was not reaching winkle state in my testing.

Yup, that will work.

> 
> Thanks,
> Nick
> 
> ---
>  arch/powerpc/include/asm/exception-64s.h | 13 +++++++++++--
>  arch/powerpc/kernel/exceptions-64s.S     | 11 ++++++++---
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index 84d49b1..3ce4366 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -158,14 +158,17 @@ BEGIN_FTR_SECTION_NESTED(943)						\
>  	std	ra,offset(r13);						\
>  END_FTR_SECTION_NESTED(ftr,ftr,943)
> 
> -#define EXCEPTION_PROLOG_0(area)					\
> -	GET_PACA(r13);							\
> +#define EXCEPTION_PROLOG_0_PACA(area)					\
>  	std	r9,area+EX_R9(r13);	/* save r9 */			\
>  	OPT_GET_SPR(r9, SPRN_PPR, CPU_FTR_HAS_PPR);			\
>  	HMT_MEDIUM;							\
>  	std	r10,area+EX_R10(r13);	/* save r10 - r12 */		\
>  	OPT_GET_SPR(r10, SPRN_CFAR, CPU_FTR_CFAR)
> 
> +#define EXCEPTION_PROLOG_0(area)					\
> +	GET_PACA(r13);							\
> +	EXCEPTION_PROLOG_0_PACA(area)
> +
>  #define __EXCEPTION_PROLOG_1(area, extra, vec)				\
>  	OPT_SAVE_REG_TO_PACA(area+EX_PPR, r9, CPU_FTR_HAS_PPR);		\
>  	OPT_SAVE_REG_TO_PACA(area+EX_CFAR, r10, CPU_FTR_CFAR);		\
> @@ -196,6 +199,12 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
>  	EXCEPTION_PROLOG_1(area, extra, vec);				\
>  	EXCEPTION_PROLOG_PSERIES_1(label, h);
> 
> +/* Have the PACA in r13 already */
> +#define EXCEPTION_PROLOG_PSERIES_PACA(area, label, h, extra, vec)	\
> +	EXCEPTION_PROLOG_0_PACA(area);					\
> +	EXCEPTION_PROLOG_1(area, extra, vec);				\
> +	EXCEPTION_PROLOG_PSERIES_1(label, h);
> +
>  #define __KVMTEST(h, n)							\
>  	lbz	r10,HSTATE_IN_GUEST(r13);				\
>  	cmpwi	r10,0;							\
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 08ba447..1ba82ea 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -116,7 +116,9 @@ EXC_VIRT_NONE(0x4000, 0x4100)
> 
>  EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
>  	SET_SCRATCH0(r13)
> -	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
> +	GET_PACA(r13)
> +	clrrdi	r13,r13,1 /* Last bit of HSPRG0 is set if waking from winkle */
> +	EXCEPTION_PROLOG_PSERIES_PACA(PACA_EXGEN, system_reset_common, EXC_STD,
>  				 IDLETEST, 0x100)
> 
>  EXC_REAL_END(system_reset, 0x100, 0x200)
> @@ -124,6 +126,9 @@ EXC_VIRT_NONE(0x4100, 0x4200)
> 
>  #ifdef CONFIG_PPC_P7_NAP
>  EXC_COMMON_BEGIN(system_reset_idle_common)
> +BEGIN_FTR_SECTION
> +	GET_PACA(r13) /* Restore HSPRG0 to get the winkle bit in r13 */
> +END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
>  	bl	pnv_restore_hyp_resource
> 
>  	li	r0,PNV_THREAD_RUNNING
> @@ -169,7 +174,7 @@ EXC_REAL_BEGIN(machine_check, 0x200, 0x300)
>  	SET_SCRATCH0(r13)		/* save r13 */
>  	/*
>  	 * 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
> +	 * inside machine check. If yes, then last bit of HSPRG0 would be set
>  	 * to 1. Hence clear it unconditionally.
>  	 */
>  	GET_PACA(r13)
> @@ -388,7 +393,7 @@ EXC_COMMON_BEGIN(machine_check_handle_early)
>  	/*
>  	 * 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
> +	 * state. Hence before going back to winkle, set last bit of HSPRG0
>  	 * 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.
> -- 
> 2.9.3
> 

-- 
Mahesh J Salgaonkar

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Nicholas Piggin]

On Wed, 2 Nov 2016 13:54:43 +0530
Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> wrote:

> On 2016-11-02 17:57:01 Wed, Nicholas Piggin wrote:
> > On Wed, 2 Nov 2016 11:34:59 +0530
> > Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com> wrote:
> >   
> > > On 10/13/2016 07:47 AM, Nicholas Piggin wrote:  
> > > > This patch does a couple of things. First of all, powernv immediately
> > > > explodes when running a relocated kernel, because the system reset
> > > > exception for handling sleeps does not do correct relocated branches.
> > > > 
> > > > Secondly, the sleep handling code trashes the condition and cfar
> > > > registers, which we would like to preserve for debugging purposes (for
> > > > non-sleep case exception).
> > > > 
> > > > This patch changes the exception to use the standard format that saves
> > > > registers before any tests or branches are made. It adds the test for
> > > > idle-wakeup as an "extra" to break out of the normal exception path.
> > > > Then it branches to a relocated idle handler that calls the various
> > > > idle handling functions.
> > > > 
> > > > After this patch, POWER8 CPU simulator now boots powernv kernel that is
> > > > running at non-zero.
> > > > 
> > > > Cc: Balbir Singh <bsingharora@gmail.com>
> > > > Cc: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> > > > Cc: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > > ---
> > > >  arch/powerpc/include/asm/exception-64s.h | 16 ++++++++++
> > > >  arch/powerpc/kernel/exceptions-64s.S     | 50 ++++++++++++++++++--------------
> > > >  2 files changed, 45 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> > > > index 2e4e7d8..84d49b1 100644
> > > > --- a/arch/powerpc/include/asm/exception-64s.h
> > > > +++ b/arch/powerpc/include/asm/exception-64s.h
> > > > @@ -93,6 +93,10 @@
> > > >  	ld	reg,PACAKBASE(r13);	/* get high part of &label */	\
> > > >  	ori	reg,reg,(FIXED_SYMBOL_ABS_ADDR(label))@l;
> > > > 
> > > > +#define __LOAD_HANDLER(reg, label)					\
> > > > +	ld	reg,PACAKBASE(r13);					\
> > > > +	ori	reg,reg,(ABS_ADDR(label))@l;
> > > > +
> > > >  /* Exception register prefixes */
> > > >  #define EXC_HV	H
> > > >  #define EXC_STD
> > > > @@ -208,6 +212,18 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
> > > >  #define kvmppc_interrupt kvmppc_interrupt_pr
> > > >  #endif
> > > > 
> > > > +#ifdef CONFIG_RELOCATABLE
> > > > +#define BRANCH_TO_COMMON(reg, label)					\
> > > > +	__LOAD_HANDLER(reg, label);					\
> > > > +	mtctr	reg;							\
> > > > +	bctr
> > > > +
> > > > +#else
> > > > +#define BRANCH_TO_COMMON(reg, label)					\
> > > > +	b	label
> > > > +
> > > > +#endif
> > > > +
> > > >  #define __KVM_HANDLER_PROLOG(area, n)					\
> > > >  	BEGIN_FTR_SECTION_NESTED(947)					\
> > > >  	ld	r10,area+EX_CFAR(r13);					\
> > > > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> > > > index 08992f8..e680e84 100644
> > > > --- a/arch/powerpc/kernel/exceptions-64s.S
> > > > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > > > @@ -95,19 +95,35 @@ __start_interrupts:
> > > >  /* No virt vectors corresponding with 0x0..0x100 */
> > > >  EXC_VIRT_NONE(0x4000, 0x4100)
> > > > 
> > > > -EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> > > > -	SET_SCRATCH0(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/sleep/winkle.
> > > > +	/*
> > > > +	 * If running native on arch 2.06 or later, check if we are waking up
> > > > +	 * from nap/sleep/winkle, and branch to idle handler.
> > > >  	 */
> > > > -	mfspr	r13,SPRN_SRR1
> > > > -	rlwinm.	r13,r13,47-31,30,31
> > > > -	beq	9f
> > > > +#define IDLETEST(n)							\
> > > > +	BEGIN_FTR_SECTION ;						\
> > > > +	mfspr	r10,SPRN_SRR1 ;						\
> > > > +	rlwinm.	r10,r10,47-31,30,31 ;					\
> > > > +	beq-	1f ;							\
> > > > +	cmpwi	cr3,r10,2 ;						\
> > > > +	BRANCH_TO_COMMON(r10, system_reset_idle_common) ;		\
> > > > +1:									\
> > > > +	END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
> > > > +#else
> > > > +#define IDLETEST NOTEST
> > > > +#endif
> > > > 
> > > > -	cmpwi	cr3,r13,2
> > > > -	GET_PACA(r13)
> > > > +EXC_REAL_BEGIN(system_reset, 0x100, 0x200)
> > > > +	SET_SCRATCH0(r13)
> > > > +	EXCEPTION_PROLOG_PSERIES(PACA_EXGEN, system_reset_common, EXC_STD,
> > > > +				 IDLETEST, 0x100)    
> > > 
> > > Very sorry for late review. On arch 2.07 and less if we wakeup from
> > > winkle then last bit of HSPGR0 would be set to 1. Hence before we access
> > > paca we need to fix it by clearing that bit and that is done in
> > > pnv_restore_hyp_resource(). But with this patch, we would end up there
> > > after going through EXCEPTION_PROLOG_PSERIES(). This macro gets the paca
> > > using GET_PACA(r13) and all the EXCEPTION_PROLOG_* starts
> > > using/accessing r13/paca without fixing it. Wouldn't this break things
> > > badly on arch 2.07 and less ? Am I missing anything ?  
> > 
> > Arg, that's a stupid bug :( Thanks for catching it.
> > 
> > Would something like the following do the trick, do you think? I obviously
> > was not reaching winkle state in my testing.  
> 
> Yup, that will work.

Okay, I'll work with that. What's the best way to make a P8 do winkle sleeps?

Thanks,
Nick

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Gautham R Shenoy]

Hi Nick,

On Wed, Nov 02, 2016 at 07:36:24PM +1100, Nicholas Piggin wrote:
> 
> Okay, I'll work with that. What's the best way to make a P8 do
> winkle sleeps?

>From the userspace, offlining the CPUs of the core will put them to
winkle.

> 
> Thanks,
> Nick
> 
--
Thanks and Regards
gautham.

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Nicholas Piggin]

On Wed, 2 Nov 2016 14:15:48 +0530
Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:

> Hi Nick,
> 
> On Wed, Nov 02, 2016 at 07:36:24PM +1100, Nicholas Piggin wrote:
> > 
> > Okay, I'll work with that. What's the best way to make a P8 do
> > winkle sleeps?  
> 
> From the userspace, offlining the CPUs of the core will put them to
> winkle.

Thanks for this. Hum, that r13 manipulation throughout the idle
and exception code is a bit interesting. I'll do the minimal patch
for 4.9, but what's the reason not to just use the winkle state
in the PACA rather than storing it into HSPRG0 bit, can you (or
Shreyas) explain?

Thanks,
Nick

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Shreyas B. Prabhu]

On Thu, Nov 3, 2016 at 1:21 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> On Wed, 2 Nov 2016 14:15:48 +0530
> Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
>
>> Hi Nick,
>>
>> On Wed, Nov 02, 2016 at 07:36:24PM +1100, Nicholas Piggin wrote:
>> >
>> > Okay, I'll work with that. What's the best way to make a P8 do
>> > winkle sleeps?
>>
>> From the userspace, offlining the CPUs of the core will put them to
>> winkle.
>
> Thanks for this. Hum, that r13 manipulation throughout the idle
> and exception code is a bit interesting. I'll do the minimal patch
> for 4.9, but what's the reason not to just use the winkle state
> in the PACA rather than storing it into HSPRG0 bit, can you (or
> Shreyas) explain?
>
Hi Nick,

Before deep winkle, checking SRR1's wakeup bits (Bits 46:47) was enough to
figure out which idle state we are waking up from. But in P8, SRR1's wakeup
bits aren't enough since bits 46:47 are 0b11 for both fast sleep and
deep winkle.
So to distinguish bw fastsleep and deep winkle, we use the current HSPRG0/PORE
trick. We program the PORE engine (which is used for state restore when waking
up from deep winkle) to restore HSPRG0 with the last bit set (we do this in
pnv_save_sprs_for_winkle()). R13 bit manipulation in pnv_restore_hyp_resource
is related to this.

Thanks,
Shreyas

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Nicholas Piggin]

On Thu, 3 Nov 2016 01:56:46 -0400
"Shreyas B. Prabhu" <shreyasbp@gmail.com> wrote:

> On Thu, Nov 3, 2016 at 1:21 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> > On Wed, 2 Nov 2016 14:15:48 +0530
> > Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> >  
> >> Hi Nick,
> >>
> >> On Wed, Nov 02, 2016 at 07:36:24PM +1100, Nicholas Piggin wrote:  
> >> >
> >> > Okay, I'll work with that. What's the best way to make a P8 do
> >> > winkle sleeps?  
> >>
> >> From the userspace, offlining the CPUs of the core will put them to
> >> winkle.  
> >
> > Thanks for this. Hum, that r13 manipulation throughout the idle
> > and exception code is a bit interesting. I'll do the minimal patch
> > for 4.9, but what's the reason not to just use the winkle state
> > in the PACA rather than storing it into HSPRG0 bit, can you (or
> > Shreyas) explain?
> >  
> Hi Nick,
> 
> Before deep winkle, checking SRR1's wakeup bits (Bits 46:47) was enough to
> figure out which idle state we are waking up from. But in P8, SRR1's wakeup
> bits aren't enough since bits 46:47 are 0b11 for both fast sleep and
> deep winkle.
> So to distinguish bw fastsleep and deep winkle, we use the current HSPRG0/PORE
> trick. We program the PORE engine (which is used for state restore when waking
> up from deep winkle) to restore HSPRG0 with the last bit set (we do this in
> pnv_save_sprs_for_winkle()). R13 bit manipulation in pnv_restore_hyp_resource
> is related to this.

Right, I didn't realize how that exactly worked until I had to go read
the code just now. It's a neat little trick. I'm wondering can we use PACA_THREAD_IDLE_STATE==PNV_THREAD_WINKLE for this instead? It would just
make the early PACA usage in the exception handlers able to use more common
code.

Thanks,
Nick

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Shreyas B. Prabhu]

On Thu, Nov 3, 2016 at 2:17 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> On Thu, 3 Nov 2016 01:56:46 -0400
> "Shreyas B. Prabhu" <shreyasbp@gmail.com> wrote:
>
>> On Thu, Nov 3, 2016 at 1:21 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
>> > On Wed, 2 Nov 2016 14:15:48 +0530
>> > Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
>> >
>> >> Hi Nick,
>> >>
>> >> On Wed, Nov 02, 2016 at 07:36:24PM +1100, Nicholas Piggin wrote:
>> >> >
>> >> > Okay, I'll work with that. What's the best way to make a P8 do
>> >> > winkle sleeps?
>> >>
>> >> From the userspace, offlining the CPUs of the core will put them to
>> >> winkle.
>> >
>> > Thanks for this. Hum, that r13 manipulation throughout the idle
>> > and exception code is a bit interesting. I'll do the minimal patch
>> > for 4.9, but what's the reason not to just use the winkle state
>> > in the PACA rather than storing it into HSPRG0 bit, can you (or
>> > Shreyas) explain?
>> >
>> Hi Nick,
>>
>> Before deep winkle, checking SRR1's wakeup bits (Bits 46:47) was enough to
>> figure out which idle state we are waking up from. But in P8, SRR1's wakeup
>> bits aren't enough since bits 46:47 are 0b11 for both fast sleep and
>> deep winkle.
>> So to distinguish bw fastsleep and deep winkle, we use the current HSPRG0/PORE
>> trick. We program the PORE engine (which is used for state restore when waking
>> up from deep winkle) to restore HSPRG0 with the last bit set (we do this in
>> pnv_save_sprs_for_winkle()). R13 bit manipulation in pnv_restore_hyp_resource
>> is related to this.
>
> Right, I didn't realize how that exactly worked until I had to go read
> the code just now. It's a neat little trick. I'm wondering can we use PACA_THREAD_IDLE_STATE==PNV_THREAD_WINKLE for this instead? It would just
> make the early PACA usage in the exception handlers able to use more common
> code.
>

PACA_THREAD_IDLE_STATE will have what was 'requested'. It may not be the
state we are waking up from. For example, if 7 threads of the core execute
winkle instruction while 1 thread of the same core executes sleep. Here
the core only enters sleep whereas PACA_THREAD_IDLE_STATE for the 7 threads
will have PNV_THREAD_WINKLE.

Thanks,
Shreyas

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Nicholas Piggin]

On Thu, 3 Nov 2016 02:32:39 -0400
"Shreyas B. Prabhu" <shreyasbp@gmail.com> wrote:

> On Thu, Nov 3, 2016 at 2:17 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> > On Thu, 3 Nov 2016 01:56:46 -0400
> > "Shreyas B. Prabhu" <shreyasbp@gmail.com> wrote:
> >  
> >> On Thu, Nov 3, 2016 at 1:21 AM, Nicholas Piggin <npiggin@gmail.com> wrote:  
> >> > On Wed, 2 Nov 2016 14:15:48 +0530
> >> > Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> >> >  
> >> >> Hi Nick,
> >> >>
> >> >> On Wed, Nov 02, 2016 at 07:36:24PM +1100, Nicholas Piggin wrote:  
> >> >> >
> >> >> > Okay, I'll work with that. What's the best way to make a P8 do
> >> >> > winkle sleeps?  
> >> >>
> >> >> From the userspace, offlining the CPUs of the core will put them to
> >> >> winkle.  
> >> >
> >> > Thanks for this. Hum, that r13 manipulation throughout the idle
> >> > and exception code is a bit interesting. I'll do the minimal patch
> >> > for 4.9, but what's the reason not to just use the winkle state
> >> > in the PACA rather than storing it into HSPRG0 bit, can you (or
> >> > Shreyas) explain?
> >> >  
> >> Hi Nick,
> >>
> >> Before deep winkle, checking SRR1's wakeup bits (Bits 46:47) was enough to
> >> figure out which idle state we are waking up from. But in P8, SRR1's wakeup
> >> bits aren't enough since bits 46:47 are 0b11 for both fast sleep and
> >> deep winkle.
> >> So to distinguish bw fastsleep and deep winkle, we use the current HSPRG0/PORE
> >> trick. We program the PORE engine (which is used for state restore when waking
> >> up from deep winkle) to restore HSPRG0 with the last bit set (we do this in
> >> pnv_save_sprs_for_winkle()). R13 bit manipulation in pnv_restore_hyp_resource
> >> is related to this.  
> >
> > Right, I didn't realize how that exactly worked until I had to go read
> > the code just now. It's a neat little trick. I'm wondering can we use PACA_THREAD_IDLE_STATE==PNV_THREAD_WINKLE for this instead? It would just
> > make the early PACA usage in the exception handlers able to use more common
> > code.
> >  
> 
> PACA_THREAD_IDLE_STATE will have what was 'requested'. It may not be the
> state we are waking up from. For example, if 7 threads of the core execute
> winkle instruction while 1 thread of the same core executes sleep. Here
> the core only enters sleep whereas PACA_THREAD_IDLE_STATE for the 7 threads
> will have PNV_THREAD_WINKLE.

I see, that makes sense. Would it be possible to keep count of the number of
threads going into winkle in core_idle_state? Even if that is not a guarantee
if them requiring a PORE wakeup, would the restore case be harmful?

Thanks,
Nick

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Gautham R Shenoy]

Hi Nick,

On Thu, Nov 03, 2016 at 06:02:44PM +1100, Nicholas Piggin wrote:
> On Thu, 3 Nov 2016 02:32:39 -0400
> "Shreyas B. Prabhu" <shreyasbp@gmail.com> wrote:
> 
> > On Thu, Nov 3, 2016 at 2:17 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> > > On Thu, 3 Nov 2016 01:56:46 -0400
> > > "Shreyas B. Prabhu" <shreyasbp@gmail.com> wrote:
> > >  
> > >> On Thu, Nov 3, 2016 at 1:21 AM, Nicholas Piggin <npiggin@gmail.com> wrote:  
> > >> > On Wed, 2 Nov 2016 14:15:48 +0530
> > >> > Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> > >> >  
> > >> >> Hi Nick,
> > >> >>
> > >> >> On Wed, Nov 02, 2016 at 07:36:24PM +1100, Nicholas Piggin wrote:  
> > >> >> >
> > >> >> > Okay, I'll work with that. What's the best way to make a P8 do
> > >> >> > winkle sleeps?  
> > >> >>
> > >> >> From the userspace, offlining the CPUs of the core will put them to
> > >> >> winkle.  
> > >> >
> > >> > Thanks for this. Hum, that r13 manipulation throughout the idle
> > >> > and exception code is a bit interesting. I'll do the minimal patch
> > >> > for 4.9, but what's the reason not to just use the winkle state
> > >> > in the PACA rather than storing it into HSPRG0 bit, can you (or
> > >> > Shreyas) explain?
> > >> >  
> > >> Hi Nick,
> > >>
> > >> Before deep winkle, checking SRR1's wakeup bits (Bits 46:47) was enough to
> > >> figure out which idle state we are waking up from. But in P8, SRR1's wakeup
> > >> bits aren't enough since bits 46:47 are 0b11 for both fast sleep and
> > >> deep winkle.
> > >> So to distinguish bw fastsleep and deep winkle, we use the current HSPRG0/PORE
> > >> trick. We program the PORE engine (which is used for state restore when waking
> > >> up from deep winkle) to restore HSPRG0 with the last bit set (we do this in
> > >> pnv_save_sprs_for_winkle()). R13 bit manipulation in pnv_restore_hyp_resource
> > >> is related to this.  
> > >
> > > Right, I didn't realize how that exactly worked until I had to go read
> > > the code just now. It's a neat little trick. I'm wondering can we use PACA_THREAD_IDLE_STATE==PNV_THREAD_WINKLE for this instead? It would just
> > > make the early PACA usage in the exception handlers able to use more common
> > > code.
> > >  
> > 
> > PACA_THREAD_IDLE_STATE will have what was 'requested'. It may not be the
> > state we are waking up from. For example, if 7 threads of the core execute
> > winkle instruction while 1 thread of the same core executes sleep. Here
> > the core only enters sleep whereas PACA_THREAD_IDLE_STATE for the 7 threads
> > will have PNV_THREAD_WINKLE.
> 
> I see, that makes sense. Would it be possible to keep count of the number of
> threads going into winkle in core_idle_state? Even if that is not a guarantee
> if them requiring a PORE wakeup, would the restore case be harmful?

Doing a full restore on wakeup when the hardware didn't actually go to
winkle isn't harmful. The first few iterations of the winkle
enablement patchset based the decision on
PACA_THREAD_IDLE_STATE==PNV_THREAD_WINKLE upon a wakeup.

The disadvantage was that we would end up restoring a whole bunch of
Subcore SPRs (SDR1, RPR, AMOR), Core SPRs (TSCR,WORC) and per thread
SPRs (SLBs, SPURR,PURR,DSCR,WORT) which would waste quite lot of
cycles if the hardware didn't actually demote the CPU all the way to
winkle.

Hence Ben suggested piggybacking on PORE engine to set the LSB of the
HSPRG0 to indicate wakeup from winkle.

> 
> Thanks,
> Nick
> 
--
Thanks and Regards
gautham.

Re: [PATCH] powerpc/64s: relocation, register save fixes for system reset interrupt

[Nicholas Piggin]

On Fri, 4 Nov 2016 14:34:09 +0530
Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:

> Hi Nick,
> 
> On Thu, Nov 03, 2016 at 06:02:44PM +1100, Nicholas Piggin wrote:
> > On Thu, 3 Nov 2016 02:32:39 -0400
> > "Shreyas B. Prabhu" <shreyasbp@gmail.com> wrote:
> >   
> > > On Thu, Nov 3, 2016 at 2:17 AM, Nicholas Piggin <npiggin@gmail.com> wrote:  
> > > > On Thu, 3 Nov 2016 01:56:46 -0400
> > > > "Shreyas B. Prabhu" <shreyasbp@gmail.com> wrote:
> > > >    
> > > >> On Thu, Nov 3, 2016 at 1:21 AM, Nicholas Piggin <npiggin@gmail.com> wrote:    
> > > >> > On Wed, 2 Nov 2016 14:15:48 +0530
> > > >> > Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> > > >> >    
> > > >> >> Hi Nick,
> > > >> >>
> > > >> >> On Wed, Nov 02, 2016 at 07:36:24PM +1100, Nicholas Piggin wrote:    
> > > >> >> >
> > > >> >> > Okay, I'll work with that. What's the best way to make a P8 do
> > > >> >> > winkle sleeps?    
> > > >> >>
> > > >> >> From the userspace, offlining the CPUs of the core will put them to
> > > >> >> winkle.    
> > > >> >
> > > >> > Thanks for this. Hum, that r13 manipulation throughout the idle
> > > >> > and exception code is a bit interesting. I'll do the minimal patch
> > > >> > for 4.9, but what's the reason not to just use the winkle state
> > > >> > in the PACA rather than storing it into HSPRG0 bit, can you (or
> > > >> > Shreyas) explain?
> > > >> >    
> > > >> Hi Nick,
> > > >>
> > > >> Before deep winkle, checking SRR1's wakeup bits (Bits 46:47) was enough to
> > > >> figure out which idle state we are waking up from. But in P8, SRR1's wakeup
> > > >> bits aren't enough since bits 46:47 are 0b11 for both fast sleep and
> > > >> deep winkle.
> > > >> So to distinguish bw fastsleep and deep winkle, we use the current HSPRG0/PORE
> > > >> trick. We program the PORE engine (which is used for state restore when waking
> > > >> up from deep winkle) to restore HSPRG0 with the last bit set (we do this in
> > > >> pnv_save_sprs_for_winkle()). R13 bit manipulation in pnv_restore_hyp_resource
> > > >> is related to this.    
> > > >
> > > > Right, I didn't realize how that exactly worked until I had to go read
> > > > the code just now. It's a neat little trick. I'm wondering can we use PACA_THREAD_IDLE_STATE==PNV_THREAD_WINKLE for this instead? It would just
> > > > make the early PACA usage in the exception handlers able to use more common
> > > > code.
> > > >    
> > > 
> > > PACA_THREAD_IDLE_STATE will have what was 'requested'. It may not be the
> > > state we are waking up from. For example, if 7 threads of the core execute
> > > winkle instruction while 1 thread of the same core executes sleep. Here
> > > the core only enters sleep whereas PACA_THREAD_IDLE_STATE for the 7 threads
> > > will have PNV_THREAD_WINKLE.  
> > 
> > I see, that makes sense. Would it be possible to keep count of the number of
> > threads going into winkle in core_idle_state? Even if that is not a guarantee
> > if them requiring a PORE wakeup, would the restore case be harmful?  
> 
> Doing a full restore on wakeup when the hardware didn't actually go to
> winkle isn't harmful. The first few iterations of the winkle
> enablement patchset based the decision on
> PACA_THREAD_IDLE_STATE==PNV_THREAD_WINKLE upon a wakeup.
> 
> The disadvantage was that we would end up restoring a whole bunch of
> Subcore SPRs (SDR1, RPR, AMOR), Core SPRs (TSCR,WORC) and per thread
> SPRs (SLBs, SPURR,PURR,DSCR,WORT) which would waste quite lot of
> cycles if the hardware didn't actually demote the CPU all the way to
> winkle.
> 
> Hence Ben suggested piggybacking on PORE engine to set the LSB of the
> HSPRG0 to indicate wakeup from winkle.

Hi Gautham,

Okay this makes sense, thanks for the clarification.

Thanks,
Nick