* [RFC v2 PATCH 0/1] ARM64: KGDB: Add FPSIMD debug support
@ 2013-11-18 12:06 vijay.kilari at gmail.com
2013-11-18 12:06 ` [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD " vijay.kilari at gmail.com
0 siblings, 1 reply; 13+ messages in thread
From: vijay.kilari at gmail.com @ 2013-11-18 12:06 UTC (permalink / raw)
To: linux-arm-kernel
From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
Based on ARM64 KGDB support patches, KGDB support for
FPSIMD is added. Only debugging of FPSIMD kernel context
is supported.
This patch requires Ard's patches where in kernel support and
below patch for holding thread's fpsimd state.
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/277228
So CONFIG_KERNEL_MODE_NEON should be enabled.
with this, FPSIMD registers can be viewed or set from gdb tool.
Unlike CPU registers, the FPSIMD registers are not saved on
exception entry. With the known restriction that FPSIMD should not be
touched in interrupt/exception context, in this patch the FPSIMD
registers are directly read/written on gdb tool request
Here, the FPSIMD registers are read and restored for every FPSIMD
register read and write by GDB tool. So this has impact on
gdb tool response which is neglible. Other architectures like
mips are also implemented similarly
v2 changes:
- Added API to know thread fpsimd state by checking
TIF_FOREIGN_FPSTATE flag. This is based on below patch
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/277228
- Allow FPSIMD registers access only when FPSIMD is under use
by current thread
v1 changes:
- Initial patch
Tested on ARM64 simulator
Vijaya Kumar K (1):
ARM64: KGDB: Add FP/SIMD debug support
arch/arm64/include/asm/fpsimd.h | 1 +
arch/arm64/kernel/fpsimd.c | 5 ++
arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++--------------
3 files changed, 73 insertions(+), 38 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 13+ messages in thread* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2013-11-18 12:06 [RFC v2 PATCH 0/1] ARM64: KGDB: Add FPSIMD debug support vijay.kilari at gmail.com @ 2013-11-18 12:06 ` vijay.kilari at gmail.com 2013-11-25 18:52 ` Will Deacon 2013-11-29 8:53 ` Ard Biesheuvel 0 siblings, 2 replies; 13+ messages in thread From: vijay.kilari at gmail.com @ 2013-11-18 12:06 UTC (permalink / raw) To: linux-arm-kernel From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> Add KGDB debug support for FP/SIMD processor.This support only debugging of FP/SIMD in kernel mode. With this patch one can view or set FP/SIMD registers in kernel context Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> --- arch/arm64/include/asm/fpsimd.h | 1 + arch/arm64/kernel/fpsimd.c | 5 ++ arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++-------------- 3 files changed, 73 insertions(+), 38 deletions(-) diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h index 609bc44..5039d6d 100644 --- a/arch/arm64/include/asm/fpsimd.h +++ b/arch/arm64/include/asm/fpsimd.h @@ -60,6 +60,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state); extern void fpsimd_thread_switch(struct task_struct *next); extern void fpsimd_flush_thread(void); extern void fpsimd_reload_fpstate(void); +extern int fpsimd_thread_state(void); #endif diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 5b13c17..27604c1 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -124,6 +124,11 @@ void fpsimd_flush_thread(void) set_thread_flag(TIF_FOREIGN_FPSTATE); } +int fpsimd_thread_state(void) +{ + return (current->mm && test_thread_flag(TIF_FOREIGN_FPSTATE)) ? 1 : 0; +} + void fpsimd_reload_fpstate(void) { preempt_disable(); diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index f10f2ba..5e34852 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -23,6 +23,14 @@ #include <linux/kdebug.h> #include <linux/kgdb.h> #include <asm/traps.h> +#include <asm/fpsimd.h> + +#ifdef CONFIG_KERNEL_MODE_NEON +/* + * Structure to hold FP/SIMD register contents + */ +struct fpsimd_state kgdb_fpsimd_regs; +#endif struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { { "x0", 8, offsetof(struct pt_regs, regs[0])}, @@ -59,40 +67,40 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { { "sp", 8, offsetof(struct pt_regs, sp)}, { "pc", 8, offsetof(struct pt_regs, pc)}, { "pstate", 4, offsetof(struct pt_regs, pstate)}, - { "v0", 16, -1 }, - { "v1", 16, -1 }, - { "v2", 16, -1 }, - { "v3", 16, -1 }, - { "v4", 16, -1 }, - { "v5", 16, -1 }, - { "v6", 16, -1 }, - { "v7", 16, -1 }, - { "v8", 16, -1 }, - { "v9", 16, -1 }, - { "v10", 16, -1 }, - { "v11", 16, -1 }, - { "v12", 16, -1 }, - { "v13", 16, -1 }, - { "v14", 16, -1 }, - { "v15", 16, -1 }, - { "v16", 16, -1 }, - { "v17", 16, -1 }, - { "v18", 16, -1 }, - { "v19", 16, -1 }, - { "v20", 16, -1 }, - { "v21", 16, -1 }, - { "v22", 16, -1 }, - { "v23", 16, -1 }, - { "v24", 16, -1 }, - { "v25", 16, -1 }, - { "v26", 16, -1 }, - { "v27", 16, -1 }, - { "v28", 16, -1 }, - { "v29", 16, -1 }, - { "v30", 16, -1 }, - { "v31", 16, -1 }, - { "fpsr", 4, -1 }, - { "fpcr", 4, -1 }, + { "v0", 16, offsetof(struct fpsimd_state, vregs[0])}, + { "v1", 16, offsetof(struct fpsimd_state, vregs[1])}, + { "v2", 16, offsetof(struct fpsimd_state, vregs[2])}, + { "v3", 16, offsetof(struct fpsimd_state, vregs[3])}, + { "v4", 16, offsetof(struct fpsimd_state, vregs[4])}, + { "v5", 16, offsetof(struct fpsimd_state, vregs[5])}, + { "v6", 16, offsetof(struct fpsimd_state, vregs[6])}, + { "v7", 16, offsetof(struct fpsimd_state, vregs[7])}, + { "v8", 16, offsetof(struct fpsimd_state, vregs[8])}, + { "v9", 16, offsetof(struct fpsimd_state, vregs[9])}, + { "v10", 16, offsetof(struct fpsimd_state, vregs[10])}, + { "v11", 16, offsetof(struct fpsimd_state, vregs[11])}, + { "v12", 16, offsetof(struct fpsimd_state, vregs[12])}, + { "v13", 16, offsetof(struct fpsimd_state, vregs[13])}, + { "v14", 16, offsetof(struct fpsimd_state, vregs[14])}, + { "v15", 16, offsetof(struct fpsimd_state, vregs[15])}, + { "v16", 16, offsetof(struct fpsimd_state, vregs[16])}, + { "v17", 16, offsetof(struct fpsimd_state, vregs[17])}, + { "v18", 16, offsetof(struct fpsimd_state, vregs[18])}, + { "v19", 16, offsetof(struct fpsimd_state, vregs[19])}, + { "v20", 16, offsetof(struct fpsimd_state, vregs[20])}, + { "v21", 16, offsetof(struct fpsimd_state, vregs[21])}, + { "v22", 16, offsetof(struct fpsimd_state, vregs[22])}, + { "v23", 16, offsetof(struct fpsimd_state, vregs[23])}, + { "v24", 16, offsetof(struct fpsimd_state, vregs[24])}, + { "v25", 16, offsetof(struct fpsimd_state, vregs[25])}, + { "v26", 16, offsetof(struct fpsimd_state, vregs[26])}, + { "v27", 16, offsetof(struct fpsimd_state, vregs[27])}, + { "v28", 16, offsetof(struct fpsimd_state, vregs[28])}, + { "v29", 16, offsetof(struct fpsimd_state, vregs[29])}, + { "v30", 16, offsetof(struct fpsimd_state, vregs[30])}, + { "v31", 16, offsetof(struct fpsimd_state, vregs[31])}, + { "fpsr", 4, offsetof(struct fpsimd_state, fpsr)}, + { "fpcr", 4, offsetof(struct fpsimd_state, fpcr)}, }; char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) @@ -100,11 +108,22 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) if (regno >= DBG_MAX_REG_NUM || regno < 0) return NULL; - if (dbg_reg_def[regno].offset != -1) + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) memcpy(mem, (void *)regs + dbg_reg_def[regno].offset, dbg_reg_def[regno].size); - else + else { +#ifdef CONFIG_KERNEL_MODE_NEON + if (fpsimd_thread_state()) { + fpsimd_save_state(&kgdb_fpsimd_regs); + memcpy(mem, (void *)&kgdb_fpsimd_regs + + dbg_reg_def[regno].offset, + dbg_reg_def[regno].size); + } else + memset(mem, 0, dbg_reg_def[regno].size); +#else memset(mem, 0, dbg_reg_def[regno].size); +#endif + } return dbg_reg_def[regno].name; } @@ -113,9 +132,19 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs) if (regno >= DBG_MAX_REG_NUM || regno < 0) return -EINVAL; - if (dbg_reg_def[regno].offset != -1) + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) memcpy((void *)regs + dbg_reg_def[regno].offset, mem, - dbg_reg_def[regno].size); + dbg_reg_def[regno].size); +#ifdef CONFIG_KERNEL_MODE_NEON + else { + if (fpsimd_thread_state()) { + memcpy((void *)&kgdb_fpsimd_regs + + dbg_reg_def[regno].offset, + mem, dbg_reg_def[regno].size); + fpsimd_load_state(&kgdb_fpsimd_regs); + } + } +#endif return 0; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2013-11-18 12:06 ` [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD " vijay.kilari at gmail.com @ 2013-11-25 18:52 ` Will Deacon 2013-11-25 19:03 ` Ard Biesheuvel 2013-11-26 11:57 ` Vijay Kilari 2013-11-29 8:53 ` Ard Biesheuvel 1 sibling, 2 replies; 13+ messages in thread From: Will Deacon @ 2013-11-25 18:52 UTC (permalink / raw) To: linux-arm-kernel On Mon, Nov 18, 2013 at 12:06:57PM +0000, vijay.kilari at gmail.com wrote: > From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> > > Add KGDB debug support for FP/SIMD processor.This support > only debugging of FP/SIMD in kernel mode. > With this patch one can view or set FP/SIMD registers in > kernel context > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> > --- > arch/arm64/include/asm/fpsimd.h | 1 + > arch/arm64/kernel/fpsimd.c | 5 ++ > arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++-------------- > 3 files changed, 73 insertions(+), 38 deletions(-) > > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index 609bc44..5039d6d 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -60,6 +60,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state); > extern void fpsimd_thread_switch(struct task_struct *next); > extern void fpsimd_flush_thread(void); > extern void fpsimd_reload_fpstate(void); > +extern int fpsimd_thread_state(void); > > #endif > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 5b13c17..27604c1 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -124,6 +124,11 @@ void fpsimd_flush_thread(void) > set_thread_flag(TIF_FOREIGN_FPSTATE); > } > > +int fpsimd_thread_state(void) > +{ > + return (current->mm && test_thread_flag(TIF_FOREIGN_FPSTATE)) ? 1 : 0; > +} Erm... 1. `? 1 : 0' doesn't do anything 2. TIF_FOREIGN_FPSTATE doesn't seem to appear in my source tree (3.13-rc1) 3. Does it make sense for threads without an mm to have TIF_FOREIGN_FPSTATE set? > void fpsimd_reload_fpstate(void) > { > preempt_disable(); > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c > index f10f2ba..5e34852 100644 > --- a/arch/arm64/kernel/kgdb.c > +++ b/arch/arm64/kernel/kgdb.c > @@ -23,6 +23,14 @@ > #include <linux/kdebug.h> > #include <linux/kgdb.h> > #include <asm/traps.h> > +#include <asm/fpsimd.h> > + > +#ifdef CONFIG_KERNEL_MODE_NEON > +/* > + * Structure to hold FP/SIMD register contents > + */ > +struct fpsimd_state kgdb_fpsimd_regs; > +#endif > > struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { > { "x0", 8, offsetof(struct pt_regs, regs[0])}, > @@ -59,40 +67,40 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { > { "sp", 8, offsetof(struct pt_regs, sp)}, > { "pc", 8, offsetof(struct pt_regs, pc)}, > { "pstate", 4, offsetof(struct pt_regs, pstate)}, > - { "v0", 16, -1 }, > - { "v1", 16, -1 }, > - { "v2", 16, -1 }, > - { "v3", 16, -1 }, > - { "v4", 16, -1 }, > - { "v5", 16, -1 }, > - { "v6", 16, -1 }, > - { "v7", 16, -1 }, > - { "v8", 16, -1 }, > - { "v9", 16, -1 }, > - { "v10", 16, -1 }, > - { "v11", 16, -1 }, > - { "v12", 16, -1 }, > - { "v13", 16, -1 }, > - { "v14", 16, -1 }, > - { "v15", 16, -1 }, > - { "v16", 16, -1 }, > - { "v17", 16, -1 }, > - { "v18", 16, -1 }, > - { "v19", 16, -1 }, > - { "v20", 16, -1 }, > - { "v21", 16, -1 }, > - { "v22", 16, -1 }, > - { "v23", 16, -1 }, > - { "v24", 16, -1 }, > - { "v25", 16, -1 }, > - { "v26", 16, -1 }, > - { "v27", 16, -1 }, > - { "v28", 16, -1 }, > - { "v29", 16, -1 }, > - { "v30", 16, -1 }, > - { "v31", 16, -1 }, > - { "fpsr", 4, -1 }, > - { "fpcr", 4, -1 }, > + { "v0", 16, offsetof(struct fpsimd_state, vregs[0])}, > + { "v1", 16, offsetof(struct fpsimd_state, vregs[1])}, > + { "v2", 16, offsetof(struct fpsimd_state, vregs[2])}, > + { "v3", 16, offsetof(struct fpsimd_state, vregs[3])}, > + { "v4", 16, offsetof(struct fpsimd_state, vregs[4])}, > + { "v5", 16, offsetof(struct fpsimd_state, vregs[5])}, > + { "v6", 16, offsetof(struct fpsimd_state, vregs[6])}, > + { "v7", 16, offsetof(struct fpsimd_state, vregs[7])}, > + { "v8", 16, offsetof(struct fpsimd_state, vregs[8])}, > + { "v9", 16, offsetof(struct fpsimd_state, vregs[9])}, > + { "v10", 16, offsetof(struct fpsimd_state, vregs[10])}, > + { "v11", 16, offsetof(struct fpsimd_state, vregs[11])}, > + { "v12", 16, offsetof(struct fpsimd_state, vregs[12])}, > + { "v13", 16, offsetof(struct fpsimd_state, vregs[13])}, > + { "v14", 16, offsetof(struct fpsimd_state, vregs[14])}, > + { "v15", 16, offsetof(struct fpsimd_state, vregs[15])}, > + { "v16", 16, offsetof(struct fpsimd_state, vregs[16])}, > + { "v17", 16, offsetof(struct fpsimd_state, vregs[17])}, > + { "v18", 16, offsetof(struct fpsimd_state, vregs[18])}, > + { "v19", 16, offsetof(struct fpsimd_state, vregs[19])}, > + { "v20", 16, offsetof(struct fpsimd_state, vregs[20])}, > + { "v21", 16, offsetof(struct fpsimd_state, vregs[21])}, > + { "v22", 16, offsetof(struct fpsimd_state, vregs[22])}, > + { "v23", 16, offsetof(struct fpsimd_state, vregs[23])}, > + { "v24", 16, offsetof(struct fpsimd_state, vregs[24])}, > + { "v25", 16, offsetof(struct fpsimd_state, vregs[25])}, > + { "v26", 16, offsetof(struct fpsimd_state, vregs[26])}, > + { "v27", 16, offsetof(struct fpsimd_state, vregs[27])}, > + { "v28", 16, offsetof(struct fpsimd_state, vregs[28])}, > + { "v29", 16, offsetof(struct fpsimd_state, vregs[29])}, > + { "v30", 16, offsetof(struct fpsimd_state, vregs[30])}, > + { "v31", 16, offsetof(struct fpsimd_state, vregs[31])}, > + { "fpsr", 4, offsetof(struct fpsimd_state, fpsr)}, > + { "fpcr", 4, offsetof(struct fpsimd_state, fpcr)}, > }; > > char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) > @@ -100,11 +108,22 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) > if (regno >= DBG_MAX_REG_NUM || regno < 0) > return NULL; > > - if (dbg_reg_def[regno].offset != -1) > + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) > memcpy(mem, (void *)regs + dbg_reg_def[regno].offset, > dbg_reg_def[regno].size); > - else > + else { > +#ifdef CONFIG_KERNEL_MODE_NEON > + if (fpsimd_thread_state()) { > + fpsimd_save_state(&kgdb_fpsimd_regs); > + memcpy(mem, (void *)&kgdb_fpsimd_regs + > + dbg_reg_def[regno].offset, > + dbg_reg_def[regno].size); > + } else > + memset(mem, 0, dbg_reg_def[regno].size); > +#else > memset(mem, 0, dbg_reg_def[regno].size); Can you use IS_ENABLED to tidy this up? Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2013-11-25 18:52 ` Will Deacon @ 2013-11-25 19:03 ` Ard Biesheuvel 2013-11-26 11:57 ` Vijay Kilari 1 sibling, 0 replies; 13+ messages in thread From: Ard Biesheuvel @ 2013-11-25 19:03 UTC (permalink / raw) To: linux-arm-kernel On 25 November 2013 19:52, Will Deacon <will.deacon@arm.com> wrote: > On Mon, Nov 18, 2013 at 12:06:57PM +0000, vijay.kilari at gmail.com wrote: >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >> >> Add KGDB debug support for FP/SIMD processor.This support >> only debugging of FP/SIMD in kernel mode. >> With this patch one can view or set FP/SIMD registers in >> kernel context >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >> --- >> arch/arm64/include/asm/fpsimd.h | 1 + >> arch/arm64/kernel/fpsimd.c | 5 ++ >> arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++-------------- >> 3 files changed, 73 insertions(+), 38 deletions(-) >> >> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h >> index 609bc44..5039d6d 100644 >> --- a/arch/arm64/include/asm/fpsimd.h >> +++ b/arch/arm64/include/asm/fpsimd.h >> @@ -60,6 +60,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state); >> extern void fpsimd_thread_switch(struct task_struct *next); >> extern void fpsimd_flush_thread(void); >> extern void fpsimd_reload_fpstate(void); >> +extern int fpsimd_thread_state(void); >> >> #endif >> >> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >> index 5b13c17..27604c1 100644 >> --- a/arch/arm64/kernel/fpsimd.c >> +++ b/arch/arm64/kernel/fpsimd.c >> @@ -124,6 +124,11 @@ void fpsimd_flush_thread(void) >> set_thread_flag(TIF_FOREIGN_FPSTATE); >> } >> >> +int fpsimd_thread_state(void) >> +{ >> + return (current->mm && test_thread_flag(TIF_FOREIGN_FPSTATE)) ? 1 : 0; >> +} > > Erm... > > 1. `? 1 : 0' doesn't do anything > 2. TIF_FOREIGN_FPSTATE doesn't seem to appear in my source tree (3.13-rc1) > 3. Does it make sense for threads without an mm to have TIF_FOREIGN_FPSTATE set? > I think Vijay did mention at some point that his patches depend on my patch 'arm64: defer reloading a task's FPSIMD state to userland resume' which I posted here http://marc.info/?l=linux-arm-kernel&m=138418879110655&w=2 (with you on cc, IIRC) but which did not evoke any response on the list. -- Ard. >> void fpsimd_reload_fpstate(void) >> { >> preempt_disable(); >> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c >> index f10f2ba..5e34852 100644 >> --- a/arch/arm64/kernel/kgdb.c >> +++ b/arch/arm64/kernel/kgdb.c >> @@ -23,6 +23,14 @@ >> #include <linux/kdebug.h> >> #include <linux/kgdb.h> >> #include <asm/traps.h> >> +#include <asm/fpsimd.h> >> + >> +#ifdef CONFIG_KERNEL_MODE_NEON >> +/* >> + * Structure to hold FP/SIMD register contents >> + */ >> +struct fpsimd_state kgdb_fpsimd_regs; >> +#endif >> >> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >> { "x0", 8, offsetof(struct pt_regs, regs[0])}, >> @@ -59,40 +67,40 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >> { "sp", 8, offsetof(struct pt_regs, sp)}, >> { "pc", 8, offsetof(struct pt_regs, pc)}, >> { "pstate", 4, offsetof(struct pt_regs, pstate)}, >> - { "v0", 16, -1 }, >> - { "v1", 16, -1 }, >> - { "v2", 16, -1 }, >> - { "v3", 16, -1 }, >> - { "v4", 16, -1 }, >> - { "v5", 16, -1 }, >> - { "v6", 16, -1 }, >> - { "v7", 16, -1 }, >> - { "v8", 16, -1 }, >> - { "v9", 16, -1 }, >> - { "v10", 16, -1 }, >> - { "v11", 16, -1 }, >> - { "v12", 16, -1 }, >> - { "v13", 16, -1 }, >> - { "v14", 16, -1 }, >> - { "v15", 16, -1 }, >> - { "v16", 16, -1 }, >> - { "v17", 16, -1 }, >> - { "v18", 16, -1 }, >> - { "v19", 16, -1 }, >> - { "v20", 16, -1 }, >> - { "v21", 16, -1 }, >> - { "v22", 16, -1 }, >> - { "v23", 16, -1 }, >> - { "v24", 16, -1 }, >> - { "v25", 16, -1 }, >> - { "v26", 16, -1 }, >> - { "v27", 16, -1 }, >> - { "v28", 16, -1 }, >> - { "v29", 16, -1 }, >> - { "v30", 16, -1 }, >> - { "v31", 16, -1 }, >> - { "fpsr", 4, -1 }, >> - { "fpcr", 4, -1 }, >> + { "v0", 16, offsetof(struct fpsimd_state, vregs[0])}, >> + { "v1", 16, offsetof(struct fpsimd_state, vregs[1])}, >> + { "v2", 16, offsetof(struct fpsimd_state, vregs[2])}, >> + { "v3", 16, offsetof(struct fpsimd_state, vregs[3])}, >> + { "v4", 16, offsetof(struct fpsimd_state, vregs[4])}, >> + { "v5", 16, offsetof(struct fpsimd_state, vregs[5])}, >> + { "v6", 16, offsetof(struct fpsimd_state, vregs[6])}, >> + { "v7", 16, offsetof(struct fpsimd_state, vregs[7])}, >> + { "v8", 16, offsetof(struct fpsimd_state, vregs[8])}, >> + { "v9", 16, offsetof(struct fpsimd_state, vregs[9])}, >> + { "v10", 16, offsetof(struct fpsimd_state, vregs[10])}, >> + { "v11", 16, offsetof(struct fpsimd_state, vregs[11])}, >> + { "v12", 16, offsetof(struct fpsimd_state, vregs[12])}, >> + { "v13", 16, offsetof(struct fpsimd_state, vregs[13])}, >> + { "v14", 16, offsetof(struct fpsimd_state, vregs[14])}, >> + { "v15", 16, offsetof(struct fpsimd_state, vregs[15])}, >> + { "v16", 16, offsetof(struct fpsimd_state, vregs[16])}, >> + { "v17", 16, offsetof(struct fpsimd_state, vregs[17])}, >> + { "v18", 16, offsetof(struct fpsimd_state, vregs[18])}, >> + { "v19", 16, offsetof(struct fpsimd_state, vregs[19])}, >> + { "v20", 16, offsetof(struct fpsimd_state, vregs[20])}, >> + { "v21", 16, offsetof(struct fpsimd_state, vregs[21])}, >> + { "v22", 16, offsetof(struct fpsimd_state, vregs[22])}, >> + { "v23", 16, offsetof(struct fpsimd_state, vregs[23])}, >> + { "v24", 16, offsetof(struct fpsimd_state, vregs[24])}, >> + { "v25", 16, offsetof(struct fpsimd_state, vregs[25])}, >> + { "v26", 16, offsetof(struct fpsimd_state, vregs[26])}, >> + { "v27", 16, offsetof(struct fpsimd_state, vregs[27])}, >> + { "v28", 16, offsetof(struct fpsimd_state, vregs[28])}, >> + { "v29", 16, offsetof(struct fpsimd_state, vregs[29])}, >> + { "v30", 16, offsetof(struct fpsimd_state, vregs[30])}, >> + { "v31", 16, offsetof(struct fpsimd_state, vregs[31])}, >> + { "fpsr", 4, offsetof(struct fpsimd_state, fpsr)}, >> + { "fpcr", 4, offsetof(struct fpsimd_state, fpcr)}, >> }; >> >> char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >> @@ -100,11 +108,22 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >> if (regno >= DBG_MAX_REG_NUM || regno < 0) >> return NULL; >> >> - if (dbg_reg_def[regno].offset != -1) >> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >> memcpy(mem, (void *)regs + dbg_reg_def[regno].offset, >> dbg_reg_def[regno].size); >> - else >> + else { >> +#ifdef CONFIG_KERNEL_MODE_NEON >> + if (fpsimd_thread_state()) { >> + fpsimd_save_state(&kgdb_fpsimd_regs); >> + memcpy(mem, (void *)&kgdb_fpsimd_regs + >> + dbg_reg_def[regno].offset, >> + dbg_reg_def[regno].size); >> + } else >> + memset(mem, 0, dbg_reg_def[regno].size); >> +#else >> memset(mem, 0, dbg_reg_def[regno].size); > > Can you use IS_ENABLED to tidy this up? > > Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2013-11-25 18:52 ` Will Deacon 2013-11-25 19:03 ` Ard Biesheuvel @ 2013-11-26 11:57 ` Vijay Kilari 1 sibling, 0 replies; 13+ messages in thread From: Vijay Kilari @ 2013-11-26 11:57 UTC (permalink / raw) To: linux-arm-kernel On Tue, Nov 26, 2013 at 12:22 AM, Will Deacon <will.deacon@arm.com> wrote: > On Mon, Nov 18, 2013 at 12:06:57PM +0000, vijay.kilari at gmail.com wrote: >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >> >> Add KGDB debug support for FP/SIMD processor.This support >> only debugging of FP/SIMD in kernel mode. >> With this patch one can view or set FP/SIMD registers in >> kernel context >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >> --- >> arch/arm64/include/asm/fpsimd.h | 1 + >> arch/arm64/kernel/fpsimd.c | 5 ++ >> arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++-------------- >> 3 files changed, 73 insertions(+), 38 deletions(-) >> >> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h >> index 609bc44..5039d6d 100644 >> --- a/arch/arm64/include/asm/fpsimd.h >> +++ b/arch/arm64/include/asm/fpsimd.h >> @@ -60,6 +60,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state); >> extern void fpsimd_thread_switch(struct task_struct *next); >> extern void fpsimd_flush_thread(void); >> extern void fpsimd_reload_fpstate(void); >> +extern int fpsimd_thread_state(void); >> >> #endif >> >> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >> index 5b13c17..27604c1 100644 >> --- a/arch/arm64/kernel/fpsimd.c >> +++ b/arch/arm64/kernel/fpsimd.c >> @@ -124,6 +124,11 @@ void fpsimd_flush_thread(void) >> set_thread_flag(TIF_FOREIGN_FPSTATE); >> } >> >> +int fpsimd_thread_state(void) >> +{ >> + return (current->mm && test_thread_flag(TIF_FOREIGN_FPSTATE)) ? 1 : 0; >> +} > > Erm... > > 1. `? 1 : 0' doesn't do anything it worked for me. on true condition it returned 1 else 0. Any thing specific? > 2. TIF_FOREIGN_FPSTATE doesn't seem to appear in my source tree (3.13-rc1) As mentioned in cover patch, it is based on Ard's patch. I can include his patch if required. > 3. Does it make sense for threads without an mm to have TIF_FOREIGN_FPSTATE set? As per Ard's patch, this flag is set only if threads has mm. So mm check would be redundant here. Ard might have more inputs How about allowing to view FP/SIMD registers irrespective of FP/SIMD is used by kernel & user-space?. it will help user to view FP registers for debugging >> void fpsimd_reload_fpstate(void) >> { >> preempt_disable(); >> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c >> index f10f2ba..5e34852 100644 >> --- a/arch/arm64/kernel/kgdb.c >> +++ b/arch/arm64/kernel/kgdb.c >> @@ -23,6 +23,14 @@ >> #include <linux/kdebug.h> >> #include <linux/kgdb.h> >> #include <asm/traps.h> >> +#include <asm/fpsimd.h> >> + >> +#ifdef CONFIG_KERNEL_MODE_NEON >> +/* >> + * Structure to hold FP/SIMD register contents >> + */ >> +struct fpsimd_state kgdb_fpsimd_regs; >> +#endif >> >> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >> { "x0", 8, offsetof(struct pt_regs, regs[0])}, >> @@ -59,40 +67,40 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >> { "sp", 8, offsetof(struct pt_regs, sp)}, >> { "pc", 8, offsetof(struct pt_regs, pc)}, >> { "pstate", 4, offsetof(struct pt_regs, pstate)}, >> - { "v0", 16, -1 }, >> - { "v1", 16, -1 }, >> - { "v2", 16, -1 }, >> - { "v3", 16, -1 }, >> - { "v4", 16, -1 }, >> - { "v5", 16, -1 }, >> - { "v6", 16, -1 }, >> - { "v7", 16, -1 }, >> - { "v8", 16, -1 }, >> - { "v9", 16, -1 }, >> - { "v10", 16, -1 }, >> - { "v11", 16, -1 }, >> - { "v12", 16, -1 }, >> - { "v13", 16, -1 }, >> - { "v14", 16, -1 }, >> - { "v15", 16, -1 }, >> - { "v16", 16, -1 }, >> - { "v17", 16, -1 }, >> - { "v18", 16, -1 }, >> - { "v19", 16, -1 }, >> - { "v20", 16, -1 }, >> - { "v21", 16, -1 }, >> - { "v22", 16, -1 }, >> - { "v23", 16, -1 }, >> - { "v24", 16, -1 }, >> - { "v25", 16, -1 }, >> - { "v26", 16, -1 }, >> - { "v27", 16, -1 }, >> - { "v28", 16, -1 }, >> - { "v29", 16, -1 }, >> - { "v30", 16, -1 }, >> - { "v31", 16, -1 }, >> - { "fpsr", 4, -1 }, >> - { "fpcr", 4, -1 }, >> + { "v0", 16, offsetof(struct fpsimd_state, vregs[0])}, >> + { "v1", 16, offsetof(struct fpsimd_state, vregs[1])}, >> + { "v2", 16, offsetof(struct fpsimd_state, vregs[2])}, >> + { "v3", 16, offsetof(struct fpsimd_state, vregs[3])}, >> + { "v4", 16, offsetof(struct fpsimd_state, vregs[4])}, >> + { "v5", 16, offsetof(struct fpsimd_state, vregs[5])}, >> + { "v6", 16, offsetof(struct fpsimd_state, vregs[6])}, >> + { "v7", 16, offsetof(struct fpsimd_state, vregs[7])}, >> + { "v8", 16, offsetof(struct fpsimd_state, vregs[8])}, >> + { "v9", 16, offsetof(struct fpsimd_state, vregs[9])}, >> + { "v10", 16, offsetof(struct fpsimd_state, vregs[10])}, >> + { "v11", 16, offsetof(struct fpsimd_state, vregs[11])}, >> + { "v12", 16, offsetof(struct fpsimd_state, vregs[12])}, >> + { "v13", 16, offsetof(struct fpsimd_state, vregs[13])}, >> + { "v14", 16, offsetof(struct fpsimd_state, vregs[14])}, >> + { "v15", 16, offsetof(struct fpsimd_state, vregs[15])}, >> + { "v16", 16, offsetof(struct fpsimd_state, vregs[16])}, >> + { "v17", 16, offsetof(struct fpsimd_state, vregs[17])}, >> + { "v18", 16, offsetof(struct fpsimd_state, vregs[18])}, >> + { "v19", 16, offsetof(struct fpsimd_state, vregs[19])}, >> + { "v20", 16, offsetof(struct fpsimd_state, vregs[20])}, >> + { "v21", 16, offsetof(struct fpsimd_state, vregs[21])}, >> + { "v22", 16, offsetof(struct fpsimd_state, vregs[22])}, >> + { "v23", 16, offsetof(struct fpsimd_state, vregs[23])}, >> + { "v24", 16, offsetof(struct fpsimd_state, vregs[24])}, >> + { "v25", 16, offsetof(struct fpsimd_state, vregs[25])}, >> + { "v26", 16, offsetof(struct fpsimd_state, vregs[26])}, >> + { "v27", 16, offsetof(struct fpsimd_state, vregs[27])}, >> + { "v28", 16, offsetof(struct fpsimd_state, vregs[28])}, >> + { "v29", 16, offsetof(struct fpsimd_state, vregs[29])}, >> + { "v30", 16, offsetof(struct fpsimd_state, vregs[30])}, >> + { "v31", 16, offsetof(struct fpsimd_state, vregs[31])}, >> + { "fpsr", 4, offsetof(struct fpsimd_state, fpsr)}, >> + { "fpcr", 4, offsetof(struct fpsimd_state, fpcr)}, >> }; >> >> char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >> @@ -100,11 +108,22 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >> if (regno >= DBG_MAX_REG_NUM || regno < 0) >> return NULL; >> >> - if (dbg_reg_def[regno].offset != -1) >> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >> memcpy(mem, (void *)regs + dbg_reg_def[regno].offset, >> dbg_reg_def[regno].size); >> - else >> + else { >> +#ifdef CONFIG_KERNEL_MODE_NEON >> + if (fpsimd_thread_state()) { >> + fpsimd_save_state(&kgdb_fpsimd_regs); >> + memcpy(mem, (void *)&kgdb_fpsimd_regs + >> + dbg_reg_def[regno].offset, >> + dbg_reg_def[regno].size); >> + } else >> + memset(mem, 0, dbg_reg_def[regno].size); >> +#else >> memset(mem, 0, dbg_reg_def[regno].size); > > Can you use IS_ENABLED to tidy this up? Yes, will take care in next version > > Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2013-11-18 12:06 ` [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD " vijay.kilari at gmail.com 2013-11-25 18:52 ` Will Deacon @ 2013-11-29 8:53 ` Ard Biesheuvel 2013-11-29 9:20 ` Vijay Kilari 1 sibling, 1 reply; 13+ messages in thread From: Ard Biesheuvel @ 2013-11-29 8:53 UTC (permalink / raw) To: linux-arm-kernel On 18 November 2013 13:06, <vijay.kilari@gmail.com> wrote: > From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> > > Add KGDB debug support for FP/SIMD processor.This support > only debugging of FP/SIMD in kernel mode. > With this patch one can view or set FP/SIMD registers in > kernel context > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> > --- > arch/arm64/include/asm/fpsimd.h | 1 + > arch/arm64/kernel/fpsimd.c | 5 ++ > arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++-------------- > 3 files changed, 73 insertions(+), 38 deletions(-) > > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index 609bc44..5039d6d 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -60,6 +60,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state); > extern void fpsimd_thread_switch(struct task_struct *next); > extern void fpsimd_flush_thread(void); > extern void fpsimd_reload_fpstate(void); > +extern int fpsimd_thread_state(void); > > #endif > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 5b13c17..27604c1 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -124,6 +124,11 @@ void fpsimd_flush_thread(void) > set_thread_flag(TIF_FOREIGN_FPSTATE); > } > > +int fpsimd_thread_state(void) Please use a meaningful name for the function. Currently, it returns true iff 'current' is a non-kernel thread whose FPSIMD state is /not/ present in the FPSIMD register file. You should choose a name that reflects that purpose. > +{ > + return (current->mm && test_thread_flag(TIF_FOREIGN_FPSTATE)) ? 1 : 0; > +} > + As Will pointed out, the expression before ? already evaluates to 1 or 0, so appending '? 1 : 0' is redundant. > void fpsimd_reload_fpstate(void) > { > preempt_disable(); > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c > index f10f2ba..5e34852 100644 > --- a/arch/arm64/kernel/kgdb.c > +++ b/arch/arm64/kernel/kgdb.c > @@ -23,6 +23,14 @@ > #include <linux/kdebug.h> > #include <linux/kgdb.h> > #include <asm/traps.h> > +#include <asm/fpsimd.h> > + > +#ifdef CONFIG_KERNEL_MODE_NEON > +/* > + * Structure to hold FP/SIMD register contents > + */ > +struct fpsimd_state kgdb_fpsimd_regs; 'static' if it's local to this source file > +#endif > > struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { > { "x0", 8, offsetof(struct pt_regs, regs[0])}, > @@ -59,40 +67,40 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { > { "sp", 8, offsetof(struct pt_regs, sp)}, > { "pc", 8, offsetof(struct pt_regs, pc)}, > { "pstate", 4, offsetof(struct pt_regs, pstate)}, > - { "v0", 16, -1 }, > - { "v1", 16, -1 }, > - { "v2", 16, -1 }, > - { "v3", 16, -1 }, > - { "v4", 16, -1 }, > - { "v5", 16, -1 }, > - { "v6", 16, -1 }, > - { "v7", 16, -1 }, > - { "v8", 16, -1 }, > - { "v9", 16, -1 }, > - { "v10", 16, -1 }, > - { "v11", 16, -1 }, > - { "v12", 16, -1 }, > - { "v13", 16, -1 }, > - { "v14", 16, -1 }, > - { "v15", 16, -1 }, > - { "v16", 16, -1 }, > - { "v17", 16, -1 }, > - { "v18", 16, -1 }, > - { "v19", 16, -1 }, > - { "v20", 16, -1 }, > - { "v21", 16, -1 }, > - { "v22", 16, -1 }, > - { "v23", 16, -1 }, > - { "v24", 16, -1 }, > - { "v25", 16, -1 }, > - { "v26", 16, -1 }, > - { "v27", 16, -1 }, > - { "v28", 16, -1 }, > - { "v29", 16, -1 }, > - { "v30", 16, -1 }, > - { "v31", 16, -1 }, > - { "fpsr", 4, -1 }, > - { "fpcr", 4, -1 }, > + { "v0", 16, offsetof(struct fpsimd_state, vregs[0])}, > + { "v1", 16, offsetof(struct fpsimd_state, vregs[1])}, > + { "v2", 16, offsetof(struct fpsimd_state, vregs[2])}, > + { "v3", 16, offsetof(struct fpsimd_state, vregs[3])}, > + { "v4", 16, offsetof(struct fpsimd_state, vregs[4])}, > + { "v5", 16, offsetof(struct fpsimd_state, vregs[5])}, > + { "v6", 16, offsetof(struct fpsimd_state, vregs[6])}, > + { "v7", 16, offsetof(struct fpsimd_state, vregs[7])}, > + { "v8", 16, offsetof(struct fpsimd_state, vregs[8])}, > + { "v9", 16, offsetof(struct fpsimd_state, vregs[9])}, > + { "v10", 16, offsetof(struct fpsimd_state, vregs[10])}, > + { "v11", 16, offsetof(struct fpsimd_state, vregs[11])}, > + { "v12", 16, offsetof(struct fpsimd_state, vregs[12])}, > + { "v13", 16, offsetof(struct fpsimd_state, vregs[13])}, > + { "v14", 16, offsetof(struct fpsimd_state, vregs[14])}, > + { "v15", 16, offsetof(struct fpsimd_state, vregs[15])}, > + { "v16", 16, offsetof(struct fpsimd_state, vregs[16])}, > + { "v17", 16, offsetof(struct fpsimd_state, vregs[17])}, > + { "v18", 16, offsetof(struct fpsimd_state, vregs[18])}, > + { "v19", 16, offsetof(struct fpsimd_state, vregs[19])}, > + { "v20", 16, offsetof(struct fpsimd_state, vregs[20])}, > + { "v21", 16, offsetof(struct fpsimd_state, vregs[21])}, > + { "v22", 16, offsetof(struct fpsimd_state, vregs[22])}, > + { "v23", 16, offsetof(struct fpsimd_state, vregs[23])}, > + { "v24", 16, offsetof(struct fpsimd_state, vregs[24])}, > + { "v25", 16, offsetof(struct fpsimd_state, vregs[25])}, > + { "v26", 16, offsetof(struct fpsimd_state, vregs[26])}, > + { "v27", 16, offsetof(struct fpsimd_state, vregs[27])}, > + { "v28", 16, offsetof(struct fpsimd_state, vregs[28])}, > + { "v29", 16, offsetof(struct fpsimd_state, vregs[29])}, > + { "v30", 16, offsetof(struct fpsimd_state, vregs[30])}, > + { "v31", 16, offsetof(struct fpsimd_state, vregs[31])}, > + { "fpsr", 4, offsetof(struct fpsimd_state, fpsr)}, > + { "fpcr", 4, offsetof(struct fpsimd_state, fpcr)}, > }; > > char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) > @@ -100,11 +108,22 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) > if (regno >= DBG_MAX_REG_NUM || regno < 0) > return NULL; > > - if (dbg_reg_def[regno].offset != -1) > + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) > memcpy(mem, (void *)regs + dbg_reg_def[regno].offset, > dbg_reg_def[regno].size); > - else > + else { > +#ifdef CONFIG_KERNEL_MODE_NEON else if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON)) { > + if (fpsimd_thread_state()) { > + fpsimd_save_state(&kgdb_fpsimd_regs); > + memcpy(mem, (void *)&kgdb_fpsimd_regs + > + dbg_reg_def[regno].offset, > + dbg_reg_def[regno].size); Why do this conditionally? Shouldn't kgdb return whatever is currently present in the register file, regardless of whether 'current' is a kernel thread or has had its FPSIMD state swapped out? Also, one could wonder why this is dependent on KERNEL_MODE_NEON being set > + } else > + memset(mem, 0, dbg_reg_def[regno].size); > +#else > memset(mem, 0, dbg_reg_def[regno].size); > +#endif > + } > return dbg_reg_def[regno].name; > } > > @@ -113,9 +132,19 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs) > if (regno >= DBG_MAX_REG_NUM || regno < 0) > return -EINVAL; > > - if (dbg_reg_def[regno].offset != -1) > + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) > memcpy((void *)regs + dbg_reg_def[regno].offset, mem, > - dbg_reg_def[regno].size); > + dbg_reg_def[regno].size); > +#ifdef CONFIG_KERNEL_MODE_NEON > + else { > + if (fpsimd_thread_state()) { Again, why the conditional? > + memcpy((void *)&kgdb_fpsimd_regs + > + dbg_reg_def[regno].offset, > + mem, dbg_reg_def[regno].size); > + fpsimd_load_state(&kgdb_fpsimd_regs); Are you sure it is safe to clobber all the registers here? Shouldn't you save the state first, do the memcpy() and then load? Regards, Ard. > + } > + } > +#endif > return 0; > } > > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2013-11-29 8:53 ` Ard Biesheuvel @ 2013-11-29 9:20 ` Vijay Kilari 2013-11-29 9:27 ` Ard Biesheuvel 0 siblings, 1 reply; 13+ messages in thread From: Vijay Kilari @ 2013-11-29 9:20 UTC (permalink / raw) To: linux-arm-kernel On Fri, Nov 29, 2013 at 2:23 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 18 November 2013 13:06, <vijay.kilari@gmail.com> wrote: >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >> >> Add KGDB debug support for FP/SIMD processor.This support >> only debugging of FP/SIMD in kernel mode. >> With this patch one can view or set FP/SIMD registers in >> kernel context >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >> --- >> arch/arm64/include/asm/fpsimd.h | 1 + >> arch/arm64/kernel/fpsimd.c | 5 ++ >> arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++-------------- >> 3 files changed, 73 insertions(+), 38 deletions(-) >> >> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h >> index 609bc44..5039d6d 100644 >> --- a/arch/arm64/include/asm/fpsimd.h >> +++ b/arch/arm64/include/asm/fpsimd.h >> @@ -60,6 +60,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state); >> extern void fpsimd_thread_switch(struct task_struct *next); >> extern void fpsimd_flush_thread(void); >> extern void fpsimd_reload_fpstate(void); >> +extern int fpsimd_thread_state(void); >> >> #endif >> >> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >> index 5b13c17..27604c1 100644 >> --- a/arch/arm64/kernel/fpsimd.c >> +++ b/arch/arm64/kernel/fpsimd.c >> @@ -124,6 +124,11 @@ void fpsimd_flush_thread(void) >> set_thread_flag(TIF_FOREIGN_FPSTATE); >> } >> >> +int fpsimd_thread_state(void) > > Please use a meaningful name for the function. Currently, it returns > true iff 'current' is a non-kernel thread whose FPSIMD state is /not/ > present in the FPSIMD register file. You should choose a name that > reflects that purpose. > do you have any reason why only TIF_FOREIGN_FPSTATE flag is set only for non-kernel threads in kernel_neon_begin()? >> +{ >> + return (current->mm && test_thread_flag(TIF_FOREIGN_FPSTATE)) ? 1 : 0; >> +} >> + > > As Will pointed out, the expression before ? already evaluates to 1 or > 0, so appending '? 1 : 0' is redundant. > >> void fpsimd_reload_fpstate(void) >> { >> preempt_disable(); >> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c >> index f10f2ba..5e34852 100644 >> --- a/arch/arm64/kernel/kgdb.c >> +++ b/arch/arm64/kernel/kgdb.c >> @@ -23,6 +23,14 @@ >> #include <linux/kdebug.h> >> #include <linux/kgdb.h> >> #include <asm/traps.h> >> +#include <asm/fpsimd.h> >> + >> +#ifdef CONFIG_KERNEL_MODE_NEON >> +/* >> + * Structure to hold FP/SIMD register contents >> + */ >> +struct fpsimd_state kgdb_fpsimd_regs; > > 'static' if it's local to this source file > OK >> +#endif >> >> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >> { "x0", 8, offsetof(struct pt_regs, regs[0])}, >> @@ -59,40 +67,40 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >> { "sp", 8, offsetof(struct pt_regs, sp)}, >> { "pc", 8, offsetof(struct pt_regs, pc)}, >> { "pstate", 4, offsetof(struct pt_regs, pstate)}, >> - { "v0", 16, -1 }, >> - { "v1", 16, -1 }, >> - { "v2", 16, -1 }, >> - { "v3", 16, -1 }, >> - { "v4", 16, -1 }, >> - { "v5", 16, -1 }, >> - { "v6", 16, -1 }, >> - { "v7", 16, -1 }, >> - { "v8", 16, -1 }, >> - { "v9", 16, -1 }, >> - { "v10", 16, -1 }, >> - { "v11", 16, -1 }, >> - { "v12", 16, -1 }, >> - { "v13", 16, -1 }, >> - { "v14", 16, -1 }, >> - { "v15", 16, -1 }, >> - { "v16", 16, -1 }, >> - { "v17", 16, -1 }, >> - { "v18", 16, -1 }, >> - { "v19", 16, -1 }, >> - { "v20", 16, -1 }, >> - { "v21", 16, -1 }, >> - { "v22", 16, -1 }, >> - { "v23", 16, -1 }, >> - { "v24", 16, -1 }, >> - { "v25", 16, -1 }, >> - { "v26", 16, -1 }, >> - { "v27", 16, -1 }, >> - { "v28", 16, -1 }, >> - { "v29", 16, -1 }, >> - { "v30", 16, -1 }, >> - { "v31", 16, -1 }, >> - { "fpsr", 4, -1 }, >> - { "fpcr", 4, -1 }, >> + { "v0", 16, offsetof(struct fpsimd_state, vregs[0])}, >> + { "v1", 16, offsetof(struct fpsimd_state, vregs[1])}, >> + { "v2", 16, offsetof(struct fpsimd_state, vregs[2])}, >> + { "v3", 16, offsetof(struct fpsimd_state, vregs[3])}, >> + { "v4", 16, offsetof(struct fpsimd_state, vregs[4])}, >> + { "v5", 16, offsetof(struct fpsimd_state, vregs[5])}, >> + { "v6", 16, offsetof(struct fpsimd_state, vregs[6])}, >> + { "v7", 16, offsetof(struct fpsimd_state, vregs[7])}, >> + { "v8", 16, offsetof(struct fpsimd_state, vregs[8])}, >> + { "v9", 16, offsetof(struct fpsimd_state, vregs[9])}, >> + { "v10", 16, offsetof(struct fpsimd_state, vregs[10])}, >> + { "v11", 16, offsetof(struct fpsimd_state, vregs[11])}, >> + { "v12", 16, offsetof(struct fpsimd_state, vregs[12])}, >> + { "v13", 16, offsetof(struct fpsimd_state, vregs[13])}, >> + { "v14", 16, offsetof(struct fpsimd_state, vregs[14])}, >> + { "v15", 16, offsetof(struct fpsimd_state, vregs[15])}, >> + { "v16", 16, offsetof(struct fpsimd_state, vregs[16])}, >> + { "v17", 16, offsetof(struct fpsimd_state, vregs[17])}, >> + { "v18", 16, offsetof(struct fpsimd_state, vregs[18])}, >> + { "v19", 16, offsetof(struct fpsimd_state, vregs[19])}, >> + { "v20", 16, offsetof(struct fpsimd_state, vregs[20])}, >> + { "v21", 16, offsetof(struct fpsimd_state, vregs[21])}, >> + { "v22", 16, offsetof(struct fpsimd_state, vregs[22])}, >> + { "v23", 16, offsetof(struct fpsimd_state, vregs[23])}, >> + { "v24", 16, offsetof(struct fpsimd_state, vregs[24])}, >> + { "v25", 16, offsetof(struct fpsimd_state, vregs[25])}, >> + { "v26", 16, offsetof(struct fpsimd_state, vregs[26])}, >> + { "v27", 16, offsetof(struct fpsimd_state, vregs[27])}, >> + { "v28", 16, offsetof(struct fpsimd_state, vregs[28])}, >> + { "v29", 16, offsetof(struct fpsimd_state, vregs[29])}, >> + { "v30", 16, offsetof(struct fpsimd_state, vregs[30])}, >> + { "v31", 16, offsetof(struct fpsimd_state, vregs[31])}, >> + { "fpsr", 4, offsetof(struct fpsimd_state, fpsr)}, >> + { "fpcr", 4, offsetof(struct fpsimd_state, fpcr)}, >> }; >> >> char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >> @@ -100,11 +108,22 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >> if (regno >= DBG_MAX_REG_NUM || regno < 0) >> return NULL; >> >> - if (dbg_reg_def[regno].offset != -1) >> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >> memcpy(mem, (void *)regs + dbg_reg_def[regno].offset, >> dbg_reg_def[regno].size); >> - else >> + else { >> +#ifdef CONFIG_KERNEL_MODE_NEON > > else if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON)) { > >> + if (fpsimd_thread_state()) { >> + fpsimd_save_state(&kgdb_fpsimd_regs); >> + memcpy(mem, (void *)&kgdb_fpsimd_regs + >> + dbg_reg_def[regno].offset, >> + dbg_reg_def[regno].size); > > Why do this conditionally? Shouldn't kgdb return whatever is currently > present in the register file, regardless of whether 'current' is a > kernel thread or has had its FPSIMD state swapped out? > Also, one could wonder why this is dependent on KERNEL_MODE_NEON being set > KGDB, being kernel debugger it was thought that allow debugging only if kernel mode is supported. In fact, I proposed (previous email) to remove this condition and allow to reading neon registers always and allow write only if neon is in kernel mode. But completely removing this condition check and allowing to modify is user choice to ensure neon registers state. Will, Please let me know your opinion? >> + } else >> + memset(mem, 0, dbg_reg_def[regno].size); >> +#else >> memset(mem, 0, dbg_reg_def[regno].size); >> +#endif >> + } >> return dbg_reg_def[regno].name; >> } >> >> @@ -113,9 +132,19 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs) >> if (regno >= DBG_MAX_REG_NUM || regno < 0) >> return -EINVAL; >> >> - if (dbg_reg_def[regno].offset != -1) >> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >> memcpy((void *)regs + dbg_reg_def[regno].offset, mem, >> - dbg_reg_def[regno].size); >> + dbg_reg_def[regno].size); >> +#ifdef CONFIG_KERNEL_MODE_NEON >> + else { >> + if (fpsimd_thread_state()) { > > Again, why the conditional? > >> + memcpy((void *)&kgdb_fpsimd_regs + >> + dbg_reg_def[regno].offset, >> + mem, dbg_reg_def[regno].size); >> + fpsimd_load_state(&kgdb_fpsimd_regs); > > Are you sure it is safe to clobber all the registers here? Shouldn't > you save the state first, do the memcpy() and then load? > yes it is safe. gdb read registers after every command so all registers are saved first before they are modified > Regards, > Ard. > > > > >> + } >> + } >> +#endif >> return 0; >> } >> >> -- >> 1.7.9.5 >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2013-11-29 9:20 ` Vijay Kilari @ 2013-11-29 9:27 ` Ard Biesheuvel 2013-11-29 9:57 ` Vijay Kilari 0 siblings, 1 reply; 13+ messages in thread From: Ard Biesheuvel @ 2013-11-29 9:27 UTC (permalink / raw) To: linux-arm-kernel On 29 November 2013 10:20, Vijay Kilari <vijay.kilari@gmail.com> wrote: > On Fri, Nov 29, 2013 at 2:23 PM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> On 18 November 2013 13:06, <vijay.kilari@gmail.com> wrote: >>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >>> >>> Add KGDB debug support for FP/SIMD processor.This support >>> only debugging of FP/SIMD in kernel mode. >>> With this patch one can view or set FP/SIMD registers in >>> kernel context >>> >>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >>> --- >>> arch/arm64/include/asm/fpsimd.h | 1 + >>> arch/arm64/kernel/fpsimd.c | 5 ++ >>> arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++-------------- >>> 3 files changed, 73 insertions(+), 38 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h >>> index 609bc44..5039d6d 100644 >>> --- a/arch/arm64/include/asm/fpsimd.h >>> +++ b/arch/arm64/include/asm/fpsimd.h >>> @@ -60,6 +60,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state); >>> extern void fpsimd_thread_switch(struct task_struct *next); >>> extern void fpsimd_flush_thread(void); >>> extern void fpsimd_reload_fpstate(void); >>> +extern int fpsimd_thread_state(void); >>> >>> #endif >>> >>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >>> index 5b13c17..27604c1 100644 >>> --- a/arch/arm64/kernel/fpsimd.c >>> +++ b/arch/arm64/kernel/fpsimd.c >>> @@ -124,6 +124,11 @@ void fpsimd_flush_thread(void) >>> set_thread_flag(TIF_FOREIGN_FPSTATE); >>> } >>> >>> +int fpsimd_thread_state(void) >> >> Please use a meaningful name for the function. Currently, it returns >> true iff 'current' is a non-kernel thread whose FPSIMD state is /not/ >> present in the FPSIMD register file. You should choose a name that >> reflects that purpose. >> > do you have any reason why only TIF_FOREIGN_FPSTATE flag is > set only for non-kernel threads in kernel_neon_begin()? > The purpose of the TIF_FOREIGN_FPSTATE flag is to convey that the FPSIMD context should be restored before a task enters userland. A kernel thread never enters userland by definition so the flag has no meaning there. >>> +{ >>> + return (current->mm && test_thread_flag(TIF_FOREIGN_FPSTATE)) ? 1 : 0; >>> +} >>> + >> >> As Will pointed out, the expression before ? already evaluates to 1 or >> 0, so appending '? 1 : 0' is redundant. >> >>> void fpsimd_reload_fpstate(void) >>> { >>> preempt_disable(); >>> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c >>> index f10f2ba..5e34852 100644 >>> --- a/arch/arm64/kernel/kgdb.c >>> +++ b/arch/arm64/kernel/kgdb.c >>> @@ -23,6 +23,14 @@ >>> #include <linux/kdebug.h> >>> #include <linux/kgdb.h> >>> #include <asm/traps.h> >>> +#include <asm/fpsimd.h> >>> + >>> +#ifdef CONFIG_KERNEL_MODE_NEON >>> +/* >>> + * Structure to hold FP/SIMD register contents >>> + */ >>> +struct fpsimd_state kgdb_fpsimd_regs; >> >> 'static' if it's local to this source file >> > OK >>> +#endif >>> >>> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >>> { "x0", 8, offsetof(struct pt_regs, regs[0])}, >>> @@ -59,40 +67,40 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >>> { "sp", 8, offsetof(struct pt_regs, sp)}, >>> { "pc", 8, offsetof(struct pt_regs, pc)}, >>> { "pstate", 4, offsetof(struct pt_regs, pstate)}, >>> - { "v0", 16, -1 }, >>> - { "v1", 16, -1 }, >>> - { "v2", 16, -1 }, >>> - { "v3", 16, -1 }, >>> - { "v4", 16, -1 }, >>> - { "v5", 16, -1 }, >>> - { "v6", 16, -1 }, >>> - { "v7", 16, -1 }, >>> - { "v8", 16, -1 }, >>> - { "v9", 16, -1 }, >>> - { "v10", 16, -1 }, >>> - { "v11", 16, -1 }, >>> - { "v12", 16, -1 }, >>> - { "v13", 16, -1 }, >>> - { "v14", 16, -1 }, >>> - { "v15", 16, -1 }, >>> - { "v16", 16, -1 }, >>> - { "v17", 16, -1 }, >>> - { "v18", 16, -1 }, >>> - { "v19", 16, -1 }, >>> - { "v20", 16, -1 }, >>> - { "v21", 16, -1 }, >>> - { "v22", 16, -1 }, >>> - { "v23", 16, -1 }, >>> - { "v24", 16, -1 }, >>> - { "v25", 16, -1 }, >>> - { "v26", 16, -1 }, >>> - { "v27", 16, -1 }, >>> - { "v28", 16, -1 }, >>> - { "v29", 16, -1 }, >>> - { "v30", 16, -1 }, >>> - { "v31", 16, -1 }, >>> - { "fpsr", 4, -1 }, >>> - { "fpcr", 4, -1 }, >>> + { "v0", 16, offsetof(struct fpsimd_state, vregs[0])}, >>> + { "v1", 16, offsetof(struct fpsimd_state, vregs[1])}, >>> + { "v2", 16, offsetof(struct fpsimd_state, vregs[2])}, >>> + { "v3", 16, offsetof(struct fpsimd_state, vregs[3])}, >>> + { "v4", 16, offsetof(struct fpsimd_state, vregs[4])}, >>> + { "v5", 16, offsetof(struct fpsimd_state, vregs[5])}, >>> + { "v6", 16, offsetof(struct fpsimd_state, vregs[6])}, >>> + { "v7", 16, offsetof(struct fpsimd_state, vregs[7])}, >>> + { "v8", 16, offsetof(struct fpsimd_state, vregs[8])}, >>> + { "v9", 16, offsetof(struct fpsimd_state, vregs[9])}, >>> + { "v10", 16, offsetof(struct fpsimd_state, vregs[10])}, >>> + { "v11", 16, offsetof(struct fpsimd_state, vregs[11])}, >>> + { "v12", 16, offsetof(struct fpsimd_state, vregs[12])}, >>> + { "v13", 16, offsetof(struct fpsimd_state, vregs[13])}, >>> + { "v14", 16, offsetof(struct fpsimd_state, vregs[14])}, >>> + { "v15", 16, offsetof(struct fpsimd_state, vregs[15])}, >>> + { "v16", 16, offsetof(struct fpsimd_state, vregs[16])}, >>> + { "v17", 16, offsetof(struct fpsimd_state, vregs[17])}, >>> + { "v18", 16, offsetof(struct fpsimd_state, vregs[18])}, >>> + { "v19", 16, offsetof(struct fpsimd_state, vregs[19])}, >>> + { "v20", 16, offsetof(struct fpsimd_state, vregs[20])}, >>> + { "v21", 16, offsetof(struct fpsimd_state, vregs[21])}, >>> + { "v22", 16, offsetof(struct fpsimd_state, vregs[22])}, >>> + { "v23", 16, offsetof(struct fpsimd_state, vregs[23])}, >>> + { "v24", 16, offsetof(struct fpsimd_state, vregs[24])}, >>> + { "v25", 16, offsetof(struct fpsimd_state, vregs[25])}, >>> + { "v26", 16, offsetof(struct fpsimd_state, vregs[26])}, >>> + { "v27", 16, offsetof(struct fpsimd_state, vregs[27])}, >>> + { "v28", 16, offsetof(struct fpsimd_state, vregs[28])}, >>> + { "v29", 16, offsetof(struct fpsimd_state, vregs[29])}, >>> + { "v30", 16, offsetof(struct fpsimd_state, vregs[30])}, >>> + { "v31", 16, offsetof(struct fpsimd_state, vregs[31])}, >>> + { "fpsr", 4, offsetof(struct fpsimd_state, fpsr)}, >>> + { "fpcr", 4, offsetof(struct fpsimd_state, fpcr)}, >>> }; >>> >>> char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >>> @@ -100,11 +108,22 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >>> if (regno >= DBG_MAX_REG_NUM || regno < 0) >>> return NULL; >>> >>> - if (dbg_reg_def[regno].offset != -1) >>> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >>> memcpy(mem, (void *)regs + dbg_reg_def[regno].offset, >>> dbg_reg_def[regno].size); >>> - else >>> + else { >>> +#ifdef CONFIG_KERNEL_MODE_NEON >> >> else if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON)) { >> >>> + if (fpsimd_thread_state()) { >>> + fpsimd_save_state(&kgdb_fpsimd_regs); >>> + memcpy(mem, (void *)&kgdb_fpsimd_regs + >>> + dbg_reg_def[regno].offset, >>> + dbg_reg_def[regno].size); >> >> Why do this conditionally? Shouldn't kgdb return whatever is currently >> present in the register file, regardless of whether 'current' is a >> kernel thread or has had its FPSIMD state swapped out? >> Also, one could wonder why this is dependent on KERNEL_MODE_NEON being set >> > > KGDB, being kernel debugger it was thought that allow > debugging only if kernel mode is supported. > In fact, I proposed (previous email) to remove this condition and > allow to reading neon > registers always and allow write only if neon is in kernel mode. > I think KGDB is a special case here and should not considered as a user of kernel mode NEON. So even writing the registers should allowable regardless of this setting imo. > But completely removing this condition check and allowing to modify > is user choice to ensure neon registers state. > > Will, Please let me know your opinion? > >>> + } else >>> + memset(mem, 0, dbg_reg_def[regno].size); >>> +#else >>> memset(mem, 0, dbg_reg_def[regno].size); >>> +#endif >>> + } >>> return dbg_reg_def[regno].name; >>> } >>> >>> @@ -113,9 +132,19 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs) >>> if (regno >= DBG_MAX_REG_NUM || regno < 0) >>> return -EINVAL; >>> >>> - if (dbg_reg_def[regno].offset != -1) >>> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >>> memcpy((void *)regs + dbg_reg_def[regno].offset, mem, >>> - dbg_reg_def[regno].size); >>> + dbg_reg_def[regno].size); >>> +#ifdef CONFIG_KERNEL_MODE_NEON >>> + else { >>> + if (fpsimd_thread_state()) { >> >> Again, why the conditional? >> >>> + memcpy((void *)&kgdb_fpsimd_regs + >>> + dbg_reg_def[regno].offset, >>> + mem, dbg_reg_def[regno].size); >>> + fpsimd_load_state(&kgdb_fpsimd_regs); >> >> Are you sure it is safe to clobber all the registers here? Shouldn't >> you save the state first, do the memcpy() and then load? >> > yes it is safe. gdb read registers after every command so all registers > are saved first before they are modified > Does that imply that -after every command- the whole NEON state (32*16 bytes) is saved to kgdb_fpsimd_regs 32 times, each time to access the contents of a single register? -- Ard. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2013-11-29 9:27 ` Ard Biesheuvel @ 2013-11-29 9:57 ` Vijay Kilari 2014-02-28 11:08 ` Vijay Kilari 0 siblings, 1 reply; 13+ messages in thread From: Vijay Kilari @ 2013-11-29 9:57 UTC (permalink / raw) To: linux-arm-kernel On Fri, Nov 29, 2013 at 2:57 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 29 November 2013 10:20, Vijay Kilari <vijay.kilari@gmail.com> wrote: >> On Fri, Nov 29, 2013 at 2:23 PM, Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> On 18 November 2013 13:06, <vijay.kilari@gmail.com> wrote: >>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >>>> >>>> Add KGDB debug support for FP/SIMD processor.This support >>>> only debugging of FP/SIMD in kernel mode. >>>> With this patch one can view or set FP/SIMD registers in >>>> kernel context >>>> >>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >>>> --- >>>> arch/arm64/include/asm/fpsimd.h | 1 + >>>> arch/arm64/kernel/fpsimd.c | 5 ++ >>>> arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++-------------- >>>> 3 files changed, 73 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h >>>> index 609bc44..5039d6d 100644 >>>> --- a/arch/arm64/include/asm/fpsimd.h >>>> +++ b/arch/arm64/include/asm/fpsimd.h >>>> @@ -60,6 +60,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state); >>>> extern void fpsimd_thread_switch(struct task_struct *next); >>>> extern void fpsimd_flush_thread(void); >>>> extern void fpsimd_reload_fpstate(void); >>>> +extern int fpsimd_thread_state(void); >>>> >>>> #endif >>>> >>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >>>> index 5b13c17..27604c1 100644 >>>> --- a/arch/arm64/kernel/fpsimd.c >>>> +++ b/arch/arm64/kernel/fpsimd.c >>>> @@ -124,6 +124,11 @@ void fpsimd_flush_thread(void) >>>> set_thread_flag(TIF_FOREIGN_FPSTATE); >>>> } >>>> >>>> +int fpsimd_thread_state(void) >>> >>> Please use a meaningful name for the function. Currently, it returns >>> true iff 'current' is a non-kernel thread whose FPSIMD state is /not/ >>> present in the FPSIMD register file. You should choose a name that >>> reflects that purpose. >>> >> do you have any reason why only TIF_FOREIGN_FPSTATE flag is >> set only for non-kernel threads in kernel_neon_begin()? >> > > The purpose of the TIF_FOREIGN_FPSTATE flag is to convey that the > FPSIMD context should be restored before a task enters userland. A > kernel thread never enters userland by definition so the flag has no > meaning there. > >>>> +{ >>>> + return (current->mm && test_thread_flag(TIF_FOREIGN_FPSTATE)) ? 1 : 0; >>>> +} >>>> + >>> >>> As Will pointed out, the expression before ? already evaluates to 1 or >>> 0, so appending '? 1 : 0' is redundant. >>> >>>> void fpsimd_reload_fpstate(void) >>>> { >>>> preempt_disable(); >>>> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c >>>> index f10f2ba..5e34852 100644 >>>> --- a/arch/arm64/kernel/kgdb.c >>>> +++ b/arch/arm64/kernel/kgdb.c >>>> @@ -23,6 +23,14 @@ >>>> #include <linux/kdebug.h> >>>> #include <linux/kgdb.h> >>>> #include <asm/traps.h> >>>> +#include <asm/fpsimd.h> >>>> + >>>> +#ifdef CONFIG_KERNEL_MODE_NEON >>>> +/* >>>> + * Structure to hold FP/SIMD register contents >>>> + */ >>>> +struct fpsimd_state kgdb_fpsimd_regs; >>> >>> 'static' if it's local to this source file >>> >> OK >>>> +#endif >>>> >>>> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >>>> { "x0", 8, offsetof(struct pt_regs, regs[0])}, >>>> @@ -59,40 +67,40 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >>>> { "sp", 8, offsetof(struct pt_regs, sp)}, >>>> { "pc", 8, offsetof(struct pt_regs, pc)}, >>>> { "pstate", 4, offsetof(struct pt_regs, pstate)}, >>>> - { "v0", 16, -1 }, >>>> - { "v1", 16, -1 }, >>>> - { "v2", 16, -1 }, >>>> - { "v3", 16, -1 }, >>>> - { "v4", 16, -1 }, >>>> - { "v5", 16, -1 }, >>>> - { "v6", 16, -1 }, >>>> - { "v7", 16, -1 }, >>>> - { "v8", 16, -1 }, >>>> - { "v9", 16, -1 }, >>>> - { "v10", 16, -1 }, >>>> - { "v11", 16, -1 }, >>>> - { "v12", 16, -1 }, >>>> - { "v13", 16, -1 }, >>>> - { "v14", 16, -1 }, >>>> - { "v15", 16, -1 }, >>>> - { "v16", 16, -1 }, >>>> - { "v17", 16, -1 }, >>>> - { "v18", 16, -1 }, >>>> - { "v19", 16, -1 }, >>>> - { "v20", 16, -1 }, >>>> - { "v21", 16, -1 }, >>>> - { "v22", 16, -1 }, >>>> - { "v23", 16, -1 }, >>>> - { "v24", 16, -1 }, >>>> - { "v25", 16, -1 }, >>>> - { "v26", 16, -1 }, >>>> - { "v27", 16, -1 }, >>>> - { "v28", 16, -1 }, >>>> - { "v29", 16, -1 }, >>>> - { "v30", 16, -1 }, >>>> - { "v31", 16, -1 }, >>>> - { "fpsr", 4, -1 }, >>>> - { "fpcr", 4, -1 }, >>>> + { "v0", 16, offsetof(struct fpsimd_state, vregs[0])}, >>>> + { "v1", 16, offsetof(struct fpsimd_state, vregs[1])}, >>>> + { "v2", 16, offsetof(struct fpsimd_state, vregs[2])}, >>>> + { "v3", 16, offsetof(struct fpsimd_state, vregs[3])}, >>>> + { "v4", 16, offsetof(struct fpsimd_state, vregs[4])}, >>>> + { "v5", 16, offsetof(struct fpsimd_state, vregs[5])}, >>>> + { "v6", 16, offsetof(struct fpsimd_state, vregs[6])}, >>>> + { "v7", 16, offsetof(struct fpsimd_state, vregs[7])}, >>>> + { "v8", 16, offsetof(struct fpsimd_state, vregs[8])}, >>>> + { "v9", 16, offsetof(struct fpsimd_state, vregs[9])}, >>>> + { "v10", 16, offsetof(struct fpsimd_state, vregs[10])}, >>>> + { "v11", 16, offsetof(struct fpsimd_state, vregs[11])}, >>>> + { "v12", 16, offsetof(struct fpsimd_state, vregs[12])}, >>>> + { "v13", 16, offsetof(struct fpsimd_state, vregs[13])}, >>>> + { "v14", 16, offsetof(struct fpsimd_state, vregs[14])}, >>>> + { "v15", 16, offsetof(struct fpsimd_state, vregs[15])}, >>>> + { "v16", 16, offsetof(struct fpsimd_state, vregs[16])}, >>>> + { "v17", 16, offsetof(struct fpsimd_state, vregs[17])}, >>>> + { "v18", 16, offsetof(struct fpsimd_state, vregs[18])}, >>>> + { "v19", 16, offsetof(struct fpsimd_state, vregs[19])}, >>>> + { "v20", 16, offsetof(struct fpsimd_state, vregs[20])}, >>>> + { "v21", 16, offsetof(struct fpsimd_state, vregs[21])}, >>>> + { "v22", 16, offsetof(struct fpsimd_state, vregs[22])}, >>>> + { "v23", 16, offsetof(struct fpsimd_state, vregs[23])}, >>>> + { "v24", 16, offsetof(struct fpsimd_state, vregs[24])}, >>>> + { "v25", 16, offsetof(struct fpsimd_state, vregs[25])}, >>>> + { "v26", 16, offsetof(struct fpsimd_state, vregs[26])}, >>>> + { "v27", 16, offsetof(struct fpsimd_state, vregs[27])}, >>>> + { "v28", 16, offsetof(struct fpsimd_state, vregs[28])}, >>>> + { "v29", 16, offsetof(struct fpsimd_state, vregs[29])}, >>>> + { "v30", 16, offsetof(struct fpsimd_state, vregs[30])}, >>>> + { "v31", 16, offsetof(struct fpsimd_state, vregs[31])}, >>>> + { "fpsr", 4, offsetof(struct fpsimd_state, fpsr)}, >>>> + { "fpcr", 4, offsetof(struct fpsimd_state, fpcr)}, >>>> }; >>>> >>>> char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >>>> @@ -100,11 +108,22 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >>>> if (regno >= DBG_MAX_REG_NUM || regno < 0) >>>> return NULL; >>>> >>>> - if (dbg_reg_def[regno].offset != -1) >>>> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >>>> memcpy(mem, (void *)regs + dbg_reg_def[regno].offset, >>>> dbg_reg_def[regno].size); >>>> - else >>>> + else { >>>> +#ifdef CONFIG_KERNEL_MODE_NEON >>> >>> else if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON)) { >>> >>>> + if (fpsimd_thread_state()) { >>>> + fpsimd_save_state(&kgdb_fpsimd_regs); >>>> + memcpy(mem, (void *)&kgdb_fpsimd_regs + >>>> + dbg_reg_def[regno].offset, >>>> + dbg_reg_def[regno].size); >>> >>> Why do this conditionally? Shouldn't kgdb return whatever is currently >>> present in the register file, regardless of whether 'current' is a >>> kernel thread or has had its FPSIMD state swapped out? >>> Also, one could wonder why this is dependent on KERNEL_MODE_NEON being set >>> >> >> KGDB, being kernel debugger it was thought that allow >> debugging only if kernel mode is supported. >> In fact, I proposed (previous email) to remove this condition and >> allow to reading neon >> registers always and allow write only if neon is in kernel mode. >> > > I think KGDB is a special case here and should not considered as a > user of kernel mode NEON. So even writing the registers should > allowable regardless of this setting imo. > >> But completely removing this condition check and allowing to modify >> is user choice to ensure neon registers state. >> >> Will, Please let me know your opinion? >> >>>> + } else >>>> + memset(mem, 0, dbg_reg_def[regno].size); >>>> +#else >>>> memset(mem, 0, dbg_reg_def[regno].size); >>>> +#endif >>>> + } >>>> return dbg_reg_def[regno].name; >>>> } >>>> >>>> @@ -113,9 +132,19 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs) >>>> if (regno >= DBG_MAX_REG_NUM || regno < 0) >>>> return -EINVAL; >>>> >>>> - if (dbg_reg_def[regno].offset != -1) >>>> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >>>> memcpy((void *)regs + dbg_reg_def[regno].offset, mem, >>>> - dbg_reg_def[regno].size); >>>> + dbg_reg_def[regno].size); >>>> +#ifdef CONFIG_KERNEL_MODE_NEON >>>> + else { >>>> + if (fpsimd_thread_state()) { >>> >>> Again, why the conditional? >>> >>>> + memcpy((void *)&kgdb_fpsimd_regs + >>>> + dbg_reg_def[regno].offset, >>>> + mem, dbg_reg_def[regno].size); >>>> + fpsimd_load_state(&kgdb_fpsimd_regs); >>> >>> Are you sure it is safe to clobber all the registers here? Shouldn't >>> you save the state first, do the memcpy() and then load? >>> >> yes it is safe. gdb read registers after every command so all registers >> are saved first before they are modified >> > > Does that imply that -after every command- the whole NEON state (32*16 > bytes) is saved to kgdb_fpsimd_regs 32 times, each time to access the > contents of a single register? > Yes, I mentioned this overhead in cover letter. There is no single call to architecture specific code to read all the registers at once for gdb 'register read' command. The kgdb makes one call for every register read. I can think of saving this registers for every debug exception call, but that too is not optimized because gdb tool issues multiple step commands for every 'next' command. IMO, this little overhead is still ok as it is used only in step debugging > -- > Ard. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2013-11-29 9:57 ` Vijay Kilari @ 2014-02-28 11:08 ` Vijay Kilari 2014-02-28 17:36 ` Will Deacon 0 siblings, 1 reply; 13+ messages in thread From: Vijay Kilari @ 2014-02-28 11:08 UTC (permalink / raw) To: linux-arm-kernel Hi Will, On Fri, Nov 29, 2013 at 3:27 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote: > On Fri, Nov 29, 2013 at 2:57 PM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> On 29 November 2013 10:20, Vijay Kilari <vijay.kilari@gmail.com> wrote: >>> On Fri, Nov 29, 2013 at 2:23 PM, Ard Biesheuvel >>> <ard.biesheuvel@linaro.org> wrote: >>>> On 18 November 2013 13:06, <vijay.kilari@gmail.com> wrote: >>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >>>>> >>>>> Add KGDB debug support for FP/SIMD processor.This support >>>>> only debugging of FP/SIMD in kernel mode. >>>>> With this patch one can view or set FP/SIMD registers in >>>>> kernel context >>>>> >>>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >>>>> --- >>>>> arch/arm64/include/asm/fpsimd.h | 1 + >>>>> arch/arm64/kernel/fpsimd.c | 5 ++ >>>>> arch/arm64/kernel/kgdb.c | 105 +++++++++++++++++++++++++-------------- >>>>> 3 files changed, 73 insertions(+), 38 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h >>>>> index 609bc44..5039d6d 100644 >>>>> --- a/arch/arm64/include/asm/fpsimd.h >>>>> +++ b/arch/arm64/include/asm/fpsimd.h >>>>> @@ -60,6 +60,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state); >>>>> extern void fpsimd_thread_switch(struct task_struct *next); >>>>> extern void fpsimd_flush_thread(void); >>>>> extern void fpsimd_reload_fpstate(void); >>>>> +extern int fpsimd_thread_state(void); >>>>> >>>>> #endif >>>>> >>>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >>>>> index 5b13c17..27604c1 100644 >>>>> --- a/arch/arm64/kernel/fpsimd.c >>>>> +++ b/arch/arm64/kernel/fpsimd.c >>>>> @@ -124,6 +124,11 @@ void fpsimd_flush_thread(void) >>>>> set_thread_flag(TIF_FOREIGN_FPSTATE); >>>>> } >>>>> >>>>> +int fpsimd_thread_state(void) >>>> >>>> Please use a meaningful name for the function. Currently, it returns >>>> true iff 'current' is a non-kernel thread whose FPSIMD state is /not/ >>>> present in the FPSIMD register file. You should choose a name that >>>> reflects that purpose. >>>> >>> do you have any reason why only TIF_FOREIGN_FPSTATE flag is >>> set only for non-kernel threads in kernel_neon_begin()? >>> >> >> The purpose of the TIF_FOREIGN_FPSTATE flag is to convey that the >> FPSIMD context should be restored before a task enters userland. A >> kernel thread never enters userland by definition so the flag has no >> meaning there. >> >>>>> +{ >>>>> + return (current->mm && test_thread_flag(TIF_FOREIGN_FPSTATE)) ? 1 : 0; >>>>> +} >>>>> + >>>> >>>> As Will pointed out, the expression before ? already evaluates to 1 or >>>> 0, so appending '? 1 : 0' is redundant. >>>> >>>>> void fpsimd_reload_fpstate(void) >>>>> { >>>>> preempt_disable(); >>>>> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c >>>>> index f10f2ba..5e34852 100644 >>>>> --- a/arch/arm64/kernel/kgdb.c >>>>> +++ b/arch/arm64/kernel/kgdb.c >>>>> @@ -23,6 +23,14 @@ >>>>> #include <linux/kdebug.h> >>>>> #include <linux/kgdb.h> >>>>> #include <asm/traps.h> >>>>> +#include <asm/fpsimd.h> >>>>> + >>>>> +#ifdef CONFIG_KERNEL_MODE_NEON >>>>> +/* >>>>> + * Structure to hold FP/SIMD register contents >>>>> + */ >>>>> +struct fpsimd_state kgdb_fpsimd_regs; >>>> >>>> 'static' if it's local to this source file >>>> >>> OK >>>>> +#endif >>>>> >>>>> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >>>>> { "x0", 8, offsetof(struct pt_regs, regs[0])}, >>>>> @@ -59,40 +67,40 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { >>>>> { "sp", 8, offsetof(struct pt_regs, sp)}, >>>>> { "pc", 8, offsetof(struct pt_regs, pc)}, >>>>> { "pstate", 4, offsetof(struct pt_regs, pstate)}, >>>>> - { "v0", 16, -1 }, >>>>> - { "v1", 16, -1 }, >>>>> - { "v2", 16, -1 }, >>>>> - { "v3", 16, -1 }, >>>>> - { "v4", 16, -1 }, >>>>> - { "v5", 16, -1 }, >>>>> - { "v6", 16, -1 }, >>>>> - { "v7", 16, -1 }, >>>>> - { "v8", 16, -1 }, >>>>> - { "v9", 16, -1 }, >>>>> - { "v10", 16, -1 }, >>>>> - { "v11", 16, -1 }, >>>>> - { "v12", 16, -1 }, >>>>> - { "v13", 16, -1 }, >>>>> - { "v14", 16, -1 }, >>>>> - { "v15", 16, -1 }, >>>>> - { "v16", 16, -1 }, >>>>> - { "v17", 16, -1 }, >>>>> - { "v18", 16, -1 }, >>>>> - { "v19", 16, -1 }, >>>>> - { "v20", 16, -1 }, >>>>> - { "v21", 16, -1 }, >>>>> - { "v22", 16, -1 }, >>>>> - { "v23", 16, -1 }, >>>>> - { "v24", 16, -1 }, >>>>> - { "v25", 16, -1 }, >>>>> - { "v26", 16, -1 }, >>>>> - { "v27", 16, -1 }, >>>>> - { "v28", 16, -1 }, >>>>> - { "v29", 16, -1 }, >>>>> - { "v30", 16, -1 }, >>>>> - { "v31", 16, -1 }, >>>>> - { "fpsr", 4, -1 }, >>>>> - { "fpcr", 4, -1 }, >>>>> + { "v0", 16, offsetof(struct fpsimd_state, vregs[0])}, >>>>> + { "v1", 16, offsetof(struct fpsimd_state, vregs[1])}, >>>>> + { "v2", 16, offsetof(struct fpsimd_state, vregs[2])}, >>>>> + { "v3", 16, offsetof(struct fpsimd_state, vregs[3])}, >>>>> + { "v4", 16, offsetof(struct fpsimd_state, vregs[4])}, >>>>> + { "v5", 16, offsetof(struct fpsimd_state, vregs[5])}, >>>>> + { "v6", 16, offsetof(struct fpsimd_state, vregs[6])}, >>>>> + { "v7", 16, offsetof(struct fpsimd_state, vregs[7])}, >>>>> + { "v8", 16, offsetof(struct fpsimd_state, vregs[8])}, >>>>> + { "v9", 16, offsetof(struct fpsimd_state, vregs[9])}, >>>>> + { "v10", 16, offsetof(struct fpsimd_state, vregs[10])}, >>>>> + { "v11", 16, offsetof(struct fpsimd_state, vregs[11])}, >>>>> + { "v12", 16, offsetof(struct fpsimd_state, vregs[12])}, >>>>> + { "v13", 16, offsetof(struct fpsimd_state, vregs[13])}, >>>>> + { "v14", 16, offsetof(struct fpsimd_state, vregs[14])}, >>>>> + { "v15", 16, offsetof(struct fpsimd_state, vregs[15])}, >>>>> + { "v16", 16, offsetof(struct fpsimd_state, vregs[16])}, >>>>> + { "v17", 16, offsetof(struct fpsimd_state, vregs[17])}, >>>>> + { "v18", 16, offsetof(struct fpsimd_state, vregs[18])}, >>>>> + { "v19", 16, offsetof(struct fpsimd_state, vregs[19])}, >>>>> + { "v20", 16, offsetof(struct fpsimd_state, vregs[20])}, >>>>> + { "v21", 16, offsetof(struct fpsimd_state, vregs[21])}, >>>>> + { "v22", 16, offsetof(struct fpsimd_state, vregs[22])}, >>>>> + { "v23", 16, offsetof(struct fpsimd_state, vregs[23])}, >>>>> + { "v24", 16, offsetof(struct fpsimd_state, vregs[24])}, >>>>> + { "v25", 16, offsetof(struct fpsimd_state, vregs[25])}, >>>>> + { "v26", 16, offsetof(struct fpsimd_state, vregs[26])}, >>>>> + { "v27", 16, offsetof(struct fpsimd_state, vregs[27])}, >>>>> + { "v28", 16, offsetof(struct fpsimd_state, vregs[28])}, >>>>> + { "v29", 16, offsetof(struct fpsimd_state, vregs[29])}, >>>>> + { "v30", 16, offsetof(struct fpsimd_state, vregs[30])}, >>>>> + { "v31", 16, offsetof(struct fpsimd_state, vregs[31])}, >>>>> + { "fpsr", 4, offsetof(struct fpsimd_state, fpsr)}, >>>>> + { "fpcr", 4, offsetof(struct fpsimd_state, fpcr)}, >>>>> }; >>>>> >>>>> char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >>>>> @@ -100,11 +108,22 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) >>>>> if (regno >= DBG_MAX_REG_NUM || regno < 0) >>>>> return NULL; >>>>> >>>>> - if (dbg_reg_def[regno].offset != -1) >>>>> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >>>>> memcpy(mem, (void *)regs + dbg_reg_def[regno].offset, >>>>> dbg_reg_def[regno].size); >>>>> - else >>>>> + else { >>>>> +#ifdef CONFIG_KERNEL_MODE_NEON >>>> >>>> else if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON)) { >>>> >>>>> + if (fpsimd_thread_state()) { >>>>> + fpsimd_save_state(&kgdb_fpsimd_regs); >>>>> + memcpy(mem, (void *)&kgdb_fpsimd_regs + >>>>> + dbg_reg_def[regno].offset, >>>>> + dbg_reg_def[regno].size); >>>> >>>> Why do this conditionally? Shouldn't kgdb return whatever is currently >>>> present in the register file, regardless of whether 'current' is a >>>> kernel thread or has had its FPSIMD state swapped out? >>>> Also, one could wonder why this is dependent on KERNEL_MODE_NEON being set >>>> >>> >>> KGDB, being kernel debugger it was thought that allow >>> debugging only if kernel mode is supported. >>> In fact, I proposed (previous email) to remove this condition and >>> allow to reading neon >>> registers always and allow write only if neon is in kernel mode. >>> >> >> I think KGDB is a special case here and should not considered as a >> user of kernel mode NEON. So even writing the registers should >> allowable regardless of this setting imo. >> Can we avoid checking on NEON state and allow debugger to access NEON registers irrespective of state to keep it simple? and just compile config is enough? >>> But completely removing this condition check and allowing to modify >>> is user choice to ensure neon registers state. >>> >>> Will, Please let me know your opinion? >>> >>>>> + } else >>>>> + memset(mem, 0, dbg_reg_def[regno].size); >>>>> +#else >>>>> memset(mem, 0, dbg_reg_def[regno].size); >>>>> +#endif >>>>> + } >>>>> return dbg_reg_def[regno].name; >>>>> } >>>>> >>>>> @@ -113,9 +132,19 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs) >>>>> if (regno >= DBG_MAX_REG_NUM || regno < 0) >>>>> return -EINVAL; >>>>> >>>>> - if (dbg_reg_def[regno].offset != -1) >>>>> + if (dbg_reg_def[regno].offset != -1 && regno < _GP_REGS) >>>>> memcpy((void *)regs + dbg_reg_def[regno].offset, mem, >>>>> - dbg_reg_def[regno].size); >>>>> + dbg_reg_def[regno].size); >>>>> +#ifdef CONFIG_KERNEL_MODE_NEON >>>>> + else { >>>>> + if (fpsimd_thread_state()) { >>>> >>>> Again, why the conditional? >>>> >>>>> + memcpy((void *)&kgdb_fpsimd_regs + >>>>> + dbg_reg_def[regno].offset, >>>>> + mem, dbg_reg_def[regno].size); >>>>> + fpsimd_load_state(&kgdb_fpsimd_regs); >>>> >>>> Are you sure it is safe to clobber all the registers here? Shouldn't >>>> you save the state first, do the memcpy() and then load? >>>> >>> yes it is safe. gdb read registers after every command so all registers >>> are saved first before they are modified >>> >> >> Does that imply that -after every command- the whole NEON state (32*16 >> bytes) is saved to kgdb_fpsimd_regs 32 times, each time to access the >> contents of a single register? >> > Yes, I mentioned this overhead in cover letter. There is no single call to > architecture specific code to read all the registers at once for gdb > 'register read' > command. The kgdb makes one call for every register read. > > I can think of saving this registers for every debug exception call, > but that too is not optimized because gdb tool issues multiple step commands > for every 'next' command. > IMO, this little overhead is still ok as it is used only in step debugging > >> -- >> Ard. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2014-02-28 11:08 ` Vijay Kilari @ 2014-02-28 17:36 ` Will Deacon 2014-03-02 13:08 ` Ard Biesheuvel 0 siblings, 1 reply; 13+ messages in thread From: Will Deacon @ 2014-02-28 17:36 UTC (permalink / raw) To: linux-arm-kernel On Fri, Feb 28, 2014 at 11:08:58AM +0000, Vijay Kilari wrote: > On Fri, Nov 29, 2013 at 3:27 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote: > >>> KGDB, being kernel debugger it was thought that allow > >>> debugging only if kernel mode is supported. > >>> In fact, I proposed (previous email) to remove this condition and > >>> allow to reading neon > >>> registers always and allow write only if neon is in kernel mode. > >>> > >> > >> I think KGDB is a special case here and should not considered as a > >> user of kernel mode NEON. So even writing the registers should > >> allowable regardless of this setting imo. > >> > > Can we avoid checking on NEON state and allow debugger to > access NEON registers irrespective of state to keep it simple? > and just compile config is enough? I'm afraid I don't follow... I think we *do* need to do the relevant flush/sync operations, I just don't think it should be predicated on KERNEL_MODE_NEON. Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2014-02-28 17:36 ` Will Deacon @ 2014-03-02 13:08 ` Ard Biesheuvel 2014-03-03 11:43 ` Will Deacon 0 siblings, 1 reply; 13+ messages in thread From: Ard Biesheuvel @ 2014-03-02 13:08 UTC (permalink / raw) To: linux-arm-kernel On 28 February 2014 18:36, Will Deacon <will.deacon@arm.com> wrote: > On Fri, Feb 28, 2014 at 11:08:58AM +0000, Vijay Kilari wrote: >> On Fri, Nov 29, 2013 at 3:27 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote: >> >>> KGDB, being kernel debugger it was thought that allow >> >>> debugging only if kernel mode is supported. >> >>> In fact, I proposed (previous email) to remove this condition and >> >>> allow to reading neon >> >>> registers always and allow write only if neon is in kernel mode. >> >>> >> >> >> >> I think KGDB is a special case here and should not considered as a >> >> user of kernel mode NEON. So even writing the registers should >> >> allowable regardless of this setting imo. >> >> >> >> Can we avoid checking on NEON state and allow debugger to >> access NEON registers irrespective of state to keep it simple? >> and just compile config is enough? > > I'm afraid I don't follow... I think we *do* need to do the relevant > flush/sync operations, I just don't think it should be predicated on > KERNEL_MODE_NEON. Could you elaborate a bit on what you consider flush/sync operations? The point I was making is that, from kgdb's point of view, it is irrelevant whether the FPSIMD registers contain current's userland FPSIMD state or something else. kgdb should return and/or manipulate what the kernel itself sees when it accesses the FPSIMD registers directly, regardless of whether those contents belong to current's userland or not. (please refer to my pending series on kernel mode NEON optimizations) CONFIG_KERNEL_MODE_NEON is a 'def_bool y' on arm64, so whether it is more correct to depend on it or not is not that relevant. Regards, Ard. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD debug support 2014-03-02 13:08 ` Ard Biesheuvel @ 2014-03-03 11:43 ` Will Deacon 0 siblings, 0 replies; 13+ messages in thread From: Will Deacon @ 2014-03-03 11:43 UTC (permalink / raw) To: linux-arm-kernel On Sun, Mar 02, 2014 at 01:08:56PM +0000, Ard Biesheuvel wrote: > On 28 February 2014 18:36, Will Deacon <will.deacon@arm.com> wrote: > > On Fri, Feb 28, 2014 at 11:08:58AM +0000, Vijay Kilari wrote: > >> On Fri, Nov 29, 2013 at 3:27 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote: > >> >>> KGDB, being kernel debugger it was thought that allow > >> >>> debugging only if kernel mode is supported. > >> >>> In fact, I proposed (previous email) to remove this condition and > >> >>> allow to reading neon > >> >>> registers always and allow write only if neon is in kernel mode. > >> >>> > >> >> > >> >> I think KGDB is a special case here and should not considered as a > >> >> user of kernel mode NEON. So even writing the registers should > >> >> allowable regardless of this setting imo. > >> >> > >> > >> Can we avoid checking on NEON state and allow debugger to > >> access NEON registers irrespective of state to keep it simple? > >> and just compile config is enough? > > > > I'm afraid I don't follow... I think we *do* need to do the relevant > > flush/sync operations, I just don't think it should be predicated on > > KERNEL_MODE_NEON. > > Could you elaborate a bit on what you consider flush/sync operations? > > The point I was making is that, from kgdb's point of view, it is > irrelevant whether the FPSIMD registers contain current's userland > FPSIMD state or something else. kgdb should return and/or manipulate > what the kernel itself sees when it accesses the FPSIMD registers > directly, regardless of whether those contents belong to current's > userland or not. (please refer to my pending series on kernel mode > NEON optimizations) Ok, point taken. So we just allow KGDB to access the hardware directly, without any saving/restoring of state. Will ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-03-03 11:43 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-18 12:06 [RFC v2 PATCH 0/1] ARM64: KGDB: Add FPSIMD debug support vijay.kilari at gmail.com 2013-11-18 12:06 ` [RFC v2 PATCH 1/1] ARM64: KGDB: Add FP/SIMD " vijay.kilari at gmail.com 2013-11-25 18:52 ` Will Deacon 2013-11-25 19:03 ` Ard Biesheuvel 2013-11-26 11:57 ` Vijay Kilari 2013-11-29 8:53 ` Ard Biesheuvel 2013-11-29 9:20 ` Vijay Kilari 2013-11-29 9:27 ` Ard Biesheuvel 2013-11-29 9:57 ` Vijay Kilari 2014-02-28 11:08 ` Vijay Kilari 2014-02-28 17:36 ` Will Deacon 2014-03-02 13:08 ` Ard Biesheuvel 2014-03-03 11:43 ` Will Deacon
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).