Re: [PATCH v6 22/36] nds32: Debugging support

[Arnd Bergmann]

On Tue, Jan 23, 2018 at 8:28 AM, Vincent Chen  wrote:
> 2018-01-18 18:37 GMT+08:00 Arnd Bergmann :
>> On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu  wrote:
>>> From: Greentime Hu 

>>
>> It appears that you are implementing the old-style ptrace handling
>> with architecture specific commands. Please have a look at how
>> this is done in risc-v or arm64. If this takes more too much time
>> to address, I'd suggest using an empty stub function for sys_ptrace
>> and adding it back at a later point, but not send the current version
>> upstream.
>>
>
> After referring to risc-v and arm64, I realize that PTRACE_GETREGSET
> and PTRACE_SETREGSET is used to replace arch specific command.
> The needed port for the two ptrace commands had done in current
> version patch.
>
> Could I keep them and just removing the code for old-style ptrace
> handling in the next version patch?

The important part is to not merge a user space interface into the upstream
kernel that we still want to change. It's clear that it takes some time to
update gdb and other programs using the ptrace interface, so I'd suggest
to simply not have any ptrace interface submitted for inclusion until that
is complete.

In the meantime, you can keep the existing version as an add-on kernel
patch, you probably have other patches that are not ready to get merged
yet, so just keep this one in the same tree as the others.

 Arnd

Re: [PATCH v6 22/36] nds32: Debugging support

[Arnd Bergmann]

On Tue, Jan 23, 2018 at 8:28 AM, Vincent Chen  wrote:
> 2018-01-18 18:37 GMT+08:00 Arnd Bergmann :
>> On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu  wrote:
>>> From: Greentime Hu 

>>
>> It appears that you are implementing the old-style ptrace handling
>> with architecture specific commands. Please have a look at how
>> this is done in risc-v or arm64. If this takes more too much time
>> to address, I'd suggest using an empty stub function for sys_ptrace
>> and adding it back at a later point, but not send the current version
>> upstream.
>>
>
> After referring to risc-v and arm64, I realize that PTRACE_GETREGSET
> and PTRACE_SETREGSET is used to replace arch specific command.
> The needed port for the two ptrace commands had done in current
> version patch.
>
> Could I keep them and just removing the code for old-style ptrace
> handling in the next version patch?

The important part is to not merge a user space interface into the upstream
kernel that we still want to change. It's clear that it takes some time to
update gdb and other programs using the ptrace interface, so I'd suggest
to simply not have any ptrace interface submitted for inclusion until that
is complete.

In the meantime, you can keep the existing version as an add-on kernel
patch, you probably have other patches that are not ready to get merged
yet, so just keep this one in the same tree as the others.

 Arnd

Re: [PATCH v6 22/36] nds32: Debugging support

[Vincent Chen]

2018-01-18 18:37 GMT+08:00 Arnd Bergmann :
> On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu  wrote:
>> From: Greentime Hu 
>>
>> This patch adds ptrace support.
>>
>> Signed-off-by: Vincent Chen 
>> Signed-off-by: Greentime Hu 
>
> I must have missed this patch earlier, unfortunately I don't think
> this is ready:
>
>> +long arch_ptrace(struct task_struct *child, long request, unsigned long 
>> addr,
>> +unsigned long data)
>> +{
>> +   int ret;
>> +
>> +   switch (request) {
>> +   case PTRACE_PEEKUSR:
>> +   ret =
>> +   ptrace_read_user(child, addr, (unsigned long __user 
>> *)data);
>> +   break;
>> +
>> +   case PTRACE_POKEUSR:
>> +   ret = ptrace_write_user(child, addr, data);
>> +   break;
>> +
>> +   case PTRACE_GETREGS:
>> +   ret = ptrace_getregs(child, (void __user *)data);
>> +   break;
>> +
>> +   case PTRACE_SETREGS:
>> +   ret = ptrace_setregs(child, (void __user *)data);
>> +   break;
>> +
>> +   case PTRACE_GETFPREGS:
>> +   ret = ptrace_getfpregs(child, (void __user *)data);
>> +   break;
>> +
>> +   case PTRACE_SETFPREGS:
>> +   ret = ptrace_setfpregs(child, (void __user *)data);
>> +   break;
>> +
>> +   default:
>> +   ret = ptrace_request(child, request, addr, data);
>> +   break;
>> +   }
>> +
>> +   return ret;
>> +}
>
> It appears that you are implementing the old-style ptrace handling
> with architecture specific commands. Please have a look at how
> this is done in risc-v or arm64. If this takes more too much time
> to address, I'd suggest using an empty stub function for sys_ptrace
> and adding it back at a later point, but not send the current version
> upstream.
>
>  Arnd

Dear Arnd:

Thanks for your comments.

After referring to risc-v and arm64, I realize that PTRACE_GETREGSET
and PTRACE_SETREGSET is used to replace arch specific command.
The needed port for the two ptrace commands had done in current
version patch.

Could I keep them and just removing the code for old-style ptrace
handling in the next version patch?

Thanks

Vincent

Re: [PATCH v6 22/36] nds32: Debugging support

[Vincent Chen]

