[PATCH 0/2 v2] powerpc: Make ptrace work reliably

[PATCH 0/2 v2] powerpc: Make ptrace work reliably

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

v1->v2
 - Subject line was missing 0/2, 1/2, 2/2

Bharat Bhushan (2):
  powerpc: debug control and status registers are 32bit
=> This patch makes debug control and status registers as 32bit as they are.
   This does not fix anything

  powerpc: restore dbcr0 on user space exit
=> This patch fixes the ptrace reliability issue. The description is the patch
   describes one of the case where it does not work reliably

 arch/powerpc/include/asm/processor.h |    8 ++++----
 arch/powerpc/kernel/asm-offsets.c    |    1 +
 arch/powerpc/kernel/entry_64.S       |   24 ++++++++++++++++++++----
 3 files changed, 25 insertions(+), 8 deletions(-)

[PATCH 1/2 v2] powerpc: debug control and status registers are 32bit

[Bharat Bhushan]

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v1->v2
 - Subject line was not having 1/2

 arch/powerpc/include/asm/processor.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index d7e67ca..5213577 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -168,10 +168,10 @@ struct thread_struct {
 	 * The following help to manage the use of Debug Control Registers
 	 * om the BookE platforms.
 	 */
-	unsigned long	dbcr0;
-	unsigned long	dbcr1;
+	uint32_t	dbcr0;
+	uint32_t	dbcr1;
 #ifdef CONFIG_BOOKE
-	unsigned long	dbcr2;
+	uint32_t	dbcr2;
 #endif
 	/*
 	 * The stored value of the DBSR register will be the value at the
@@ -179,7 +179,7 @@ struct thread_struct {
 	 * user (will never be written to) and has value while helping to
 	 * describe the reason for the last debug trap.  Torez
 	 */
-	unsigned long	dbsr;
+	uint32_t	dbsr;
 	/*
 	 * The following will contain addresses used by debug applications
 	 * to help trace and trap on particular address locations.
-- 
1.5.6.5

[PATCH 2/2 v2] powerpc: restore dbcr0 on user space exit

[Bharat Bhushan]

On BookE (Branch taken + Single Step) is as same as Branch Taken
on BookS and in Linux we simulate BookS behavior for BookE as well.
When doing so, in Branch taken handling we want to set DBCR0_IC but
we update the current->thread->dbcr0 and not DBCR0.

Now on 64bit the current->thread.dbcr0 (and other debug registers)
is synchronized ONLY on context switch flow. But after handling
Branch taken in debug exception if we return back to user space
without context switch then single stepping change (DBCR0_ICMP)
does not get written in h/w DBCR0 and Instruction Complete exception
does not happen.

This fixes using ptrace reliably on BookE-PowerPC

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v1->v2
 - Subject line was not having 2/2

 arch/powerpc/kernel/asm-offsets.c |    1 +
 arch/powerpc/kernel/entry_64.S    |   24 ++++++++++++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index b51a97c..1e2f450 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -103,6 +103,7 @@ int main(void)
 #endif /* CONFIG_VSX */
 #ifdef CONFIG_PPC64
 	DEFINE(KSP_VSID, offsetof(struct thread_struct, ksp_vsid));
+	DEFINE(THREAD_DBCR0, offsetof(struct thread_struct, dbcr0));
 #else /* CONFIG_PPC64 */
 	DEFINE(PGDIR, offsetof(struct thread_struct, pgdir));
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 794889b..561630d 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -614,7 +614,9 @@ _GLOBAL(ret_from_except_lite)
 	 * from the interrupt.
 	 */
 #ifdef CONFIG_PPC_BOOK3E
+	ld	r3,PACACURRENT(r13)
 	wrteei	0
+	lwz	r10,(THREAD+THREAD_DBCR0)(r3)
 #else
 	ld	r10,PACAKMSR(r13) /* Get kernel MSR without EE */
 	mtmsrd	r10,1		  /* Update machine state */
@@ -628,15 +630,29 @@ _GLOBAL(ret_from_except_lite)
 
 	/* Check current_thread_info()->flags */
 	andi.	r0,r4,_TIF_USER_WORK_MASK
+	bne	1f
+#ifdef CONFIG_PPC_BOOK3E
+	/*
+	 * Check to see if the dbcr0 register is set up to debug.
+	 * Use the internal debug mode bit to do this.
+	 */
+	andis.	r0,r10,DBCR0_IDM@h
 	beq	restore
-
-	andi.	r0,r4,_TIF_NEED_RESCHED
-	beq	1f
+	mfmsr	r0
+	rlwinm	r0,r0,0,~MSR_DE	/* Clear MSR.DE */
+	mtmsr	r0
+	mtspr	SPRN_DBCR0,r10
+	li	r10, -1
+	mtspr	SPRN_DBSR,r10
+	b	restore
+#endif
+1:	andi.	r0,r4,_TIF_NEED_RESCHED
+	beq	2f
 	bl	.restore_interrupts
 	SCHEDULE_USER
 	b	.ret_from_except_lite
 
-1:	bl	.save_nvgprs
+2:	bl	.save_nvgprs
 	bl	.restore_interrupts
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	.do_notify_resume
-- 
1.5.6.5

Re: [PATCH 2/2 v2] powerpc: restore dbcr0 on user space exit

[Scott Wood]

On 05/16/2013 12:34:32 AM, Bharat Bhushan wrote:
> On BookE (Branch taken + Single Step) is as same as Branch Taken
> on BookS and in Linux we simulate BookS behavior for BookE as well.
> When doing so, in Branch taken handling we want to set DBCR0_IC but
> we update the current->thread->dbcr0 and not DBCR0.
>=20
> Now on 64bit the current->thread.dbcr0 (and other debug registers)
> is synchronized ONLY on context switch flow. But after handling
> Branch taken in debug exception if we return back to user space
> without context switch then single stepping change (DBCR0_ICMP)
> does not get written in h/w DBCR0 and Instruction Complete exception
> does not happen.
>=20
> This fixes using ptrace reliably on BookE-PowerPC
>=20
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> v1->v2
>  - Subject line was not having 2/2
>=20
>  arch/powerpc/kernel/asm-offsets.c |    1 +
>  arch/powerpc/kernel/entry_64.S    |   24 ++++++++++++++++++++----
>  2 files changed, 21 insertions(+), 4 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/asm-offsets.c =20
> b/arch/powerpc/kernel/asm-offsets.c
> index b51a97c..1e2f450 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -103,6 +103,7 @@ int main(void)
>  #endif /* CONFIG_VSX */
>  #ifdef CONFIG_PPC64
>  	DEFINE(KSP_VSID, offsetof(struct thread_struct, ksp_vsid));
> +	DEFINE(THREAD_DBCR0, offsetof(struct thread_struct, dbcr0));
>  #else /* CONFIG_PPC64 */
>  	DEFINE(PGDIR, offsetof(struct thread_struct, pgdir));
>  #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
> diff --git a/arch/powerpc/kernel/entry_64.S =20
> b/arch/powerpc/kernel/entry_64.S
> index 794889b..561630d 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -614,7 +614,9 @@ _GLOBAL(ret_from_except_lite)
>  	 * from the interrupt.
>  	 */
>  #ifdef CONFIG_PPC_BOOK3E
> +	ld	r3,PACACURRENT(r13)
>  	wrteei	0
> +	lwz	r10,(THREAD+THREAD_DBCR0)(r3)

I know I asked you to move these earlier, but this is probably too =20
early... wrteei has synchronization, so it will probably have to wait =20
until the ld completes, defeating the purpose of moving it earlier.

Ideal would probably be to load PACACURRENT immediately after _MSR(r1), =20
and load DBCR0 just after "beq resume_kernel".

Or, move DBCR0 to therad_info as I suggested internally.

Regardless of what you do, could you run a basic syscall benchmark =20
(e.g. from lmbench) before and after the patch?

-Scott=

RE: [PATCH 2/2 v2] powerpc: restore dbcr0 on user space exit

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, May 16, 2013 10:24 PM
> To: Bhushan Bharat-R65777
> Cc: galak@kernel.crashing.org; benh@kernel.crashing.org; linuxppc-
> dev@lists.ozlabs.org; Yoder Stuart-B08248; Yang James-RA8135; Bhushan Bha=
rat-
> R65777
> Subject: Re: [PATCH 2/2 v2] powerpc: restore dbcr0 on user space exit
>=20
> On 05/16/2013 12:34:32 AM, Bharat Bhushan wrote:
> > On BookE (Branch taken + Single Step) is as same as Branch Taken on
> > BookS and in Linux we simulate BookS behavior for BookE as well.
> > When doing so, in Branch taken handling we want to set DBCR0_IC but we
> > update the current->thread->dbcr0 and not DBCR0.
> >
> > Now on 64bit the current->thread.dbcr0 (and other debug registers) is
> > synchronized ONLY on context switch flow. But after handling Branch
> > taken in debug exception if we return back to user space without
> > context switch then single stepping change (DBCR0_ICMP) does not get
> > written in h/w DBCR0 and Instruction Complete exception does not
> > happen.
> >
> > This fixes using ptrace reliably on BookE-PowerPC
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > v1->v2
> >  - Subject line was not having 2/2
> >
> >  arch/powerpc/kernel/asm-offsets.c |    1 +
> >  arch/powerpc/kernel/entry_64.S    |   24 ++++++++++++++++++++----
> >  2 files changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/asm-offsets.c
> > b/arch/powerpc/kernel/asm-offsets.c
> > index b51a97c..1e2f450 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -103,6 +103,7 @@ int main(void)
> >  #endif /* CONFIG_VSX */
> >  #ifdef CONFIG_PPC64
> >  	DEFINE(KSP_VSID, offsetof(struct thread_struct, ksp_vsid));
> > +	DEFINE(THREAD_DBCR0, offsetof(struct thread_struct, dbcr0));
> >  #else /* CONFIG_PPC64 */
> >  	DEFINE(PGDIR, offsetof(struct thread_struct, pgdir));  #if
> > defined(CONFIG_4xx) || defined(CONFIG_BOOKE) diff --git
> > a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 794889b..561630d 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -614,7 +614,9 @@ _GLOBAL(ret_from_except_lite)
> >  	 * from the interrupt.
> >  	 */
> >  #ifdef CONFIG_PPC_BOOK3E
> > +	ld	r3,PACACURRENT(r13)
> >  	wrteei	0
> > +	lwz	r10,(THREAD+THREAD_DBCR0)(r3)
>=20
> I know I asked you to move these earlier, but this is probably too early.=
..
> wrteei has synchronization, so it will probably have to wait until the ld
> completes, defeating the purpose of moving it earlier.
>=20
> Ideal would probably be to load PACACURRENT immediately after _MSR(r1), a=
nd load
> DBCR0 just after "beq resume_kernel".

ok

>=20
> Or, move DBCR0 to therad_info as I suggested internally.

If no one have objection on moving dbcr0 to thread_info then I am happy to =
do that.

>=20
> Regardless of what you do, could you run a basic syscall benchmark (e.g. =
from
> lmbench) before and after the patch?

Sure.

-Bharat
>=20
> -Scott