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(-)
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; }
On Mon, Nov 18, 2013 at 12:06:57PM +0000, 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) +{
- 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);} elsememset(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
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@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 : 0' doesn't do anything
- TIF_FOREIGN_FPSTATE doesn't seem to appear in my source tree (3.13-rc1)
- 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.
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@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 : 0' doesn't do anything
it worked for me. on true condition it returned 1 else 0. Any thing specific?
- 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.
- 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);} elsememset(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
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
} elsememset(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
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?
} elsememset(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
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?
} elsememset(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?
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?
} elsememset(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.
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?
} elsememset(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.
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
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.
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
linaro-kernel@lists.linaro.org
-
Ard Biesheuvel -
Vijay Kilari -
vijay.kilari＠gmail.com
-
Will Deacon