[RFC v1 PATCH] ARM64: KGDB: Add FPSIMD debug support

[RFC v1 PATCH] ARM64: KGDB: Add FPSIMD debug support

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Based on ARM64 KGDB support patches, KGDB support for
FPSIMD is added. Only debugging of FPSIMD is kernel context
is supported.

This patch also requires Ard's patches where in kernel support
for NEON is added. 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

Tested on ARM64 simulator

Vijaya Kumar K (1):
  ARM64: KGDB: Add FP/SIMD debug support

 arch/arm64/kernel/kgdb.c |   98 ++++++++++++++++++++++++++++------------------
 1 file changed, 60 insertions(+), 38 deletions(-)

-- 
1.7.9.5

[RFC v1 PATCH] ARM64: KGDB: Add FP/SIMD debug support

[vijay.kilari at gmail.com]

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/kernel/kgdb.c |   98 ++++++++++++++++++++++++++++------------------
 1 file changed, 60 insertions(+), 38 deletions(-)

diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index f10f2ba..07d3d00 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,18 @@ 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
+		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);
+#endif
+	}
 	return dbg_reg_def[regno].name;
 }
 
@@ -113,9 +128,16 @@ 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 {
+		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

[RFC v1 PATCH] ARM64: KGDB: Add FP/SIMD debug support

[Will Deacon]

On Thu, Nov 07, 2013 at 12:38:41PM +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/kernel/kgdb.c |   98 ++++++++++++++++++++++++++++------------------
>  1 file changed, 60 insertions(+), 38 deletions(-)

[...]

> @@ -100,11 +108,18 @@ 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
> +		fpsimd_save_state(&kgdb_fpsimd_regs);
> +		memcpy(mem, (void *)&kgdb_fpsimd_regs +
> +			dbg_reg_def[regno].offset, dbg_reg_def[regno].size);

Don't you only want to do this if you're inside a kernel_neon_{begin,end}
section? Otherwise, you'll be copying out user registers. It really depends
on what semantics you want.

Will

[RFC v1 PATCH] ARM64: KGDB: Add FP/SIMD debug support

[Vijay Kilari]

On Fri, Nov 8, 2013 at 8:36 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Nov 07, 2013 at 12:38:41PM +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/kernel/kgdb.c |   98 ++++++++++++++++++++++++++++------------------
>>  1 file changed, 60 insertions(+), 38 deletions(-)
>
> [...]
>
>> @@ -100,11 +108,18 @@ 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
>> +             fpsimd_save_state(&kgdb_fpsimd_regs);
>> +             memcpy(mem, (void *)&kgdb_fpsimd_regs +
>> +                     dbg_reg_def[regno].offset, dbg_reg_def[regno].size);
>
> Don't you only want to do this if you're inside a kernel_neon_{begin,end}
> section? Otherwise, you'll be copying out user registers. It really depends
> on what semantics you want.
>
Yes, we can do that. I couldn't find any api to know the state of the neon.
I think of using a status variable and update in kernel_neon_begin/end?

> Will

[RFC v1 PATCH] ARM64: KGDB: Add FP/SIMD debug support

[Ard Biesheuvel]

On 11 November 2013 09:47, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Fri, Nov 8, 2013 at 8:36 PM, Will Deacon <will.deacon@arm.com> wrote:
>> On Thu, Nov 07, 2013 at 12:38:41PM +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/kernel/kgdb.c |   98 ++++++++++++++++++++++++++++------------------
>>>  1 file changed, 60 insertions(+), 38 deletions(-)
>>
>> [...]
>>
>>> @@ -100,11 +108,18 @@ 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
>>> +             fpsimd_save_state(&kgdb_fpsimd_regs);
>>> +             memcpy(mem, (void *)&kgdb_fpsimd_regs +
>>> +                     dbg_reg_def[regno].offset, dbg_reg_def[regno].size);
>>
>> Don't you only want to do this if you're inside a kernel_neon_{begin,end}
>> section? Otherwise, you'll be copying out user registers. It really depends
>> on what semantics you want.
>>
> Yes, we can do that. I couldn't find any api to know the state of the neon.
> I think of using a status variable and update in kernel_neon_begin/end?
>

I have posted a patch here

http://marc.info/?l=linux-arm-kernel&m=138324498203157&w=2

that already keeps track of whether the current NEON register contents
belong to the current userland or not.
This is for a different purpose (lazy restore) but could potentially
be reused here I suppose.

-- 
Ard.