[PATCH] Make system_reset_pSeries relocatable

[PATCH] Make system_reset_pSeries relocatable

[Balbir Singh]

From: Balbir Singh <bsingharora@gmail.com>

Currently the power management bits are broken w.r.t. relocation.
There are direct branches from system_reset_pSeries to
power7_wakeup_*. The correct way to do it is to do what
the slb miss handler does, which is jump to a small stub within
the first 64k of the relocated address and then jump to the
actual location.

The code has been lightly tested (not the kvm bits), I would highly
appreciate a review of the code. I suspect there might be easy
to find bugs :)

Cc: benh@kernel.crashing.org
Cc: mpe@ellerman.id.au
Cc: paulus@samba.org
Cc: npiggin@gmail.com
Cc: svaidy@linux.vnet.ibm.com

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/kernel/exceptions-64s.S | 82 ++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 31 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 8bcc1b4..64f9650 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -118,39 +118,21 @@ BEGIN_FTR_SECTION
 	cmpwi	cr4,r5,1
 	mtspr	SPRN_HSPRG0,r13
 
-	lbz	r0,PACA_THREAD_IDLE_STATE(r13)
-	cmpwi   cr2,r0,PNV_THREAD_NAP
-	bgt     cr2,8f				/* Either sleep or Winkle */
-
-	/* Waking up from nap should not cause hypervisor state loss */
-	bgt	cr3,.
-
-	/* Waking up from nap */
-	li	r0,PNV_THREAD_RUNNING
-	stb	r0,PACA_THREAD_IDLE_STATE(r13)	/* Clear thread state */
-
-#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
-	li	r0,KVM_HWTHREAD_IN_KERNEL
-	stb	r0,HSTATE_HWTHREAD_STATE(r13)
-	/* Order setting hwthread_state vs. testing hwthread_req */
-	sync
-	lbz	r0,HSTATE_HWTHREAD_REQ(r13)
-	cmpwi	r0,0
-	beq	1f
-	b	kvm_start_guest
-1:
+#ifndef CONFIG_RELOCATABLE
+	b	power7_wakeup_common
+#else
+	/*
+	 * We can't just use a direct branch to power7_wakeup_common
+	 * because the distance from here to there depends on where
+	 * the kernel ends up being put.
+	 */
+	mfctr	r11
+	ld	r10, PACAKBASE(r13)
+	LOAD_HANDLER(r10, power7_wakeup_common)
+	mtctr	r10
+	bctr
 #endif
 
-	/* Return SRR1 from power7_nap() */
-	mfspr	r3,SPRN_SRR1
-	beq	cr3,2f
-	b	power7_wakeup_noloss
-2:	b	power7_wakeup_loss
-
-	/* Fast Sleep wakeup on PowerNV */
-8:	GET_PACA(r13)
-	b 	power7_wakeup_tb_loss
-
 9:
 END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
 #endif /* CONFIG_PPC_P7_NAP */
@@ -1448,6 +1430,44 @@ power4_fixup_nap:
 	blr
 #endif
 
+	.align 7
+_GLOBAL(power7_wakeup_common)
+#ifdef CONFIG_RELOCATABLE
+	mtctr	r11
+#endif
+	lbz	r0,PACA_THREAD_IDLE_STATE(r13)
+	cmpwi   cr2,r0,PNV_THREAD_NAP
+	bgt     cr2,8f				/* Either sleep or Winkle */
+
+	/* Waking up from nap should not cause hypervisor state loss */
+	bgt	cr3,.
+
+	/* Waking up from nap */
+	li	r0,PNV_THREAD_RUNNING
+	stb	r0,PACA_THREAD_IDLE_STATE(r13)	/* Clear thread state */
+
+#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+	li	r0,KVM_HWTHREAD_IN_KERNEL
+	stb	r0,HSTATE_HWTHREAD_STATE(r13)
+	/* Order setting hwthread_state vs. testing hwthread_req */
+	sync
+	lbz	r0,HSTATE_HWTHREAD_REQ(r13)
+	cmpwi	r0,0
+	beq	1f
+	b	kvm_start_guest
+1:
+#endif
+
+	/* Return SRR1 from power7_nap() */
+	mfspr	r3,SPRN_SRR1
+	beq	cr3,2f
+	b	power7_wakeup_noloss
+2:	b	power7_wakeup_loss
+
+	/* Fast Sleep wakeup on PowerNV */
+8:	GET_PACA(r13)
+	b 	power7_wakeup_tb_loss
+
 /*
  * Hash table stuff
  */
