Patchwork [02/13] powerpc/book3e: Hack to get gdb moving along on Book3E 64-bit

login
register
mail settings
Submitter Benjamin Herrenschmidt
Date July 9, 2010, 6:16 a.m.
Message ID <1278656215-24705-2-git-send-email-benh@kernel.crashing.org>
Download mbox | patch
Permalink /patch/58342/
State Accepted, archived
Commit a2e198116f97bb1cd5b37ff33a8cfdfb4010cf5b
Headers show

Comments

Benjamin Herrenschmidt - July 9, 2010, 6:16 a.m.
Our handling of debug interrupts on Book3E 64-bit is not quite
the way it should be just yet. This is a workaround to let gdb
work at least for now. We ensure that when context switching,
we set the appropriate DBCR0 value for the new task. We also
make sure that we turn off MSR[DE] within the kernel, and set
it as part of the bits that get set when going back to userspace.

In the long run, we will probably set the userspace DBCR0 on the
exception exit code path and ensure we have some proper kernel
value to set on the way into the kernel, a bit like ppc32 does,
but that will take more work.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/include/asm/reg_booke.h |    4 ++--
 arch/powerpc/kernel/process.c        |   22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
K.Prasad - July 9, 2010, 8:52 a.m.
On Fri, Jul 09, 2010 at 04:16:44PM +1000, Benjamin Herrenschmidt wrote:

Hi,
   A few questions and some trivial comments below.

> Our handling of debug interrupts on Book3E 64-bit is not quite
> the way it should be just yet. This is a workaround to let gdb
> work at least for now. We ensure that when context switching,
> we set the appropriate DBCR0 value for the new task. We also
> make sure that we turn off MSR[DE] within the kernel, and set
> it as part of the bits that get set when going back to userspace.
> 

I think I'm missing the code where MSR_DE is set before returning
to user-space? I just found one instance where MSR_USER64 (which now
includes MSR_DE) is used (in start_thread()). If not set, we'll lose the
interrupts caused in IDM too.

> In the long run, we will probably set the userspace DBCR0 on the
> exception exit code path and ensure we have some proper kernel
> value to set on the way into the kernel, a bit like ppc32 does,
> but that will take more work.

The effort to port ppc32 BookIII E debug register usage to use generic
hw-breakpoint interfaces (linuxppc-dev message-id:
20100629165152.GA8586@in.ibm.com), in its final form, should cleanup
most of this code. Even the hook switch_booke_debug_regs() in
__switch_to() should be done away.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  arch/powerpc/include/asm/reg_booke.h |    4 ++--
>  arch/powerpc/kernel/process.c        |   22 ++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/reg_booke.h b/arch/powerpc/include/asm/reg_booke.h
> index 2360317..66dc6f0 100644
> --- a/arch/powerpc/include/asm/reg_booke.h
> +++ b/arch/powerpc/include/asm/reg_booke.h
> @@ -29,8 +29,8 @@
>  #if defined(CONFIG_PPC_BOOK3E_64)
>  #define MSR_		MSR_ME | MSR_CE
>  #define MSR_KERNEL      MSR_ | MSR_CM
> -#define MSR_USER32	MSR_ | MSR_PR | MSR_EE
> -#define MSR_USER64	MSR_USER32 | MSR_CM
> +#define MSR_USER32	MSR_ | MSR_PR | MSR_EE | MSR_DE
> +#define MSR_USER64	MSR_USER32 | MSR_CM | MSR_DE

MSR_DE is included twice in MSR_USER64 (once through MSR_USER32).

