[PATCH] powerpc/8xx: fix single_step debug

Subsystems: linux for powerpc (32-bit and 64-bit), the rest

STALE3647d

5 messages, 2 authors, 2016-08-18 · open the first message on its own page

[PATCH] powerpc/8xx: fix single_step debug

From: Christophe Leroy <hidden>
Date: 2016-08-18 09:44:25

SPRN_ICR must be read for clearing the internal freeze signal which
is asserted by the single step exception, otherwise the timebase and
decrementer remain freezed

Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/include/asm/reg_8xx.h | 1 +
 arch/powerpc/kernel/traps.c        | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/arch/powerpc/include/asm/reg_8xx.h b/arch/powerpc/include/asm/reg_8xx.h
index feaf641..6dae71f 100644
--- a/arch/powerpc/include/asm/reg_8xx.h
+++ b/arch/powerpc/include/asm/reg_8xx.h

@@ -17,6 +17,7 @@
 #define SPRN_DC_DAT	570	/* Read-only data register */
 
 /* Misc Debug */
+#define SPRN_ICR	148
 #define SPRN_DPDR	630
 #define SPRN_MI_CAM	816
 #define SPRN_MI_RAM0	817

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 2cb5892..0f1f0ce 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c

@@ -400,8 +400,16 @@ static inline int check_io_access(struct pt_regs *regs)
 #define REASON_TRAP		0x20000
 
 #define single_stepping(regs)	((regs)->msr & MSR_SE)
+#ifdef CONFIG_PPC_8xx
+static inline void clear_single_step(struct pt_regs *regs)
+{
+	regs->msr &= ~MSR_SE;
+	mfspr(SPRN_ICR);
+}
+#else
 #define clear_single_step(regs)	((regs)->msr &= ~MSR_SE)
 #endif
+#endif
 
 #if defined(CONFIG_4xx)
 int machine_check_4xx(struct pt_regs *regs)

-- 
2.1.0

Re: [PATCH] powerpc/8xx: fix single_step debug

From: Gabriel Paubert <hidden>
Date: 2016-08-18 09:59:02

On Thu, Aug 18, 2016 at 11:44:20AM +0200, Christophe Leroy wrote:

SPRN_ICR must be read for clearing the internal freeze signal which
is asserted by the single step exception, otherwise the timebase and
decrementer remain freezed

Minor nit: s/freezed/frozen/

If the timebase and decrementer are frozen even for a few cycles, this
probably upsets timekeeping. I consider this a completely stupid design
decision, and maybe I'm not alone.

    Gabriel

quoted hunk

Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/include/asm/reg_8xx.h | 1 +
 arch/powerpc/kernel/traps.c        | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/arch/powerpc/include/asm/reg_8xx.h b/arch/powerpc/include/asm/reg_8xx.h
index feaf641..6dae71f 100644
--- a/arch/powerpc/include/asm/reg_8xx.h
+++ b/arch/powerpc/include/asm/reg_8xx.h

@@ -17,6 +17,7 @@
 #define SPRN_DC_DAT	570	/* Read-only data register */
 
 /* Misc Debug */
+#define SPRN_ICR	148
 #define SPRN_DPDR	630
 #define SPRN_MI_CAM	816
 #define SPRN_MI_RAM0	817

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 2cb5892..0f1f0ce 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c

@@ -400,8 +400,16 @@ static inline int check_io_access(struct pt_regs *regs)
 #define REASON_TRAP		0x20000
 
 #define single_stepping(regs)	((regs)->msr & MSR_SE)
+#ifdef CONFIG_PPC_8xx
+static inline void clear_single_step(struct pt_regs *regs)
+{
+	regs->msr &= ~MSR_SE;
+	mfspr(SPRN_ICR);
+}
+#else
 #define clear_single_step(regs)	((regs)->msr &= ~MSR_SE)
 #endif
+#endif
 
 #if defined(CONFIG_4xx)
 int machine_check_4xx(struct pt_regs *regs)

-- 
2.1.0

Re: [PATCH] powerpc/8xx: fix single_step debug

From: Gabriel Paubert <hidden>
Date: 2016-08-18 10:17:15

On Thu, Aug 18, 2016 at 12:13:21PM +0200, Christophe Leroy wrote:


Le 18/08/2016 à 11:58, Gabriel Paubert a écrit :

quoted

On Thu, Aug 18, 2016 at 11:44:20AM +0200, Christophe Leroy wrote:

quoted

SPRN_ICR must be read for clearing the internal freeze signal which
is asserted by the single step exception, otherwise the timebase and
decrementer remain freezed

