Re: [PATCH] MIPS: KGDB: Use kernel context for sleeping threads

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello!

On 03/30/2017 06:06 PM, James Hogan wrote:

> KGDB is a kernel debug stub and it can't be used to debug userland as it
> can only safely access kernel memory.
>
> On MIPS however KGDB has always got the register state of sleeping
> processes from the userland register context at the beginning of the
> kernel stack. This is meaningless for kernel threads (which never enter
> userland), and for user threads it prevents the user seeing what it is
> doing while in the kernel:
>
> (gdb) info threads
>   Id   Target Id         Frame
>   ...
>   3    Thread 2 (kthreadd) 0x0000000000000000 in ?? ()
>   2    Thread 1 (init)   0x000000007705c4b4 in ?? ()
>   1    Thread -2 (shadowCPU0) 0xffffffff8012524c in arch_kgdb_breakpoint () at arch/mips/kernel/kgdb.c:201
>
> Get the register state instead from the (partial) kernel register
> context stored in the task's thread_struct for resume() to restore. All
> threads now correctly appear to be in context_switch():
>
> (gdb) info threads
>   Id   Target Id         Frame
>   ...
>   3    Thread 2 (kthreadd) context_switch (rq=<optimized out>, cookie=..., next=<optimized out>, prev=0x0) at kernel/sched/core.c:2903
>   2    Thread 1 (init)   context_switch (rq=<optimized out>, cookie=..., next=<optimized out>, prev=0x0) at kernel/sched/core.c:2903
>   1    Thread -2 (shadowCPU0) 0xffffffff8012524c in arch_kgdb_breakpoint () at arch/mips/kernel/kgdb.c:201
>
> Call clobbered registers which aren't saved and exception registers
> (BadVAddr & Cause) which can't be easily determined without stack
> unwinding are reported as 0. The PC is taken from the return address,
> such that the state presented matches that found immediately after
> returning from resume().
>
> Fixes: 8854700115ec ("[MIPS] kgdb: add arch support for the kernel's kgdb core")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Jason Wessel <jason.wessel@windriver.com>
> Cc: linux-mips@linux-mips.org
> Cc: stable@vger.kernel.org
> ---
>  arch/mips/kernel/kgdb.c | 48 ++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/arch/mips/kernel/kgdb.c b/arch/mips/kernel/kgdb.c
> index 1f4bd222ba76..eb6c0d582626 100644
> --- a/arch/mips/kernel/kgdb.c
> +++ b/arch/mips/kernel/kgdb.c
[...]
> @@ -254,25 +251,46 @@ void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
>  #endif
>
>  	for (reg = 0; reg < 16; reg++)
> -		*(ptr++) = regs->regs[reg];
> +		*(ptr++) = 0;

    Parens are not really necessary, you can get rid of them, while at it.

[...]

MBR, Sergei