>  #elif defined (CONFIG_40x)
>  #define MSR_KERNEL	(MSR_ME|MSR_RI|MSR_IR|MSR_DR|MSR_CE)
>  #define MSR_USER	(MSR_KERNEL|MSR_PR|MSR_EE)
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 1e78453..551f671 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -477,6 +477,28 @@ struct task_struct *__switch_to(struct task_struct *prev,
>  	new_thread = &new->thread;
>  	old_thread = &current->thread;
> 
> +#if defined(CONFIG_PPC_BOOK3E_64)
> +	/* XXX Current Book3E code doesn't deal with kernel side DBCR0,

You may want to use the style
	/*
	 * ......

> +	 * we always hold the user values, so we set it now.
> +	 *
> +	 * However, we ensure the kernel MSR:DE is appropriately cleared too
> +	 * to avoid spurrious single step exceptions in the kernel.
                 ^^^spurious^^^

> +	 *
> +	 * This will have to change to merge with the ppc32 code at some point,
> +	 * but I don't like much what ppc32 is doing today so there's some
> +	 * thinking needed there
> +	 */
> +	if ((new_thread->dbcr0 | old_thread->dbcr0) & DBCR0_IDM) {
> +		u32 dbcr0;

thread->dbcr0 is defined as "unsigned long" in processor.h however "u32 dbcr0"
here must be fine (given that DBCR0 uses 32-bits in ppc32 and uses only 32:63
bits in BOOKIIIE_64). Should dbcr<0-n> be made u32, given that there
will be no 64-bit long value to store (or am I missing something)?

Thanks,
K.Prasad


> +
> +		mtmsr(mfmsr() & ~MSR_DE);
> +		isync();
> +		dbcr0 = mfspr(SPRN_DBCR0);
> +		dbcr0 = (dbcr0 & DBCR0_EDM) | new_thread->dbcr0;
> +		mtspr(SPRN_DBCR0, dbcr0);
> +	}
> +#endif /* CONFIG_PPC64_BOOK3E */
> +
>  #ifdef CONFIG_PPC64
>  	/*
>  	 * Collect processor utilization data per process
> -- 
> 1.6.3.3
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Patch

diff --git a/arch/powerpc/include/asm/reg_booke.h b/arch/powerpc/include/asm/reg_booke.h
index 2360317..66dc6f0 100644
--- a/arch/powerpc/include/asm/reg_booke.h
+++ b/arch/powerpc/include/asm/reg_booke.h
@@ -29,8 +29,8 @@ 
 #if defined(CONFIG_PPC_BOOK3E_64)
 #define MSR_		MSR_ME | MSR_CE
 #define MSR_KERNEL      MSR_ | MSR_CM
-#define MSR_USER32	MSR_ | MSR_PR | MSR_EE
-#define MSR_USER64	MSR_USER32 | MSR_CM
+#define MSR_USER32	MSR_ | MSR_PR | MSR_EE | MSR_DE
+#define MSR_USER64	MSR_USER32 | MSR_CM | MSR_DE
 #elif defined (CONFIG_40x)
 #define MSR_KERNEL	(MSR_ME|MSR_RI|MSR_IR|MSR_DR|MSR_CE)
 #define MSR_USER	(MSR_KERNEL|MSR_PR|MSR_EE)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 1e78453..551f671 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -477,6 +477,28 @@  struct task_struct *__switch_to(struct task_struct *prev,
 	new_thread = &new->thread;
 	old_thread = &current->thread;
 
+#if defined(CONFIG_PPC_BOOK3E_64)
+	/* XXX Current Book3E code doesn't deal with kernel side DBCR0,
+	 * we always hold the user values, so we set it now.
+	 *
+	 * However, we ensure the kernel MSR:DE is appropriately cleared too
+	 * to avoid spurrious single step exceptions in the kernel.
+	 *
+	 * This will have to change to merge with the ppc32 code at some point,
+	 * but I don't like much what ppc32 is doing today so there's some
+	 * thinking needed there
+	 */
+	if ((new_thread->dbcr0 | old_thread->dbcr0) & DBCR0_IDM) {
+		u32 dbcr0;
+
+		mtmsr(mfmsr() & ~MSR_DE);
+		isync();
+		dbcr0 = mfspr(SPRN_DBCR0);
+		dbcr0 = (dbcr0 & DBCR0_EDM) | new_thread->dbcr0;
+		mtspr(SPRN_DBCR0, dbcr0);
+	}
+#endif /* CONFIG_PPC64_BOOK3E */
+
 #ifdef CONFIG_PPC64
 	/*
 	 * Collect processor utilization data per process