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
We need to store thread info to these exception thread info like something
we already did for PPC32.
Signed-off-by: Tiejun Chen <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Currently we need to skip this for supporting KGDB.
Signed-off-by: Tiejun Chen <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> ---
>> 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
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
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
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
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 <[email protected]>
>> ---
>> 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