[v5][PATCH 0/6] powerpc/book3e: powerpc/book3e: make kgdb to work well

[v5][PATCH 0/6] powerpc/book3e: powerpc/book3e: make kgdb to work well

[Tiejun Chen]

Ben,

As you mention just now, I resend this another pending patch sets v5 used
to support kgdb/gdb on book3e.

v5:

* rebase on merge branch.

Note the original patch, [ATCH 5/7] kgdb/kgdbts: support ppc64, is already merged
by Jason.

v4:

* use DEFINE_PER_CPU to allocate kgdb's thread_info
* add patch 7 to make usre copy thread_info only !__check_irq_replay
* leave "andi.   r14,r11,MSR_PR" out of "#ifndef CONFIG_KGDB"
  since cr0 is still used lately.
* retest

v3:

* make work when enable CONFIG_RELOCATABLE
* fix one typo in patch,
  "powerpc/book3e: store critical/machine/debug exception thread info": 
	ld	r1,PACAKSAVE(r13);
    ->  ld	r14,PACAKSAVE(r13);
* remove copying the thread_info since booke and book3e always copy
  the thead_info now when we enter the debug exception, and so drop
  the v2 patch, "book3e/kgdb: Fix a single stgep case of lazy IRQ"

v2:

* Make sure we cover CONFIG_PPC_BOOK3E_64 safely
* Use LOAD_REG_IMMEDIATE() to load properly
	the value of the constant expression in load debug exception stack 
* Copy thread infor form the kernel stack coming from usr
* Rebase latest powerpc git tree

v1:
* Copy thread info only when we are from !user mode since we'll get kernel stack
  coming from usr directly.
* remove save/restore EX_R14/EX_R15 since DBG_EXCEPTION_PROLOG already covered
  this.
* use CURRENT_THREAD_INFO() conveniently to get thread.
* fix some typos
* add a patch to make sure gdb can generate a single step properly to invoke a
  kgdb state.
* add a patch to if we need to replay an interrupt, we shouldn't restore that
  previous backup thread info to make sure we can replay an interrupt lately
  with a proper thread info.
* rebase latest powerpc git tree

v0:
This patchset is used to support kgdb for book3e.

------
Tiejun Chen (6):
      powerpc/book3e: load critical/machine/debug exception stack
      powerpc/book3e: store critical/machine/debug exception thread info
      book3e/kgdb: update thread's dbcr0
      powerpc/book3e: support kgdb for kernel space
      powerpc/kgdb: use DEFINE_PER_CPU to allocate kgdb's thread_info
      book3e/kgdb: Fix a single stgep case of lazy IRQ

 arch/powerpc/kernel/exceptions-64e.S |   68 ++++++++++++++++++++++++++++++++--
 arch/powerpc/kernel/irq.c            |   10 +++++
 arch/powerpc/kernel/kgdb.c           |   21 +++++++----
 3 files changed, 88 insertions(+), 11 deletions(-)

Tiejun

[v5][PATCH 1/6] powerpc/book3e: load critical/machine/debug exception stack

[Tiejun Chen]

We always alloc critical/machine/debug check exceptions. This is
different from the normal exception. So we should load these exception
stack properly like we did for booke.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/kernel/exceptions-64e.S |   49 +++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 4b23119..4d8e57f 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -36,6 +36,37 @@
  */
 #define	SPECIAL_EXC_FRAME_SIZE	INT_FRAME_SIZE
 