2018-01-18 18:37 GMT+08:00 Arnd Bergmann :
> On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu  wrote:
>> From: Greentime Hu 
>>
>> This patch adds ptrace support.
>>
>> Signed-off-by: Vincent Chen 
>> Signed-off-by: Greentime Hu 
>
> I must have missed this patch earlier, unfortunately I don't think
> this is ready:
>
>> +long arch_ptrace(struct task_struct *child, long request, unsigned long 
>> addr,
>> +unsigned long data)
>> +{
>> +   int ret;
>> +
>> +   switch (request) {
>> +   case PTRACE_PEEKUSR:
>> +   ret =
>> +   ptrace_read_user(child, addr, (unsigned long __user 
>> *)data);
>> +   break;
>> +
>> +   case PTRACE_POKEUSR:
>> +   ret = ptrace_write_user(child, addr, data);
>> +   break;
>> +
>> +   case PTRACE_GETREGS:
>> +   ret = ptrace_getregs(child, (void __user *)data);
>> +   break;
>> +
>> +   case PTRACE_SETREGS:
>> +   ret = ptrace_setregs(child, (void __user *)data);
>> +   break;
>> +
>> +   case PTRACE_GETFPREGS:
>> +   ret = ptrace_getfpregs(child, (void __user *)data);
>> +   break;
>> +
>> +   case PTRACE_SETFPREGS:
>> +   ret = ptrace_setfpregs(child, (void __user *)data);
>> +   break;
>> +
>> +   default:
>> +   ret = ptrace_request(child, request, addr, data);
>> +   break;
>> +   }
>> +
>> +   return ret;
>> +}
>
> It appears that you are implementing the old-style ptrace handling
> with architecture specific commands. Please have a look at how
> this is done in risc-v or arm64. If this takes more too much time
> to address, I'd suggest using an empty stub function for sys_ptrace
> and adding it back at a later point, but not send the current version
> upstream.
>
>  Arnd

Dear Arnd:

Thanks for your comments.

After referring to risc-v and arm64, I realize that PTRACE_GETREGSET
and PTRACE_SETREGSET is used to replace arch specific command.
The needed port for the two ptrace commands had done in current
version patch.

Could I keep them and just removing the code for old-style ptrace
handling in the next version patch?

Thanks

Vincent

Re: [PATCH v6 22/36] nds32: Debugging support

[Arnd Bergmann]

On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu  wrote:
> From: Greentime Hu 
>
> This patch adds ptrace support.
>
> Signed-off-by: Vincent Chen 
> Signed-off-by: Greentime Hu 

I must have missed this patch earlier, unfortunately I don't think
this is ready:

> +long arch_ptrace(struct task_struct *child, long request, unsigned long addr,
> +unsigned long data)
> +{
> +   int ret;
> +
> +   switch (request) {
> +   case PTRACE_PEEKUSR:
> +   ret =
> +   ptrace_read_user(child, addr, (unsigned long __user 
> *)data);
> +   break;
> +
> +   case PTRACE_POKEUSR:
> +   ret = ptrace_write_user(child, addr, data);
> +   break;
> +
> +   case PTRACE_GETREGS:
> +   ret = ptrace_getregs(child, (void __user *)data);
> +   break;
> +
> +   case PTRACE_SETREGS:
> +   ret = ptrace_setregs(child, (void __user *)data);
> +   break;
> +
> +   case PTRACE_GETFPREGS:
> +   ret = ptrace_getfpregs(child, (void __user *)data);
> +   break;
> +
> +   case PTRACE_SETFPREGS:
> +   ret = ptrace_setfpregs(child, (void __user *)data);
> +   break;
> +
> +   default:
> +   ret = ptrace_request(child, request, addr, data);
> +   break;
> +   }
> +
> +   return ret;
> +}

It appears that you are implementing the old-style ptrace handling
with architecture specific commands. Please have a look at how
this is done in risc-v or arm64. If this takes more too much time
to address, I'd suggest using an empty stub function for sys_ptrace
and adding it back at a later point, but not send the current version
upstream.

 Arnd

Re: [PATCH v6 22/36] nds32: Debugging support

[Arnd Bergmann]

On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu  wrote:
> From: Greentime Hu 
>
> This patch adds ptrace support.
>
> Signed-off-by: Vincent Chen 
> Signed-off-by: Greentime Hu 

I must have missed this patch earlier, unfortunately I don't think
this is ready:

> +long arch_ptrace(struct task_struct *child, long request, unsigned long addr,
> +unsigned long data)
> +{
> +   int ret;
> +
> +   switch (request) {
> +   case PTRACE_PEEKUSR:
> +   ret =
> +   ptrace_read_user(child, addr, (unsigned long __user 
> *)data);
> +   break;
> +
> +   case PTRACE_POKEUSR:
> +   ret = ptrace_write_user(child, addr, data);
> +   break;
> +
> +   case PTRACE_GETREGS:
> +   ret = ptrace_getregs(child, (void __user *)data);
> +   break;
> +
> +   case PTRACE_SETREGS:
> +   ret = ptrace_setregs(child, (void __user *)data);
> +   break;
> +
> +   case PTRACE_GETFPREGS:
> +   ret = ptrace_getfpregs(child, (void __user *)data);
> +   break;
> +
> +   case PTRACE_SETFPREGS:
> +   ret = ptrace_setfpregs(child, (void __user *)data);
> +   break;
> +
> +   default:
> +   ret = ptrace_request(child, request, addr, data);
> +   break;
> +   }
> +
> +   return ret;
> +}

It appears that you are implementing the old-style ptrace handling
with architecture specific commands. Please have a look at how
this is done in risc-v or arm64. If this takes more too much time
to address, I'd suggest using an empty stub function for sys_ptrace
and adding it back at a later point, but not send the current version
upstream.

 Arnd