-- 
2.5.5

Re: [PATCH] Make system_reset_pSeries relocatable

[Benjamin Herrenschmidt]

On Wed, 2016-07-27 at 17:32 +1000, Balbir Singh wrote:
> From: Balbir Singh <bsingharora@gmail.com>
> 
> Currently the power management bits are broken w.r.t. relocation.
> There are direct branches from system_reset_pSeries to
> power7_wakeup_*.

Side track: we should really get rid of the _pSeries suffix for these
things :-)

>  The correct way to do it is to do what
> the slb miss handler does, which is jump to a small stub within
> the first 64k of the relocated address and then jump to the
> actual location.
> 
> The code has been lightly tested (not the kvm bits), I would highly
> appreciate a review of the code. I suspect there might be easy
> to find bugs :)
> 
> Cc: benh@kernel.crashing.org
> Cc: mpe@ellerman.id.au
> Cc: paulus@samba.org
> Cc: npiggin@gmail.com
> Cc: svaidy@linux.vnet.ibm.com
> 
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/kernel/exceptions-64s.S | 82 ++++++++++++++++++++++--
> ------------
>  1 file changed, 51 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64s.S
> b/arch/powerpc/kernel/exceptions-64s.S
> index 8bcc1b4..64f9650 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -118,39 +118,21 @@ BEGIN_FTR_SECTION
>  	cmpwi	cr4,r5,1
>  	mtspr	SPRN_HSPRG0,r13
>  
> -	lbz	r0,PACA_THREAD_IDLE_STATE(r13)
> -	cmpwi   cr2,r0,PNV_THREAD_NAP
> -	bgt     cr2,8f				/* Either
> sleep or Winkle */
> -
> -	/* Waking up from nap should not cause hypervisor state loss
> */
> -	bgt	cr3,.
> -
> -	/* Waking up from nap */
> -	li	r0,PNV_THREAD_RUNNING
> -	stb	r0,PACA_THREAD_IDLE_STATE(r13)	/* Clear
> thread state */
> -
> -#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> -	li	r0,KVM_HWTHREAD_IN_KERNEL
> -	stb	r0,HSTATE_HWTHREAD_STATE(r13)
> -	/* Order setting hwthread_state vs. testing hwthread_req */
> -	sync
> -	lbz	r0,HSTATE_HWTHREAD_REQ(r13)
> -	cmpwi	r0,0
> -	beq	1f
> -	b	kvm_start_guest
> -1:
> +#ifndef CONFIG_RELOCATABLE
> +	b	power7_wakeup_common
> +#else
> +	/*
> +	 * We can't just use a direct branch to power7_wakeup_common
> +	 * because the distance from here to there depends on where
> +	 * the kernel ends up being put.
> +	 */
> +	mfctr	r11
> +	ld	r10, PACAKBASE(r13)
> +	LOAD_HANDLER(r10, power7_wakeup_common)
> +	mtctr	r10
> +	bctr
>  #endif
>  
> -	/* Return SRR1 from power7_nap() */
> -	mfspr	r3,SPRN_SRR1
> -	beq	cr3,2f
> -	b	power7_wakeup_noloss
> -2:	b	power7_wakeup_loss
> -
> -	/* Fast Sleep wakeup on PowerNV */
> -8:	GET_PACA(r13)
> -	b 	power7_wakeup_tb_loss
> -
>  9:
>  END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
>  #endif /* CONFIG_PPC_P7_NAP */
> @@ -1448,6 +1430,44 @@ power4_fixup_nap:
>  	blr
>  #endif
>  
> +	.align 7
> +_GLOBAL(power7_wakeup_common)
> +#ifdef CONFIG_RELOCATABLE
> +	mtctr	r11
> +#endif
> +	lbz	r0,PACA_THREAD_IDLE_STATE(r13)
> +	cmpwi   cr2,r0,PNV_THREAD_NAP
> +	bgt     cr2,8f				/* Either
> sleep or Winkle */
> +
> +	/* Waking up from nap should not cause hypervisor state loss
> */
> +	bgt	cr3,.
> +
> +	/* Waking up from nap */
> +	li	r0,PNV_THREAD_RUNNING
> +	stb	r0,PACA_THREAD_IDLE_STATE(r13)	/* Clear
> thread state */
> +
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +	li	r0,KVM_HWTHREAD_IN_KERNEL
> +	stb	r0,HSTATE_HWTHREAD_STATE(r13)
> +	/* Order setting hwthread_state vs. testing hwthread_req */
> +	sync
> +	lbz	r0,HSTATE_HWTHREAD_REQ(r13)
> +	cmpwi	r0,0
> +	beq	1f
> +	b	kvm_start_guest
> +1:
> +#endif
> +
> +	/* Return SRR1 from power7_nap() */
> +	mfspr	r3,SPRN_SRR1
> +	beq	cr3,2f
> +	b	power7_wakeup_noloss
> +2:	b	power7_wakeup_loss
> +
> +	/* Fast Sleep wakeup on PowerNV */
> +8:	GET_PACA(r13)
> +	b 	power7_wakeup_tb_loss
> +
>  /*
>   * Hash table stuff
>   */

