[PATCH] Make system_reset_pSeries relocatable

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

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 :-)

On Wed, Jul 27, 2016 at 7:53 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

On Wed, 2016-07-27 at 17:32 +1000, Balbir Singh wrote:

quoted

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

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':

quoted

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'

quoted

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

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

On Wed, 27 Jul 2016 17:32:06 +1000
Balbir Singh [off-list ref] wrote:

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

On Wed, 27 Jul 2016 20:01:24 +1000
Balbir Singh [off-list ref] wrote:

On Wed, Jul 27, 2016 at 7:53 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

quoted

On Wed, 2016-07-27 at 17:32 +1000, Balbir Singh wrote:  

quoted

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

On Wed, Jul 27, 2016 at 09:50:03PM +1000, Nicholas Piggin wrote:

On Wed, 27 Jul 2016 17:32:06 +1000
Balbir Singh [off-list ref] wrote:

quoted

+#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