Hi Andrei,
Thanks for these patches. Your point about the need for a general cleanup of
current thread info pointer fetch is exellent. We definitely need to add some
ifdefs around stack size.
I would also like to add more code to kernel entry points to save thread info
pointer. It would be too intrusive though, since with such a code in place,
acceptance into mainline kernel becomes difficult. We could add it to a
non-lite patch.
Suggestions?
-Amit
On Wednesday 05 July 2006 21:58, Andrei Konovalov wrote:
> Hello,
>
> Here:
>
> + /*
> + * On Book E and perhaps other processsors, singlestep is handled on
> + * the critical exception stack. This causes current_thread_info()
> + * to fail, since it it locates the thread_info by masking off
> + * the low bits of the current stack pointer. We work around
> + * this issue by copying the thread_info from the kernel stack
> + * before calling kgdb_handle_exception, and copying it back
> + * afterwards. On most processors the copy is avoided since
> + * exception_thread_info == thread_info.
> + */
> + thread_info = (struct thread_info *)(regs->gpr[1] & ~(THREAD_SIZE-1));
> + exception_thread_info = current_thread_info();
>
> - calling current_thread_info() is incorrect, as it assumes 8K stack, but
> for Book E and ppc4xx the critical exception stack is 4K and is 4K-aligned
> (see head_4xx.S, head_44x.S and head_fsl_booke.S in arch/ppc/kernel/).
> So depending on the actual kernel layout/configuration
> exception_thread_info could point 4K below the critical exception stack.
> Even if
> (exception_stack_bottom & ~(THREAD_SIZE-1)) != 0 (i.e. the stack is NOT 8K
> aligned) everything seems to work OK in most cases (the overwritten data
> are not so critical?). But we did encountered one case when trying to
> single step from the default breakpoint made the kernel to hang.
>
> There are several possible ways to fix that:
>
> - make the critical exception stack to be 8K aligned:
>
> ===========================================================================
>========== diff -upr a/arch/ppc/kernel/head_4xx.S
> b/arch/ppc/kernel/head_4xx.S --- a/arch/ppc/kernel/head_4xx.S
> 2006-07-05 20:23:51.000000000 +0400 +++ b/arch/ppc/kernel/head_4xx.S
> 2006-07-05 19:54:51.000000000 +0400 @@ -1001,7 +1001,7 @@ swapper_pg_dir:
>
> /* Stack for handling critical exceptions from kernel mode */
> .section .bss
> - .align 12
> + .align 13
> exception_stack_bottom:
> .space 4096
> critical_stack_top:
> ===========================================================================
>==========
>
> - replace call to current_thread_info() with:
>
> ===========================================================================
>========== diff -upr a/arch/ppc/kernel/kgdb.c b/arch/ppc/kernel/kgdb.c
> --- a/arch/ppc/kernel/kgdb.c 2006-07-05 19:53:29.000000000 +0400
> +++ b/arch/ppc/kernel/kgdb.c 2006-07-05 20:18:06.000000000 +0400
> @@ -106,29 +106,32 @@ static int kgdb_breakpoint(struct pt_reg
> static int kgdb_singlestep(struct pt_regs *regs)
> {
> struct thread_info *thread_info, *exception_thread_info;
> + register unsigned long sp asm("r1");
>
> if (user_mode(regs))
> return 0;
> +
> +#if defined(CONFIG_40x) || defined(CONFIG_44x) || defined(CONFIG_E500)
> /*
> - * On Book E and perhaps other processsors, singlestep is handled on
> + * On Book E, ppc40x, and ppc44x, singlestep is handled on
> * the critical exception stack. This causes current_thread_info()
> - * to fail, since it it locates the thread_info by masking off
> + * to fail, since it locates the thread_info by masking off
> * the low bits of the current stack pointer. We work around
> * this issue by copying the thread_info from the kernel stack
> * before calling kgdb_handle_exception, and copying it back
> - * afterwards. On most processors the copy is avoided since
> - * exception_thread_info == thread_info.
> + * afterwards.
> */
> +#define CRIT_STACK_SIZE 0x1000 /* 4kB */
> thread_info = (struct thread_info *)(regs->gpr[1] &
> ~(THREAD_SIZE-1)); - exception_thread_info = current_thread_info();
> -
> - if (thread_info != exception_thread_info)
> - memcpy(exception_thread_info, thread_info, sizeof
> *thread_info); + exception_thread_info = (struct thread_info *)(sp &
> ~(CRIT_STACK_SIZE-1)); + memcpy(exception_thread_info, thread_info,
> sizeof *thread_info);
>
> kgdb_handle_exception(0, SIGTRAP, 0, regs);
>
> - if (thread_info != exception_thread_info)
> - memcpy(thread_info, exception_thread_info, sizeof
> *thread_info); + memcpy(thread_info, exception_thread_info, sizeof
> *thread_info); +#else
> + kgdb_handle_exception(0, SIGTRAP, 0, regs);
> +#endif
>
> return 1;
> }
> ===========================================================================
>==========
>
> or
>
> ===========================================================================
>========== diff -upr a/arch/ppc/kernel/kgdb.c c/arch/ppc/kernel/kgdb.c
> --- a/arch/ppc/kernel/kgdb.c 2006-07-05 19:53:29.000000000 +0400
> +++ c/arch/ppc/kernel/kgdb.c 2006-07-05 20:17:51.000000000 +0400
> @@ -110,9 +110,9 @@ static int kgdb_singlestep(struct pt_reg
> if (user_mode(regs))
> return 0;
> /*
> - * On Book E and perhaps other processsors, singlestep is handled on
> + * On Book E and perhaps other processors, singlestep is handled on
> * the critical exception stack. This causes current_thread_info()
> - * to fail, since it it locates the thread_info by masking off
> + * to fail, since it locates the thread_info by masking off
> * the low bits of the current stack pointer. We work around
> * this issue by copying the thread_info from the kernel stack
> * before calling kgdb_handle_exception, and copying it back
> @@ -122,8 +122,14 @@ static int kgdb_singlestep(struct pt_reg
> thread_info = (struct thread_info *)(regs->gpr[1] &
> ~(THREAD_SIZE-1)); exception_thread_info = current_thread_info();
>
> - if (thread_info != exception_thread_info)
> + if (thread_info != exception_thread_info) {
> + #define CRIT_STACK_SIZE 0x1000
> + register unsigned long sp asm("r1");
> +
> + exception_thread_info =
> + (struct thread_info *)(sp & ~(CRIT_STACK_SIZE-1));
> memcpy(exception_thread_info, thread_info, sizeof
> *thread_info); + }
>
> kgdb_handle_exception(0, SIGTRAP, 0, regs);
>
> ===========================================================================
>==========
>
> To say the true, I don't like all the three (these are just to illustrate
> the idea). Seems some general cleanup in handling the stack size would make
> sense.
>
> Suggestions?
>
>
> Thanks,
> Andrei
>
>
> Using Tomcat but need to do more? Need to support web services, security?
> Get stuff done quickly with pre-integrated technology to make your job
> easier Download IBM WebSphere Application Server v.1.0.1 based on Apache
> Geronimo
> http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
> _______________________________________________
> Kgdb-bugreport mailing list
> Kgdb-bugreport@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
Kgdb-bugreport mailing list
Kgdb-bugreport@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