Re: [PATCH] Make system_reset_pSeries relocatable

[Balbir Singh]

On Wed, Jul 27, 2016 at 7:53 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2016-07-27 at 17:32 +1000, Balbir Singh wrote:
>> From: Balbir Singh <bsingharora@gmail.com>
>>
>> Currently the power management bits are broken w.r.t. relocation.
>> There are direct branches from system_reset_pSeries to
>> power7_wakeup_*.
>
> Side track: we should really get rid of the _pSeries suffix for these
> things :-)
>

I thought about it, but then thought it would need more auditing
across places and hence held back. I'll be happy to send another patch
to remove _pSeries from the exception-64s.S to begin with as a follow
up

Balbir

Re: [PATCH] Make system_reset_pSeries relocatable

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2783 bytes --]

Hi,

[auto build test ERROR on v4.7-rc7]
[cannot apply to powerpc/next next-20160727]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Balbir-Singh/Make-system_reset_pSeries-relocatable/20160727-153638
config: powerpc-ps3_defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/head_64.o: In function `power7_wakeup_common':
>> arch/powerpc/kernel/exceptions-64s.S:1438: undefined reference to `PACA_THREAD_IDLE_STATE'
>> arch/powerpc/kernel/exceptions-64s.S:1439: undefined reference to `PNV_THREAD_NAP'
>> arch/powerpc/kernel/exceptions-64s.S:1446: undefined reference to `PNV_THREAD_RUNNING'
   arch/powerpc/kernel/exceptions-64s.S:1447: undefined reference to `PACA_THREAD_IDLE_STATE'
>> arch/powerpc/kernel/exceptions-64s.S:1464: undefined reference to `power7_wakeup_noloss'
>> arch/powerpc/kernel/exceptions-64s.S:1465: undefined reference to `power7_wakeup_loss'
>> arch/powerpc/kernel/exceptions-64s.S:1469: undefined reference to `power7_wakeup_tb_loss'

vim +1438 arch/powerpc/kernel/exceptions-64s.S

  1432	
  1433		.align 7
  1434	_GLOBAL(power7_wakeup_common)
  1435	#ifdef CONFIG_RELOCATABLE
  1436		mtctr	r11
  1437	#endif
> 1438		lbz	r0,PACA_THREAD_IDLE_STATE(r13)
> 1439		cmpwi   cr2,r0,PNV_THREAD_NAP
  1440		bgt     cr2,8f				/* Either sleep or Winkle */
  1441	
  1442		/* Waking up from nap should not cause hypervisor state loss */
  1443		bgt	cr3,.
  1444	
  1445		/* Waking up from nap */
> 1446		li	r0,PNV_THREAD_RUNNING
  1447		stb	r0,PACA_THREAD_IDLE_STATE(r13)	/* Clear thread state */
  1448	
  1449	#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
  1450		li	r0,KVM_HWTHREAD_IN_KERNEL
  1451		stb	r0,HSTATE_HWTHREAD_STATE(r13)
  1452		/* Order setting hwthread_state vs. testing hwthread_req */
  1453		sync
  1454		lbz	r0,HSTATE_HWTHREAD_REQ(r13)
  1455		cmpwi	r0,0
  1456		beq	1f
  1457		b	kvm_start_guest
  1458	1:
  1459	#endif
  1460	
  1461		/* Return SRR1 from power7_nap() */
  1462		mfspr	r3,SPRN_SRR1
  1463		beq	cr3,2f