Minor nit: s/freezed/frozen/

If the timebase and decrementer are frozen even for a few cycles, this
probably upsets timekeeping. I consider this a completely stupid design
decision, and maybe I'm not alone.

   Gabriel

We could also unset TBF bit (TimeBase Freeze enable) in TBSCR
register (today it is set in
arch/powerpc/platforms/8xx/m8xx_setup.c) but then it would impact
debug done with an external BDM system which expects the decrementer
and TB frozen when it freezes the execution.

Ok, I believe that systematically setting it is a mistake, but then I'm
always a bit nervous about screwing up timekeeping (it certainly is
always a very bad idea when you are driving telescopes).

    Gabriel

Re: [PATCH] powerpc/8xx: fix single_step debug

From: Christophe Leroy <hidden>
Date: 2016-08-18 10:20:47


Le 18/08/2016 à 11:58, Gabriel Paubert a écrit :

On Thu, Aug 18, 2016 at 11:44:20AM +0200, Christophe Leroy wrote:

quoted

SPRN_ICR must be read for clearing the internal freeze signal which
is asserted by the single step exception, otherwise the timebase and
decrementer remain freezed

Minor nit: s/freezed/frozen/

If the timebase and decrementer are frozen even for a few cycles, this
probably upsets timekeeping. I consider this a completely stupid design
decision, and maybe I'm not alone.

    Gabriel

We could also unset TBF bit (TimeBase Freeze enable) in TBSCR register 
(today it is set in arch/powerpc/platforms/8xx/m8xx_setup.c) but then it 
would impact debug done with an external BDM system which expects the 
decrementer and TB frozen when it freezes the execution.

Christophe

quoted

Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/include/asm/reg_8xx.h | 1 +
 arch/powerpc/kernel/traps.c        | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/arch/powerpc/include/asm/reg_8xx.h b/arch/powerpc/include/asm/reg_8xx.h
index feaf641..6dae71f 100644
--- a/arch/powerpc/include/asm/reg_8xx.h
+++ b/arch/powerpc/include/asm/reg_8xx.h

@@ -17,6 +17,7 @@
 #define SPRN_DC_DAT	570	/* Read-only data register */

 /* Misc Debug */
+#define SPRN_ICR	148
 #define SPRN_DPDR	630
 #define SPRN_MI_CAM	816
 #define SPRN_MI_RAM0	817

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 2cb5892..0f1f0ce 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c

@@ -400,8 +400,16 @@ static inline int check_io_access(struct pt_regs *regs)
 #define REASON_TRAP		0x20000

 #define single_stepping(regs)	((regs)->msr & MSR_SE)
+#ifdef CONFIG_PPC_8xx
+static inline void clear_single_step(struct pt_regs *regs)
+{
+	regs->msr &= ~MSR_SE;
+	mfspr(SPRN_ICR);
+}
+#else
 #define clear_single_step(regs)	((regs)->msr &= ~MSR_SE)
 #endif
+#endif

 #if defined(CONFIG_4xx)
 int machine_check_4xx(struct pt_regs *regs)
--

2.1.0

Re: [PATCH] powerpc/8xx: fix single_step debug

From: Christophe Leroy <hidden>
Date: 2016-08-18 11:50:47


Le 18/08/2016 à 12:16, Gabriel Paubert a écrit :

On Thu, Aug 18, 2016 at 12:13:21PM +0200, Christophe Leroy wrote:

quoted


Le 18/08/2016 à 11:58, Gabriel Paubert a écrit :

quoted

On Thu, Aug 18, 2016 at 11:44:20AM +0200, Christophe Leroy wrote:

quoted

SPRN_ICR must be read for clearing the internal freeze signal which
is asserted by the single step exception, otherwise the timebase and
decrementer remain freezed

Minor nit: s/freezed/frozen/

If the timebase and decrementer are frozen even for a few cycles, this
probably upsets timekeeping. I consider this a completely stupid design
decision, and maybe I'm not alone.

   Gabriel

We could also unset TBF bit (TimeBase Freeze enable) in TBSCR
register (today it is set in
arch/powerpc/platforms/8xx/m8xx_setup.c) but then it would impact
debug done with an external BDM system which expects the decrementer
and TB frozen when it freezes the execution.

Ok, I believe that systematically setting it is a mistake, but then I'm
always a bit nervous about screwing up timekeeping (it certainly is
always a very bad idea when you are driving telescopes).

Indeed you are right, this should not happen. The issue is due to the 
fact that the bootloader set the TRE bit in the DER register.
So the fix is to be done in the boot loader.

Christophe

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help