+/* only on book3e */
+#define DBG_STACK_BASE		dbgirq_ctx
+#define MC_STACK_BASE		mcheckirq_ctx
+#define CRIT_STACK_BASE		critirq_ctx
+
+#ifdef CONFIG_RELOCATABLE
+#define LOAD_STACK_BASE(reg, level)			\
+	tovirt(r2,r2);					\
+	LOAD_REG_ADDR(reg, level##_STACK_BASE);
+#else
+#define LOAD_STACK_BASE(reg, level)			\
+	LOAD_REG_IMMEDIATE(reg, level##_STACK_BASE);
+#endif
+
+#ifdef CONFIG_SMP
+#define BOOK3E_LOAD_EXC_LEVEL_STACK(level)		\
+	mfspr	r14,SPRN_PIR;				\
+	slwi	r14,r14,3;				\
+	LOAD_STACK_BASE(r10, level);			\
+	add	r10,r10,r14;				\
+	ld	r10,0(r10);				\
+	addi	r10,r10,THREAD_SIZE;			\
+	std	r10,PACA_##level##_STACK(r13);
+#else
+#define BOOK3E_LOAD_EXC_LEVEL_STACK(level)		\
+	LOAD_STACK_BASE(r10, level);			\
+	ld	r10,0(r10);				\
+	addi	r10,r10,THREAD_SIZE;			\
+	std	r10,PACA_##level##_STACK(r13);
+#endif
+
 /* Exception prolog code for all exceptions */
 #define EXCEPTION_PROLOG(n, intnum, type, addition)	    		    \
 	mtspr	SPRN_SPRG_##type##_SCRATCH,r13;	/* get spare registers */   \
@@ -68,20 +99,32 @@
 #define SPRN_GDBELL_SRR1	SPRN_GSRR1
 
 #define CRIT_SET_KSTACK						            \
+	andi.	r10,r11,MSR_PR;							\
+	bne	1f;								\
+	BOOK3E_LOAD_EXC_LEVEL_STACK(CRIT);					\
 	ld	r1,PACA_CRIT_STACK(r13);				    \
-	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;
+	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\
+1:
 #define SPRN_CRIT_SRR0	SPRN_CSRR0
 #define SPRN_CRIT_SRR1	SPRN_CSRR1
 
 #define DBG_SET_KSTACK						            \
+	andi.	r10,r11,MSR_PR;							\
+	bne	1f;								\
+	BOOK3E_LOAD_EXC_LEVEL_STACK(DBG);					\
 	ld	r1,PACA_DBG_STACK(r13);					    \
-	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;
+	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\
+1:
 #define SPRN_DBG_SRR0	SPRN_DSRR0
 #define SPRN_DBG_SRR1	SPRN_DSRR1
 
 #define MC_SET_KSTACK						            \
+	andi.	r10,r11,MSR_PR;							\
+	bne	1f;								\
+	BOOK3E_LOAD_EXC_LEVEL_STACK(MC);					\
 	ld	r1,PACA_MC_STACK(r13);					    \
-	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;
+	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\
+1:
 #define SPRN_MC_SRR0	SPRN_MCSRR0
 #define SPRN_MC_SRR1	SPRN_MCSRR1
 
-- 
1.7.9.5

[v5][PATCH 2/6] powerpc/book3e: store critical/machine/debug exception thread info

[Tiejun Chen]

We need to store thread info to these exception thread info like something
we already did for PPC32.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/kernel/exceptions-64e.S |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 4d8e57f..07cf657 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -67,6 +67,18 @@
 	std	r10,PACA_##level##_STACK(r13);
 #endif
 
+/* Store something to exception thread info */
+#define	BOOK3E_STORE_EXC_LEVEL_THEAD_INFO(type)					\
+	ld	r14,PACAKSAVE(r13);						\
+	CURRENT_THREAD_INFO(r14, r14);						\
+	CURRENT_THREAD_INFO(r15, r1);						\
+	ld	r10,TI_FLAGS(r14);		     				\
+	std	r10,TI_FLAGS(r15);			     			\
+	ld	r10,TI_PREEMPT(r14);		     				\
+	std	r10,TI_PREEMPT(r15);		     				\
+	ld	r10,TI_TASK(r14);			     			\
+	std	r10,TI_TASK(r15);
+
 /* Exception prolog code for all exceptions */
 #define EXCEPTION_PROLOG(n, intnum, type, addition)	    		    \
 	mtspr	SPRN_SPRG_##type##_SCRATCH,r13;	/* get spare registers */   \
@@ -104,6 +116,7 @@
 	BOOK3E_LOAD_EXC_LEVEL_STACK(CRIT);					\
 	ld	r1,PACA_CRIT_STACK(r13);				    \
 	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\
+	BOOK3E_STORE_EXC_LEVEL_THEAD_INFO(CRIT);				\
 1:
 #define SPRN_CRIT_SRR0	SPRN_CSRR0
 #define SPRN_CRIT_SRR1	SPRN_CSRR1
@@ -114,6 +127,7 @@
 	BOOK3E_LOAD_EXC_LEVEL_STACK(DBG);					\
 	ld	r1,PACA_DBG_STACK(r13);					    \
 	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\
+	BOOK3E_STORE_EXC_LEVEL_THEAD_INFO(DBG);					\
 1:
 #define SPRN_DBG_SRR0	SPRN_DSRR0
 #define SPRN_DBG_SRR1	SPRN_DSRR1
@@ -124,6 +138,7 @@
 	BOOK3E_LOAD_EXC_LEVEL_STACK(MC);					\
 	ld	r1,PACA_MC_STACK(r13);					    \
 	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\
+	BOOK3E_STORE_EXC_LEVEL_THEAD_INFO(MC);					\
 1:
 #define SPRN_MC_SRR0	SPRN_MCSRR0
 #define SPRN_MC_SRR1	SPRN_MCSRR1
-- 
1.7.9.5

[v5][PATCH 3/6] book3e/kgdb: update thread's dbcr0

[Tiejun Chen]

gdb always need to generate a single step properly to invoke
a kgdb state. But with lazy interrupt, book3e can't always
trigger a debug exception with a single step since the current
is blocked for handling those pending exception, then we miss
that expected dbcr configuration at last to generate a debug
exception.

So here we also update thread's dbcr0 to make sure the current
can go back with that missed dbcr0 configuration.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/kernel/kgdb.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
index c1eef24..55409ac 100644
--- a/arch/powerpc/kernel/kgdb.c
+++ b/arch/powerpc/kernel/kgdb.c
@@ -410,7 +410,7 @@ int kgdb_arch_handle_exception(int vector, int signo, int err_code,
 			       struct pt_regs *linux_regs)
 {
 	char *ptr = &remcom_in_buffer[1];
-	unsigned long addr;
+	unsigned long addr, dbcr0;
 
 	switch (remcom_in_buffer[0]) {
 		/*
@@ -427,8 +427,15 @@ int kgdb_arch_handle_exception(int vector, int signo, int err_code,
 		/* set the trace bit if we're stepping */
 		if (remcom_in_buffer[0] == 's') {
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
-			mtspr(SPRN_DBCR0,
-			      mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);
+			dbcr0 = mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM;
+			mtspr(SPRN_DBCR0, dbcr0);
+#ifdef CONFIG_PPC_BOOK3E_64
+			/* With lazy interrut we have to update thread dbcr0 here
+			 * to make sure we can set debug properly at last to invoke
+			 * kgdb again to work well.
+			 */
+			current->thread.dbcr0 = dbcr0;
+#endif
 			linux_regs->msr |= MSR_DE;
 #else
 			linux_regs->msr |= MSR_SE;
-- 
1.7.9.5

[v5][PATCH 4/6] powerpc/book3e: support kgdb for kernel space

[Tiejun Chen]

Currently we need to skip this for supporting KGDB.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/kernel/exceptions-64e.S |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 07cf657..a286b51 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -639,11 +639,13 @@ kernel_dbg_exc:
 	rfdi
 
 	/* Normal debug exception */
+1:	andi.	r14,r11,MSR_PR;		/* check for userspace again */
+#ifndef CONFIG_KGDB
 	/* XXX We only handle coming from userspace for now since we can't
 	 *     quite save properly an interrupted kernel state yet
 	 */
-1:	andi.	r14,r11,MSR_PR;		/* check for userspace again */
 	beq	kernel_dbg_exc;		/* if from kernel mode */
+#endif
 
 	/* Now we mash up things to make it look like we are coming on a
 	 * normal exception
-- 
1.7.9.5

[v5][PATCH 5/6] powerpc/kgdb: use DEFINE_PER_CPU to allocate kgdb's thread_info

[Tiejun Chen]

Use DEFINE_PER_CPU to allocate thread_info statically instead of kmalloc().
This can avoid introducing more memory check codes.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/kernel/kgdb.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
index 55409ac..cde7818 100644
--- a/arch/powerpc/kernel/kgdb.c
+++ b/arch/powerpc/kernel/kgdb.c
@@ -151,15 +151,15 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
 	return 1;
 }
 
+static DEFINE_PER_CPU(struct thread_info, kgdb_thread_info);
 static int kgdb_singlestep(struct pt_regs *regs)
 {
 	struct thread_info *thread_info, *exception_thread_info;
-	struct thread_info *backup_current_thread_info;
+	struct thread_info *backup_current_thread_info = &__get_cpu_var(kgdb_thread_info);
 
 	if (user_mode(regs))
 		return 0;
 
-	backup_current_thread_info = kmalloc(sizeof(struct thread_info), GFP_KERNEL);
 	/*
 	 * On Book E and perhaps other processors, singlestep is handled on
 	 * the critical exception stack.  This causes current_thread_info()
@@ -185,7 +185,6 @@ static int kgdb_singlestep(struct pt_regs *regs)
 		/* Restore current_thread_info lastly. */
 		memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
 
-	kfree(backup_current_thread_info);
 	return 1;
 }
 
-- 
1.7.9.5

[v5][PATCH 6/6] book3e/kgdb: Fix a single stgep case of lazy IRQ

[Tiejun Chen]

When we're in kgdb_singlestep(), we have to work around to get
thread_info by copying from the kernel stack before calling
kgdb_handle_exception(), then copying it back afterwards.

But for PPC64, we have a lazy interrupt implementation. So after
copying thread info frome kernle stack, if we need to replay an
interrupt, we shouldn't restore that previous backup thread info
to make sure we can replay an interrupt lately with a proper
thread info.

This patch use __check_irq_replay() to guarantee this process.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/kernel/irq.c  |   10 ++++++++++
 arch/powerpc/kernel/kgdb.c |    3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index ea185e0..3625453 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -339,7 +339,17 @@ bool prep_irq_for_idle(void)
 	return true;
 }
 
+notrace unsigned int check_irq_replay(void)
+{
+	return __check_irq_replay();
+}
+#else
+notrace unsigned int check_irq_replay(void)
+{
+	return 0;
+}
 #endif /* CONFIG_PPC64 */
+EXPORT_SYMBOL(check_irq_replay);
 
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
index cde7818..5b30408 100644
--- a/arch/powerpc/kernel/kgdb.c
+++ b/arch/powerpc/kernel/kgdb.c
@@ -152,6 +152,7 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
 }
 
 static DEFINE_PER_CPU(struct thread_info, kgdb_thread_info);
+extern notrace unsigned int check_irq_replay(void);
 static int kgdb_singlestep(struct pt_regs *regs)
 {
 	struct thread_info *thread_info, *exception_thread_info;
@@ -181,7 +182,7 @@ static int kgdb_singlestep(struct pt_regs *regs)
 
 	kgdb_handle_exception(0, SIGTRAP, 0, regs);
 
-	if (thread_info != exception_thread_info)
+	if ((thread_info != exception_thread_info) && (!check_irq_replay()))
 		/* Restore current_thread_info lastly. */
 		memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
 
-- 
1.7.9.5

Re: [v5][PATCH 1/6] powerpc/book3e: load critical/machine/debug exception stack

[Scott Wood]

On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
> We always alloc critical/machine/debug check exceptions. This is
> different from the normal exception. So we should load these exception
> stack properly like we did for booke.

This is "booke".  Do you mean like "like we did for 32-bit"?

And the code is already trying to load the special stack; it just
happens that it's loading from a different location than the C code
placed the stack addresses.  The changelog should point out the specific
thing that is being fixed.

> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  arch/powerpc/kernel/exceptions-64e.S |   49 +++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index 4b23119..4d8e57f 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -36,6 +36,37 @@
>   */
>  #define	SPECIAL_EXC_FRAME_SIZE	INT_FRAME_SIZE
>  
> +/* only on book3e */
> +#define DBG_STACK_BASE		dbgirq_ctx
> +#define MC_STACK_BASE		mcheckirq_ctx
> +#define CRIT_STACK_BASE		critirq_ctx
> +
> +#ifdef CONFIG_RELOCATABLE
> +#define LOAD_STACK_BASE(reg, level)			\
> +	tovirt(r2,r2);					\
> +	LOAD_REG_ADDR(reg, level##_STACK_BASE);
> +#else
> +#define LOAD_STACK_BASE(reg, level)			\
> +	LOAD_REG_IMMEDIATE(reg, level##_STACK_BASE);
> +#endif
> +
> +#ifdef CONFIG_SMP
> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level)		\
> +	mfspr	r14,SPRN_PIR;				\
> +	slwi	r14,r14,3;				\
> +	LOAD_STACK_BASE(r10, level);			\
> +	add	r10,r10,r14;				\
> +	ld	r10,0(r10);				\
> +	addi	r10,r10,THREAD_SIZE;			\
> +	std	r10,PACA_##level##_STACK(r13);
> +#else
> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level)		\
> +	LOAD_STACK_BASE(r10, level);			\
> +	ld	r10,0(r10);				\
> +	addi	r10,r10,THREAD_SIZE;			\
> +	std	r10,PACA_##level##_STACK(r13);
> +#endif

It looks like you're loading the stack from *irq_ctx, storing it in
PACA_*_stack, and then (immediately after this in the caller) loading it
back from PACA_*_STACK.  Why not just load it from *irq_ctx and get rid
of PACA_*_STACK altogether -- or change the C code to initialize the
addresses in the PACA instead, and get ird of *irq_ctx on 64-bit?

>  /* Exception prolog code for all exceptions */
>  #define EXCEPTION_PROLOG(n, intnum, type, addition)	    		    \
>  	mtspr	SPRN_SPRG_##type##_SCRATCH,r13;	/* get spare registers */   \
> @@ -68,20 +99,32 @@
>  #define SPRN_GDBELL_SRR1	SPRN_GSRR1
>  
>  #define CRIT_SET_KSTACK						            \
> +	andi.	r10,r11,MSR_PR;							\
> +	bne	1f;								\
> +	BOOK3E_LOAD_EXC_LEVEL_STACK(CRIT);					\
>  	ld	r1,PACA_CRIT_STACK(r13);				    \
> -	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;
> +	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\

The caller will already check MSR_PR and override this if coming from
userspace; why do you need to check again here?

-Scott

Re: [v5][PATCH 2/6] powerpc/book3e: store critical/machine/debug exception thread info

[Scott Wood]

On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
> We need to store thread info to these exception thread info like something
> we already did for PPC32.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  arch/powerpc/kernel/exceptions-64e.S |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index 4d8e57f..07cf657 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -67,6 +67,18 @@
>  	std	r10,PACA_##level##_STACK(r13);
>  #endif
>  
> +/* Store something to exception thread info */
> +#define	BOOK3E_STORE_EXC_LEVEL_THEAD_INFO(type)					\
> +	ld	r14,PACAKSAVE(r13);						\
> +	CURRENT_THREAD_INFO(r14, r14);						\
> +	CURRENT_THREAD_INFO(r15, r1);						\
> +	ld	r10,TI_FLAGS(r14);		     				\
> +	std	r10,TI_FLAGS(r15);			     			\
> +	ld	r10,TI_PREEMPT(r14);		     				\
> +	std	r10,TI_PREEMPT(r15);		     				\
> +	ld	r10,TI_TASK(r14);			     			\
> +	std	r10,TI_TASK(r15);

Where is "type" used?

BTW, no need for a BOOK3E prefix for things local to this file.

-Scott

Re: [v5][PATCH 3/6] book3e/kgdb: update thread's dbcr0

[Scott Wood]

On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
> gdb always need to generate a single step properly to invoke
> a kgdb state. But with lazy interrupt, book3e can't always
> trigger a debug exception with a single step since the current
> is blocked for handling those pending exception, then we miss
> that expected dbcr configuration at last to generate a debug
> exception.

What do you mean by "the current is blocked"?  Could you explain more
clearly what lazy EE has to do with MSR_DE and DBCR0?

> So here we also update thread's dbcr0 to make sure the current
> can go back with that missed dbcr0 configuration.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  arch/powerpc/kernel/kgdb.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
> index c1eef24..55409ac 100644
> --- a/arch/powerpc/kernel/kgdb.c
> +++ b/arch/powerpc/kernel/kgdb.c
> @@ -410,7 +410,7 @@ int kgdb_arch_handle_exception(int vector, int signo, int err_code,
>  			       struct pt_regs *linux_regs)
>  {
>  	char *ptr = &remcom_in_buffer[1];
> -	unsigned long addr;
> +	unsigned long addr, dbcr0;
>  
>  	switch (remcom_in_buffer[0]) {
>  		/*
> @@ -427,8 +427,15 @@ int kgdb_arch_handle_exception(int vector, int signo, int err_code,
>  		/* set the trace bit if we're stepping */
>  		if (remcom_in_buffer[0] == 's') {
>  #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> -			mtspr(SPRN_DBCR0,
> -			      mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);
> +			dbcr0 = mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM;
> +			mtspr(SPRN_DBCR0, dbcr0);
> +#ifdef CONFIG_PPC_BOOK3E_64

This could as well be "CONFIG_PPC64" -- CONFIG_PPC_ADV_DEBUG_REGS
implies booke or 40x.  Lazy EE is a CONFIG_PPC64 thing, not specifically
CONFIG_PPC_BOOK3E_64.

> +			/* With lazy interrut we have to update thread dbcr0 here

s/interrut/interrupts/

> +			 * to make sure we can set debug properly at last to invoke
> +			 * kgdb again to work well.
> +			 */
> +			current->thread.dbcr0 = dbcr0;
> +#endif
>  			linux_regs->msr |= MSR_DE;
>  #else
>  			linux_regs->msr |= MSR_SE;

Hmm, what happens here if we enable KGDB on booke without
CONFIG_PPC_ADV_DEBUG_REGS?  Kconfig doesn't appear to prevent it.

-Scott

Re: [v5][PATCH 4/6] powerpc/book3e: support kgdb for kernel space

[Scott Wood]

On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
> Currently we need to skip this for supporting KGDB.

Does it need to depend on CONFIG_KGDB?  Either you've fixed the "can't
quite save properly" part, or you haven't.

-Scott

> 
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  arch/powerpc/kernel/exceptions-64e.S |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index 07cf657..a286b51 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -639,11 +639,13 @@ kernel_dbg_exc:
>  	rfdi
>  
>  	/* Normal debug exception */
> +1:	andi.	r14,r11,MSR_PR;		/* check for userspace again */
> +#ifndef CONFIG_KGDB
>  	/* XXX We only handle coming from userspace for now since we can't
>  	 *     quite save properly an interrupted kernel state yet
>  	 */
> -1:	andi.	r14,r11,MSR_PR;		/* check for userspace again */
>  	beq	kernel_dbg_exc;		/* if from kernel mode */
> +#endif
>  
>  	/* Now we mash up things to make it look like we are coming on a
>  	 * normal exception

Re: [v5][PATCH 6/6] book3e/kgdb: Fix a single stgep case of lazy IRQ

[Scott Wood]

On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
> When we're in kgdb_singlestep(), we have to work around to get
> thread_info by copying from the kernel stack before calling
> kgdb_handle_exception(), then copying it back afterwards.
> 
> But for PPC64, we have a lazy interrupt implementation. So after
> copying thread info frome kernle stack, if we need to replay an
> interrupt, we shouldn't restore that previous backup thread info
> to make sure we can replay an interrupt lately with a proper
> thread info.

Explain why copying it would be a problem.

> This patch use __check_irq_replay() to guarantee this process.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  arch/powerpc/kernel/irq.c  |   10 ++++++++++
>  arch/powerpc/kernel/kgdb.c |    3 ++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index ea185e0..3625453 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -339,7 +339,17 @@ bool prep_irq_for_idle(void)
>  	return true;
>  }
>  
> +notrace unsigned int check_irq_replay(void)
> +{
> +	return __check_irq_replay();
> +}
> +#else
> +notrace unsigned int check_irq_replay(void)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_PPC64 */
> +EXPORT_SYMBOL(check_irq_replay);
>  
>  int arch_show_interrupts(struct seq_file *p, int prec)
>  {
> diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
> index cde7818..5b30408 100644
> --- a/arch/powerpc/kernel/kgdb.c
> +++ b/arch/powerpc/kernel/kgdb.c
> @@ -152,6 +152,7 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
>  }
>  
>  static DEFINE_PER_CPU(struct thread_info, kgdb_thread_info);
> +extern notrace unsigned int check_irq_replay(void);

Please put prototypes in headers rather than C files.  Also, "extern" is
unnecessary on function prototypes.

>  static int kgdb_singlestep(struct pt_regs *regs)
>  {
>  	struct thread_info *thread_info, *exception_thread_info;
> @@ -181,7 +182,7 @@ static int kgdb_singlestep(struct pt_regs *regs)
>  
>  	kgdb_handle_exception(0, SIGTRAP, 0, regs);
>  
> -	if (thread_info != exception_thread_info)
> +	if ((thread_info != exception_thread_info) && (!check_irq_replay()))

Unnecessary parentheses.

Are you sure it's safe to call this here?  Won't __check_irq_replay()
clear the pending event and PACA_IRQ_HARD_DIS?

-Scott

Re: [v5][PATCH 1/6] powerpc/book3e: load critical/machine/debug exception stack

[Scott Wood]

On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
> We always alloc critical/machine/debug check exceptions. This is
> different from the normal exception. So we should load these exception
> stack properly like we did for booke.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  arch/powerpc/kernel/exceptions-64e.S |   49 +++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index 4b23119..4d8e57f 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -36,6 +36,37 @@
>   */
>  #define	SPECIAL_EXC_FRAME_SIZE	INT_FRAME_SIZE
>  
> +/* only on book3e */
> +#define DBG_STACK_BASE		dbgirq_ctx
> +#define MC_STACK_BASE		mcheckirq_ctx
> +#define CRIT_STACK_BASE		critirq_ctx
> +
> +#ifdef CONFIG_RELOCATABLE
> +#define LOAD_STACK_BASE(reg, level)			\
> +	tovirt(r2,r2);					\
> +	LOAD_REG_ADDR(reg, level##_STACK_BASE);

Where does r2 come from here, where does it get used, and why do we need
tovirt() on book3e?

-Scott

Re: [v5][PATCH 1/6] powerpc/book3e: load critical/machine/debug exception stack

["“tiejun.chen”"]

On 10/19/2013 06:37 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> We always alloc critical/machine/debug check exceptions. This is
>> different from the normal exception. So we should load these exception
>> stack properly like we did for booke.
>
> This is "booke".  Do you mean like "like we did for 32-bit"?

Yes.

>
> And the code is already trying to load the special stack; it just
> happens that it's loading from a different location than the C code
> placed the stack addresses.  The changelog should point out the specific
> thing that is being fixed.

Here I don't fix anything, and I just want to do the same thing as 32-bit.

>
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>>   arch/powerpc/kernel/exceptions-64e.S |   49 +++++++++++++++++++++++++++++++---
>>   1 file changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
>> index 4b23119..4d8e57f 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -36,6 +36,37 @@
>>    */
>>   #define	SPECIAL_EXC_FRAME_SIZE	INT_FRAME_SIZE
>>
>> +/* only on book3e */
>> +#define DBG_STACK_BASE		dbgirq_ctx
>> +#define MC_STACK_BASE		mcheckirq_ctx
>> +#define CRIT_STACK_BASE		critirq_ctx
>> +
>> +#ifdef CONFIG_RELOCATABLE
>> +#define LOAD_STACK_BASE(reg, level)			\
>> +	tovirt(r2,r2);					\
>> +	LOAD_REG_ADDR(reg, level##_STACK_BASE);
>> +#else
>> +#define LOAD_STACK_BASE(reg, level)			\
>> +	LOAD_REG_IMMEDIATE(reg, level##_STACK_BASE);
>> +#endif
>> +
>> +#ifdef CONFIG_SMP
>> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level)		\
>> +	mfspr	r14,SPRN_PIR;				\
>> +	slwi	r14,r14,3;				\
>> +	LOAD_STACK_BASE(r10, level);			\
>> +	add	r10,r10,r14;				\
>> +	ld	r10,0(r10);				\
>> +	addi	r10,r10,THREAD_SIZE;			\
>> +	std	r10,PACA_##level##_STACK(r13);
>> +#else
>> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level)		\
>> +	LOAD_STACK_BASE(r10, level);			\
>> +	ld	r10,0(r10);				\
>> +	addi	r10,r10,THREAD_SIZE;			\
>> +	std	r10,PACA_##level##_STACK(r13);
>> +#endif
>
> It looks like you're loading the stack from *irq_ctx, storing it in
> PACA_*_stack, and then (immediately after this in the caller) loading it
> back from PACA_*_STACK.  Why not just load it from *irq_ctx and get rid
> of PACA_*_STACK altogether -- or change the C code to initialize the
> addresses in the PACA instead, and get ird of *irq_ctx on 64-bit?

Okay, I'd like to move forward the c code, please see next version.

>
>>   /* Exception prolog code for all exceptions */
>>   #define EXCEPTION_PROLOG(n, intnum, type, addition)	    		    \
>>   	mtspr	SPRN_SPRG_##type##_SCRATCH,r13;	/* get spare registers */   \
>> @@ -68,20 +99,32 @@
>>   #define SPRN_GDBELL_SRR1	SPRN_GSRR1
>>
>>   #define CRIT_SET_KSTACK						            \
>> +	andi.	r10,r11,MSR_PR;							\
>> +	bne	1f;								\
>> +	BOOK3E_LOAD_EXC_LEVEL_STACK(CRIT);					\
>>   	ld	r1,PACA_CRIT_STACK(r13);				    \
>> -	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;
>> +	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\
>
> The caller will already check MSR_PR and override this if coming from
> userspace; why do you need to check again here?

Looks this is redundant so this will be left out.

Thanks,

Tiejun

Re: [v5][PATCH 2/6] powerpc/book3e: store critical/machine/debug exception thread info

["“tiejun.chen”"]

On 10/19/2013 06:43 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> We need to store thread info to these exception thread info like something
>> we already did for PPC32.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>>   arch/powerpc/kernel/exceptions-64e.S |   15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
>> index 4d8e57f..07cf657 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -67,6 +67,18 @@
>>   	std	r10,PACA_##level##_STACK(r13);
>>   #endif
>>
>> +/* Store something to exception thread info */
>> +#define	BOOK3E_STORE_EXC_LEVEL_THEAD_INFO(type)					\
>> +	ld	r14,PACAKSAVE(r13);						\
>> +	CURRENT_THREAD_INFO(r14, r14);						\
>> +	CURRENT_THREAD_INFO(r15, r1);						\
>> +	ld	r10,TI_FLAGS(r14);		     				\
>> +	std	r10,TI_FLAGS(r15);			     			\
>> +	ld	r10,TI_PREEMPT(r14);		     				\
>> +	std	r10,TI_PREEMPT(r15);		     				\
>> +	ld	r10,TI_TASK(r14);			     			\
>> +	std	r10,TI_TASK(r15);
>
> Where is "type" used?
>

Yes, its noting now but its worth leaving this to extend something in the future.

> BTW, no need for a BOOK3E prefix for things local to this file.
>

What about "EXC_LEVEL_EXCEPTION_PROLOG"? Please see next version.

Thanks,

Tiejun

Re: [v5][PATCH 3/6] book3e/kgdb: update thread's dbcr0

["“tiejun.chen”"]

On 10/19/2013 06:57 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> gdb always need to generate a single step properly to invoke
>> a kgdb state. But with lazy interrupt, book3e can't always
>> trigger a debug exception with a single step since the current
>> is blocked for handling those pending exception, then we miss
>> that expected dbcr configuration at last to generate a debug
>> exception.
>
> What do you mean by "the current is blocked"?  Could you explain more
> clearly what lazy EE has to do with MSR_DE and DBCR0?
>

I will go another path to make sure the lazy EE doesn't affect KGDB, so please 
see next version.

Thanks,

Tiejun

Re: [v5][PATCH 4/6] powerpc/book3e: support kgdb for kernel space

["“tiejun.chen”"]

On 10/19/2013 06:58 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> Currently we need to skip this for supporting KGDB.
>
> Does it need to depend on CONFIG_KGDB?  Either you've fixed the "can't
> quite save properly" part, or you haven't.

I'm not 100% sure if my change is fine to other scenarios so I have to use this 
guarantee other stuff are still safe. But if you think we can remove this 
dependent, I'm happy to do :)

Thanks,

Tiejun

Re: [v5][PATCH 6/6] book3e/kgdb: Fix a single stgep case of lazy IRQ

["“tiejun.chen”"]

On 10/19/2013 07:32 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> When we're in kgdb_singlestep(), we have to work around to get
>> thread_info by copying from the kernel stack before calling
>> kgdb_handle_exception(), then copying it back afterwards.
>>
>> But for PPC64, we have a lazy interrupt implementation. So after
>> copying thread info frome kernle stack, if we need to replay an
>> interrupt, we shouldn't restore that previous backup thread info
>> to make sure we can replay an interrupt lately with a proper
>> thread info.
>
> Explain why copying it would be a problem.
>

This would be gone away in next version as well :)

Thanks,

Tiejun

Re: [v5][PATCH 1/6] powerpc/book3e: load critical/machine/debug exception stack

["“tiejun.chen”"]

On 10/19/2013 07:55 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> We always alloc critical/machine/debug check exceptions. This is
>> different from the normal exception. So we should load these exception
>> stack properly like we did for booke.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>>   arch/powerpc/kernel/exceptions-64e.S |   49 +++++++++++++++++++++++++++++++---
>>   1 file changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
>> index 4b23119..4d8e57f 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -36,6 +36,37 @@
>>    */
>>   #define	SPECIAL_EXC_FRAME_SIZE	INT_FRAME_SIZE
>>
>> +/* only on book3e */
>> +#define DBG_STACK_BASE		dbgirq_ctx
>> +#define MC_STACK_BASE		mcheckirq_ctx
>> +#define CRIT_STACK_BASE		critirq_ctx
>> +
>> +#ifdef CONFIG_RELOCATABLE
>> +#define LOAD_STACK_BASE(reg, level)			\
>> +	tovirt(r2,r2);					\
>> +	LOAD_REG_ADDR(reg, level##_STACK_BASE);
>
> Where does r2 come from here, where does it get used, and why do we need
> tovirt() on book3e?
>

As I remember this should be covered when we boot that capture kernel in 
kexec/kdump case.

Now this is also gone away after move forward the c code.

Thanks,

Tiejun