> 1464		b	power7_wakeup_noloss
> 1465	2:	b	power7_wakeup_loss
  1466	
  1467		/* Fast Sleep wakeup on PowerNV */
  1468	8:	GET_PACA(r13)
> 1469		b 	power7_wakeup_tb_loss
  1470	
  1471	/*
  1472	 * Hash table stuff

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 14011 bytes --]

Re: [PATCH] Make system_reset_pSeries relocatable

[Balbir Singh]

On Wed, Jul 27, 2016 at 05:32:06PM +1000, Balbir Singh wrote:
> 
> From: Balbir Singh <bsingharora@gmail.com>
> 
> Currently the power management bits are broken w.r.t. relocation.
> There are direct branches from system_reset_pSeries to
> power7_wakeup_*. The correct way to do it is to do what
> the slb miss handler does, which is jump to a small stub within
> the first 64k of the relocated address and then jump to the
> actual location.
> 
> The code has been lightly tested (not the kvm bits), I would highly
> appreciate a review of the code. I suspect there might be easy
> to find bugs :)
>
Snip
 
> +	.align 7
> +_GLOBAL(power7_wakeup_common)
> +#ifdef CONFIG_RELOCATABLE
> +	mtctr	r11
> +#endif
> +	lbz	r0,PACA_THREAD_IDLE_STATE(r13)
> +	cmpwi   cr2,r0,PNV_THREAD_NAP
> +	bgt     cr2,8f				/* Either sleep or Winkle */
> +
> +	/* Waking up from nap should not cause hypervisor state loss */
> +	bgt	cr3,.
> +
> +	/* Waking up from nap */
> +	li	r0,PNV_THREAD_RUNNING
> +	stb	r0,PACA_THREAD_IDLE_STATE(r13)	/* Clear thread state */
> +
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +	li	r0,KVM_HWTHREAD_IN_KERNEL
> +	stb	r0,HSTATE_HWTHREAD_STATE(r13)
> +	/* Order setting hwthread_state vs. testing hwthread_req */
> +	sync
> +	lbz	r0,HSTATE_HWTHREAD_REQ(r13)
> +	cmpwi	r0,0
> +	beq	1f
> +	b	kvm_start_guest
> +1:
> +#endif
> +
> +	/* Return SRR1 from power7_nap() */
> +	mfspr	r3,SPRN_SRR1
> +	beq	cr3,2f
> +	b	power7_wakeup_noloss
> +2:	b	power7_wakeup_loss
> +
> +	/* Fast Sleep wakeup on PowerNV */
> +8:	GET_PACA(r13)
> +	b 	power7_wakeup_tb_loss
> +
>  /*
>   * Hash table stuff
>   */

As per 0day, this part needs to be under #ifdef CONFIG_PPC_P7_NAP

I'll repose a new version tomorrow.

Balbir Singh

Re: [PATCH] Make system_reset_pSeries relocatable

[Nicholas Piggin]

On Wed, 27 Jul 2016 17:32:06 +1000
Balbir Singh <bsingharora@gmail.com> wrote:

> From: Balbir Singh <bsingharora@gmail.com>
> 
> Currently the power management bits are broken w.r.t. relocation.
> There are direct branches from system_reset_pSeries to
> power7_wakeup_*. The correct way to do it is to do what
> the slb miss handler does, which is jump to a small stub within
> the first 64k of the relocated address and then jump to the
> actual location.
> 
> The code has been lightly tested (not the kvm bits), I would highly
> appreciate a review of the code. I suspect there might be easy
> to find bugs :)
> 
> Cc: benh@kernel.crashing.org
> Cc: mpe@ellerman.id.au
> Cc: paulus@samba.org
> Cc: npiggin@gmail.com
> Cc: svaidy@linux.vnet.ibm.com
> 
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/kernel/exceptions-64s.S | 82
> ++++++++++++++++++++++-------------- 1 file changed, 51
> insertions(+), 31 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64s.S
> b/arch/powerpc/kernel/exceptions-64s.S index 8bcc1b4..64f9650 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -118,39 +118,21 @@ BEGIN_FTR_SECTION
>  	cmpwi	cr4,r5,1
>  	mtspr	SPRN_HSPRG0,r13
>  
> -	lbz	r0,PACA_THREAD_IDLE_STATE(r13)
> -	cmpwi   cr2,r0,PNV_THREAD_NAP
> -	bgt     cr2,8f				/* Either
> sleep or Winkle */ -
> -	/* Waking up from nap should not cause hypervisor state loss
> */
> -	bgt	cr3,.
> -
> -	/* Waking up from nap */
> -	li	r0,PNV_THREAD_RUNNING
> -	stb	r0,PACA_THREAD_IDLE_STATE(r13)	/* Clear
> thread state */ -
> -#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> -	li	r0,KVM_HWTHREAD_IN_KERNEL
> -	stb	r0,HSTATE_HWTHREAD_STATE(r13)
> -	/* Order setting hwthread_state vs. testing hwthread_req */
> -	sync
> -	lbz	r0,HSTATE_HWTHREAD_REQ(r13)
> -	cmpwi	r0,0
> -	beq	1f
> -	b	kvm_start_guest
> -1:
> +#ifndef CONFIG_RELOCATABLE
> +	b	power7_wakeup_common
> +#else
> +	/*
> +	 * We can't just use a direct branch to power7_wakeup_common
> +	 * because the distance from here to there depends on where
> +	 * the kernel ends up being put.
> +	 */
> +	mfctr	r11
> +	ld	r10, PACAKBASE(r13)
> +	LOAD_HANDLER(r10, power7_wakeup_common)
> +	mtctr	r10
> +	bctr
>  #endif

So r10 and r11 are safe to use (as well as existing registers
being used without saving) because we are returning via the nap
functions that caller will expect to trash volatile registers,
yes?

In that case I can't see a problem with this.

Thanks,
Nick

Re: [PATCH] Make system_reset_pSeries relocatable

[Nicholas Piggin]

On Wed, 27 Jul 2016 20:01:24 +1000
Balbir Singh <bsingharora@gmail.com> wrote:

> On Wed, Jul 27, 2016 at 7:53 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Wed, 2016-07-27 at 17:32 +1000, Balbir Singh wrote:  
> >> From: Balbir Singh <bsingharora@gmail.com>
> >>
> >> Currently the power management bits are broken w.r.t. relocation.
> >> There are direct branches from system_reset_pSeries to
> >> power7_wakeup_*.  
> >
> > Side track: we should really get rid of the _pSeries suffix for
> > these things :-)
> >  
> 
> I thought about it, but then thought it would need more auditing
> across places and hence held back. I'll be happy to send another patch
> to remove _pSeries from the exception-64s.S to begin with as a follow
> up

Ahh, my patches do that actually. Not as a primary concern, but
I removed almost all labels and naming from the exception-64s.h file,
and moved them into new head-64.h, so I changed the names a little
in the process.

It would be helpful to not touching this too much before we have some
more discussion of my reworking. I'm happy to rebase my stuff on top of
bugfixes though, of course.

Thanks,
Nick

Re: [PATCH] Make system_reset_pSeries relocatable

[Balbir Singh]

On Wed, Jul 27, 2016 at 09:50:03PM +1000, Nicholas Piggin wrote:
> On Wed, 27 Jul 2016 17:32:06 +1000
> Balbir Singh <bsingharora@gmail.com> wrote:
> 
> > +#ifndef CONFIG_RELOCATABLE
> > +	b	power7_wakeup_common
> > +#else
> > +	/*
> > +	 * We can't just use a direct branch to power7_wakeup_common
> > +	 * because the distance from here to there depends on where
> > +	 * the kernel ends up being put.
> > +	 */
> > +	mfctr	r11
> > +	ld	r10, PACAKBASE(r13)
> > +	LOAD_HANDLER(r10, power7_wakeup_common)
> > +	mtctr	r10
> > +	bctr
> >  #endif
> 
> So r10 and r11 are safe to use (as well as existing registers
> being used without saving) because we are returning via the nap
> functions that caller will expect te trash volatile registers,
> yes?
>

r10, r11 are volatile as per the ABI, so yes.

 
> In that caie I can't see a problem with this.
>

Thanks for the review

Balbir