* [PATCH] arm64/debug: Fix registers on sleeping tasks
@ 2018-03-01 19:38 Douglas Anderson
2018-03-02 18:01 ` Will Deacon
0 siblings, 1 reply; 6+ messages in thread
From: Douglas Anderson @ 2018-03-01 19:38 UTC (permalink / raw)
To: linux-arm-kernel
This is the equivalent of commit 001bf455d206 ("ARM: 8428/1: kgdb: Fix
registers on sleeping tasks") but for arm64. Nuff said.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
arch/arm64/kernel/kgdb.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 2122cd187f19..01285d4dcdc3 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -138,14 +138,26 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs)
void
sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *task)
{
- struct pt_regs *thread_regs;
+ struct thread_struct *thread = &task->thread;
+ struct cpu_context *cpu_context = &thread->cpu_context;
/* Initialize to zero */
memset((char *)gdb_regs, 0, NUMREGBYTES);
- thread_regs = task_pt_regs(task);
- memcpy((void *)gdb_regs, (void *)thread_regs->regs, GP_REG_BYTES);
- /* Special case for PSTATE (check comments in asm/kgdb.h for details) */
- dbg_get_reg(33, gdb_regs + GP_REG_BYTES, thread_regs);
+
+ gdb_regs[19] = cpu_context->x19;
+ gdb_regs[20] = cpu_context->x20;
+ gdb_regs[21] = cpu_context->x21;
+ gdb_regs[22] = cpu_context->x22;
+ gdb_regs[23] = cpu_context->x23;
+ gdb_regs[24] = cpu_context->x24;
+ gdb_regs[25] = cpu_context->x25;
+ gdb_regs[26] = cpu_context->x26;
+ gdb_regs[27] = cpu_context->x27;
+ gdb_regs[28] = cpu_context->x28;
+ gdb_regs[29] = cpu_context->fp;
+
+ gdb_regs[31] = cpu_context->sp;
+ gdb_regs[32] = cpu_context->pc;
}
void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
--
2.16.2.395.g2e18187dfd-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] arm64/debug: Fix registers on sleeping tasks
2018-03-01 19:38 [PATCH] arm64/debug: Fix registers on sleeping tasks Douglas Anderson
@ 2018-03-02 18:01 ` Will Deacon
2018-03-02 18:16 ` Mark Rutland
2018-03-02 18:43 ` Doug Anderson
0 siblings, 2 replies; 6+ messages in thread
From: Will Deacon @ 2018-03-02 18:01 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 01, 2018 at 11:38:03AM -0800, Douglas Anderson wrote:
> This is the equivalent of commit 001bf455d206 ("ARM: 8428/1: kgdb: Fix
> registers on sleeping tasks") but for arm64. Nuff said.
It's a pity that 001bf455d206 doesn't explain *why* past_pt_regs doesn't
work.
Anyway, does this mean you're actually using kgdb on arm64? Does the rest of
it appear to work?
Will
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 2122cd187f19..01285d4dcdc3 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -138,14 +138,26 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs)
> void
> sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *task)
> {
> - struct pt_regs *thread_regs;
> + struct thread_struct *thread = &task->thread;
> + struct cpu_context *cpu_context = &thread->cpu_context;
>
> /* Initialize to zero */
> memset((char *)gdb_regs, 0, NUMREGBYTES);
> - thread_regs = task_pt_regs(task);
> - memcpy((void *)gdb_regs, (void *)thread_regs->regs, GP_REG_BYTES);
> - /* Special case for PSTATE (check comments in asm/kgdb.h for details) */
> - dbg_get_reg(33, gdb_regs + GP_REG_BYTES, thread_regs);
> +
> + gdb_regs[19] = cpu_context->x19;
> + gdb_regs[20] = cpu_context->x20;
> + gdb_regs[21] = cpu_context->x21;
> + gdb_regs[22] = cpu_context->x22;
> + gdb_regs[23] = cpu_context->x23;
> + gdb_regs[24] = cpu_context->x24;
> + gdb_regs[25] = cpu_context->x25;
> + gdb_regs[26] = cpu_context->x26;
> + gdb_regs[27] = cpu_context->x27;
> + gdb_regs[28] = cpu_context->x28;
> + gdb_regs[29] = cpu_context->fp;
> +
> + gdb_regs[31] = cpu_context->sp;
> + gdb_regs[32] = cpu_context->pc;
> }
>
> void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
> --
> 2.16.2.395.g2e18187dfd-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] arm64/debug: Fix registers on sleeping tasks
2018-03-02 18:01 ` Will Deacon
@ 2018-03-02 18:16 ` Mark Rutland
2018-03-02 18:45 ` Doug Anderson
2018-03-02 18:43 ` Doug Anderson
1 sibling, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2018-03-02 18:16 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Mar 02, 2018 at 06:01:31PM +0000, Will Deacon wrote:
> On Thu, Mar 01, 2018 at 11:38:03AM -0800, Douglas Anderson wrote:
> > This is the equivalent of commit 001bf455d206 ("ARM: 8428/1: kgdb: Fix
> > registers on sleeping tasks") but for arm64. Nuff said.
>
> It's a pity that 001bf455d206 doesn't explain *why* past_pt_regs doesn't
> work.
The task_pt_regs are the userspace regs at the highest address on the
kernel stack:
#define task_pt_regs(p) \
((struct pt_regs *)(THREAD_SIZE + task_stack_page(p)) - 1)
... for kernel tasks, that's meaningless, and for user tasks, that won't
correspond to kernel state.
> > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> > index 2122cd187f19..01285d4dcdc3 100644
> > --- a/arch/arm64/kernel/kgdb.c
> > +++ b/arch/arm64/kernel/kgdb.c
> > @@ -138,14 +138,26 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs)
> > void
> > sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *task)
> > {
> > - struct pt_regs *thread_regs;
> > + struct thread_struct *thread = &task->thread;
> > + struct cpu_context *cpu_context = &thread->cpu_context;
We can do this in one go:
struct cpu_context *cpu_context = &task->thread.cpu_context;
... since we don't need the thread variable otherwise.
> >
> > /* Initialize to zero */
> > memset((char *)gdb_regs, 0, NUMREGBYTES);
> > - thread_regs = task_pt_regs(task);
> > - memcpy((void *)gdb_regs, (void *)thread_regs->regs, GP_REG_BYTES);
> > - /* Special case for PSTATE (check comments in asm/kgdb.h for details) */
> > - dbg_get_reg(33, gdb_regs + GP_REG_BYTES, thread_regs);
> > +
> > + gdb_regs[19] = cpu_context->x19;
> > + gdb_regs[20] = cpu_context->x20;
> > + gdb_regs[21] = cpu_context->x21;
> > + gdb_regs[22] = cpu_context->x22;
> > + gdb_regs[23] = cpu_context->x23;
> > + gdb_regs[24] = cpu_context->x24;
> > + gdb_regs[25] = cpu_context->x25;
> > + gdb_regs[26] = cpu_context->x26;
> > + gdb_regs[27] = cpu_context->x27;
> > + gdb_regs[28] = cpu_context->x28;
> > + gdb_regs[29] = cpu_context->fp;
> > +
> > + gdb_regs[31] = cpu_context->sp;
> > + gdb_regs[32] = cpu_context->pc;
Are the other reg fields initialised elsewhere?
We might want to zero them here.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] arm64/debug: Fix registers on sleeping tasks
2018-03-02 18:16 ` Mark Rutland
@ 2018-03-02 18:45 ` Doug Anderson
2018-03-05 11:01 ` Mark Rutland
0 siblings, 1 reply; 6+ messages in thread
From: Doug Anderson @ 2018-03-02 18:45 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Fri, Mar 2, 2018 at 10:16 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Mar 02, 2018 at 06:01:31PM +0000, Will Deacon wrote:
>> On Thu, Mar 01, 2018 at 11:38:03AM -0800, Douglas Anderson wrote:
>> > This is the equivalent of commit 001bf455d206 ("ARM: 8428/1: kgdb: Fix
>> > registers on sleeping tasks") but for arm64. Nuff said.
>>
>> It's a pity that 001bf455d206 doesn't explain *why* past_pt_regs doesn't
>> work.
>
> The task_pt_regs are the userspace regs at the highest address on the
> kernel stack:
>
> #define task_pt_regs(p) \
> ((struct pt_regs *)(THREAD_SIZE + task_stack_page(p)) - 1)
>
> ... for kernel tasks, that's meaningless, and for user tasks, that won't
> correspond to kernel state.
>
>> > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
>> > index 2122cd187f19..01285d4dcdc3 100644
>> > --- a/arch/arm64/kernel/kgdb.c
>> > +++ b/arch/arm64/kernel/kgdb.c
>> > @@ -138,14 +138,26 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs)
>> > void
>> > sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *task)
>> > {
>> > - struct pt_regs *thread_regs;
>> > + struct thread_struct *thread = &task->thread;
>> > + struct cpu_context *cpu_context = &thread->cpu_context;
>
> We can do this in one go:
>
> struct cpu_context *cpu_context = &task->thread.cpu_context;
>
> ... since we don't need the thread variable otherwise.
Sure, I can spin it that way if it makes you happy.
>> >
>> > /* Initialize to zero */
>> > memset((char *)gdb_regs, 0, NUMREGBYTES);
>> > - thread_regs = task_pt_regs(task);
>> > - memcpy((void *)gdb_regs, (void *)thread_regs->regs, GP_REG_BYTES);
>> > - /* Special case for PSTATE (check comments in asm/kgdb.h for details) */
>> > - dbg_get_reg(33, gdb_regs + GP_REG_BYTES, thread_regs);
>> > +
>> > + gdb_regs[19] = cpu_context->x19;
>> > + gdb_regs[20] = cpu_context->x20;
>> > + gdb_regs[21] = cpu_context->x21;
>> > + gdb_regs[22] = cpu_context->x22;
>> > + gdb_regs[23] = cpu_context->x23;
>> > + gdb_regs[24] = cpu_context->x24;
>> > + gdb_regs[25] = cpu_context->x25;
>> > + gdb_regs[26] = cpu_context->x26;
>> > + gdb_regs[27] = cpu_context->x27;
>> > + gdb_regs[28] = cpu_context->x28;
>> > + gdb_regs[29] = cpu_context->fp;
>> > +
>> > + gdb_regs[31] = cpu_context->sp;
>> > + gdb_regs[32] = cpu_context->pc;
>
> Are the other reg fields initialised elsewhere?
>
> We might want to zero them here.
Isn't that covered by the the "/* Initialize to zero */" comment and
and "memset((char *)gdb_regs, 0, NUMREGBYTES);"
...or are you thinking of initting something else to zero?
-Doug
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] arm64/debug: Fix registers on sleeping tasks
2018-03-02 18:45 ` Doug Anderson
@ 2018-03-05 11:01 ` Mark Rutland
0 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2018-03-05 11:01 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Mar 02, 2018 at 10:45:25AM -0800, Doug Anderson wrote:
> Hi,
>
> On Fri, Mar 2, 2018 at 10:16 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Mar 02, 2018 at 06:01:31PM +0000, Will Deacon wrote:
> >> On Thu, Mar 01, 2018 at 11:38:03AM -0800, Douglas Anderson wrote:
> >> > /* Initialize to zero */
> >> > memset((char *)gdb_regs, 0, NUMREGBYTES);
> >> > - thread_regs = task_pt_regs(task);
> >> > - memcpy((void *)gdb_regs, (void *)thread_regs->regs, GP_REG_BYTES);
> >> > - /* Special case for PSTATE (check comments in asm/kgdb.h for details) */
> >> > - dbg_get_reg(33, gdb_regs + GP_REG_BYTES, thread_regs);
> >> > +
> >> > + gdb_regs[19] = cpu_context->x19;
> >> > + gdb_regs[20] = cpu_context->x20;
> >> > + gdb_regs[21] = cpu_context->x21;
> >> > + gdb_regs[22] = cpu_context->x22;
> >> > + gdb_regs[23] = cpu_context->x23;
> >> > + gdb_regs[24] = cpu_context->x24;
> >> > + gdb_regs[25] = cpu_context->x25;
> >> > + gdb_regs[26] = cpu_context->x26;
> >> > + gdb_regs[27] = cpu_context->x27;
> >> > + gdb_regs[28] = cpu_context->x28;
> >> > + gdb_regs[29] = cpu_context->fp;
> >> > +
> >> > + gdb_regs[31] = cpu_context->sp;
> >> > + gdb_regs[32] = cpu_context->pc;
> >
> > Are the other reg fields initialised elsewhere?
> >
> > We might want to zero them here.
>
> Isn't that covered by the the "/* Initialize to zero */" comment and
> and "memset((char *)gdb_regs, 0, NUMREGBYTES);"
I'd misread the patch and thought that was part of the deleted lines.
That looks fine, then.
Mark.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64/debug: Fix registers on sleeping tasks
2018-03-02 18:01 ` Will Deacon
2018-03-02 18:16 ` Mark Rutland
@ 2018-03-02 18:43 ` Doug Anderson
1 sibling, 0 replies; 6+ messages in thread
From: Doug Anderson @ 2018-03-02 18:43 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Fri, Mar 2, 2018 at 10:01 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Mar 01, 2018 at 11:38:03AM -0800, Douglas Anderson wrote:
>> This is the equivalent of commit 001bf455d206 ("ARM: 8428/1: kgdb: Fix
>> registers on sleeping tasks") but for arm64. Nuff said.
>
> It's a pity that 001bf455d206 doesn't explain *why* past_pt_regs doesn't
> work.
Ah, sorry. I can add that to the description but Mark Rutland also
explained it. task_pt_regs are the userspace registers. That's not
what kgdb needs.
> Anyway, does this mean you're actually using kgdb on arm64? Does the rest of
> it appear to work?
Yeah, I've been using it for years. I had this patch on my list to
upstream but it conflicted with commit 0d15ef677839 ("arm64: kgdb:
Match pstate size with gdbserver protocol") and I never had time to
figure out if that mattered, so it's been sitting at
http://crosreview.com/371942 forever.
NOTE: in general I haven't been using single step and breakpoints
since those have always been a bit iffy on serial kgdb and especially
since we have read-only text on our system, but maybe I'll try that
now that I'm running with a newer kernel.
-Doug
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-05 11:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-01 19:38 [PATCH] arm64/debug: Fix registers on sleeping tasks Douglas Anderson
2018-03-02 18:01 ` Will Deacon
2018-03-02 18:16 ` Mark Rutland
2018-03-02 18:45 ` Doug Anderson
2018-03-05 11:01 ` Mark Rutland
2018-03-02 18:43 ` Doug Anderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).