[PATCH] csky: Fix a size determination in gpr_get()

[PATCH] csky: Fix a size determination in gpr_get()

[Zhenzhong Duan]

"*" is missed  in size determination as we are passing register set
rather than a pointer.

Fixes: dcad7854fcce ("sky: switch to ->regset_get()")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
---
 arch/csky/kernel/ptrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/csky/kernel/ptrace.c b/arch/csky/kernel/ptrace.c
index d822144906ac..a4cf2e2ac15a 100644
--- a/arch/csky/kernel/ptrace.c
+++ b/arch/csky/kernel/ptrace.c
@@ -83,7 +83,7 @@ static int gpr_get(struct task_struct *target,
 	/* Abiv1 regs->tls is fake and we need sync here. */
 	regs->tls = task_thread_info(target)->tp_value;
 
-	return membuf_write(&to, regs, sizeof(regs));
+	return membuf_write(&to, regs, sizeof(*regs));
 }
 
 static int gpr_set(struct task_struct *target,
-- 
2.25.1

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Al Viro]

On Tue, Sep 22, 2020 at 05:15:05PM +0800, Zhenzhong Duan wrote:
> "*" is missed  in size determination as we are passing register set
> rather than a pointer.

Ack.  I can push it to Linus today, unless you want it to go through
csky tree.  Preferences?

> Fixes: dcad7854fcce ("sky: switch to ->regset_get()")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
> ---
>  arch/csky/kernel/ptrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/csky/kernel/ptrace.c b/arch/csky/kernel/ptrace.c
> index d822144906ac..a4cf2e2ac15a 100644
> --- a/arch/csky/kernel/ptrace.c
> +++ b/arch/csky/kernel/ptrace.c
> @@ -83,7 +83,7 @@ static int gpr_get(struct task_struct *target,
>  	/* Abiv1 regs->tls is fake and we need sync here. */
>  	regs->tls = task_thread_info(target)->tp_value;
>  
> -	return membuf_write(&to, regs, sizeof(regs));
> +	return membuf_write(&to, regs, sizeof(*regs));
>  }
>  
>  static int gpr_set(struct task_struct *target,
> -- 
> 2.25.1
> 

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Guo Ren]

Thx Duan,

Acked-by: Guo Ren <guoren@kernel.org>

Hi AI,

I found the broken commit still has a question:

> commit dcad7854fcce6a2d49b6a3ead5bbefeff047e559
> Author: Al Viro <viro@zeniv.linux.org.uk>
> Date:   Tue Jun 16 15:28:29 2020 -0400

>    csky: switch to ->regset_get()

>    NB: WTF "- what the fuck :(" is fpregs_get() playing at???
The fpregs_get() is for REGSET_FPR regset used by ptrace (gdb) and all
fp regs are stored in threads' context.
So, WTF question for?

Best Regards
 Guo Ren

On Wed, Sep 23, 2020 at 12:29 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Sep 22, 2020 at 05:15:05PM +0800, Zhenzhong Duan wrote:
> > "*" is missed  in size determination as we are passing register set
> > rather than a pointer.
>
> Ack.  I can push it to Linus today, unless you want it to go through
> csky tree.  Preferences?
>
> > Fixes: dcad7854fcce ("sky: switch to ->regset_get()")
> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
> > ---
> >  arch/csky/kernel/ptrace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/csky/kernel/ptrace.c b/arch/csky/kernel/ptrace.c
> > index d822144906ac..a4cf2e2ac15a 100644
> > --- a/arch/csky/kernel/ptrace.c
> > +++ b/arch/csky/kernel/ptrace.c
> > @@ -83,7 +83,7 @@ static int gpr_get(struct task_struct *target,
> >       /* Abiv1 regs->tls is fake and we need sync here. */
> >       regs->tls = task_thread_info(target)->tp_value;
> >
> > -     return membuf_write(&to, regs, sizeof(regs));
> > +     return membuf_write(&to, regs, sizeof(*regs));
> >  }
> >
> >  static int gpr_set(struct task_struct *target,
> > --
> > 2.25.1
> >



-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Al Viro]

On Wed, Sep 23, 2020 at 08:03:20AM +0800, Guo Ren wrote:
> Thx Duan,
> 
> Acked-by: Guo Ren <guoren@kernel.org>
> 
> Hi AI,
> 
> I found the broken commit still has a question:
> 
> > commit dcad7854fcce6a2d49b6a3ead5bbefeff047e559
> > Author: Al Viro <viro@zeniv.linux.org.uk>
> > Date:   Tue Jun 16 15:28:29 2020 -0400
> 
> >    csky: switch to ->regset_get()
> 
> >    NB: WTF "- what the fuck :(" is fpregs_get() playing at???
> The fpregs_get() is for REGSET_FPR regset used by ptrace (gdb) and all
> fp regs are stored in threads' context.
> So, WTF question for?

The part under
#if defined(CONFIG_CPU_HAS_FPUV2) && !defined(CONFIG_CPU_HAS_VDSP)

What's going on there?  The mapping is really weird - assuming
you had v0..v31 in the first 32 elements of regs->vr[], you
end up with

v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31

in the beginning of the output.  Assuming it is the intended
behaviour, it's probably worth some comments...

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Zhenzhong Duan]

On Wed, Sep 23, 2020 at 12:29 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Sep 22, 2020 at 05:15:05PM +0800, Zhenzhong Duan wrote:
> > "*" is missed  in size determination as we are passing register set
> > rather than a pointer.
>
> Ack.  I can push it to Linus today, unless you want it to go through
> csky tree.  Preferences?

I prefer pushing to linus.

Regards
Zhenzhong

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Guo Ren]

On Wed, Sep 23, 2020 at 8:23 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Sep 23, 2020 at 08:03:20AM +0800, Guo Ren wrote:
> > Thx Duan,
> >
> > Acked-by: Guo Ren <guoren@kernel.org>
> >
> > Hi AI,
> >
> > I found the broken commit still has a question:
> >
> > > commit dcad7854fcce6a2d49b6a3ead5bbefeff047e559
> > > Author: Al Viro <viro@zeniv.linux.org.uk>
> > > Date:   Tue Jun 16 15:28:29 2020 -0400
> >
> > >    csky: switch to ->regset_get()
> >
> > >    NB: WTF "- what the fuck :(" is fpregs_get() playing at???
> > The fpregs_get() is for REGSET_FPR regset used by ptrace (gdb) and all
> > fp regs are stored in threads' context.
> > So, WTF question for?
>
> The part under
> #if defined(CONFIG_CPU_HAS_FPUV2) && !defined(CONFIG_CPU_HAS_VDSP)
>
> What's going on there?  The mapping is really weird - assuming
> you had v0..v31 in the first 32 elements of regs->vr[], you
> end up with
>
> v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
>
> in the beginning of the output.  Assuming it is the intended
> behaviour, it's probably worth some comments...
FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
regs' width is 128b.

vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
...
vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
vr[64], vr[65] = fp[16]
vr[66], vr[67] = fp[17]
...
vr[94], vr[95] = fp[31]

Yeah, this is confusing and I'll add a comment later.




--
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Al Viro]

On Wed, Sep 23, 2020 at 10:37:31AM +0800, Guo Ren wrote:

> > What's going on there?  The mapping is really weird - assuming
> > you had v0..v31 in the first 32 elements of regs->vr[], you
> > end up with
> >
> > v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> > v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
> >
> > in the beginning of the output.  Assuming it is the intended
> > behaviour, it's probably worth some comments...
> FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
> regs' width is 128b.
> 
> vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
> ...
> vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
> vr[64], vr[65] = fp[16]
> vr[66], vr[67] = fp[17]
> ...
> vr[94], vr[95] = fp[31]
> 
> Yeah, this is confusing and I'll add a comment later.

Umm...  It would help if you described these 3 layouts:
	1) kernel-side with VDSP
	2) userland (identical to (1)?)
	3) kernel-side without VDSP
Still confused...

PS: my apologies re commit message - I left a note to myself when doing
that series and then forgot about it ;-/

Anyway, which tree should it go through?  In any case, that fix is
Acked-by: Al Viro <viro@zeniv.linux.org.uk>
and I can take it through vfs.git or you guys can pick in csky tree;
up to you.

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Guo Ren]

On Wed, Sep 23, 2020 at 12:52 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Sep 23, 2020 at 10:37:31AM +0800, Guo Ren wrote:
>
> > > What's going on there?  The mapping is really weird - assuming
> > > you had v0..v31 in the first 32 elements of regs->vr[], you
> > > end up with
> > >
> > > v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> > > v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
> > >
> > > in the beginning of the output.  Assuming it is the intended
> > > behaviour, it's probably worth some comments...
> > FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
> > regs' width is 128b.
> >
> > vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
> > ...
> > vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
> > vr[64], vr[65] = fp[16]
> > vr[66], vr[67] = fp[17]
> > ...
> > vr[94], vr[95] = fp[31]
> >
> > Yeah, this is confusing and I'll add a comment later.
>
> Umm...  It would help if you described these 3 layouts:
>         1) kernel-side with VDSP
With VDSP: we use vdsp ld/st instructions to access first 16
128bit-regs and use fpu ld/st instructions to access last 16
64bit-regs.

>         2) userland (identical to (1)?)
Identical to 1.

>         3) kernel-side without VDSP
Without VDSP: we use fpu ld/st instructions to access last 32 64bit-regs.

So, there are 96 32bit-vr[] for the struct. And with VDSP or not will
got different storage format.
With VDSP:
vr128[16] // contain first fp64[16]
fp64[16]; // second fp64[16]

Without VDSP:
fp64[32]
no-use for the reset vr[]

> Still confused...
>
> PS: my apologies re commit message - I left a note to myself when doing
> that series and then forgot about it ;-/
>
> Anyway, which tree should it go through?  In any case, that fix is
Thx for your job, and pushing to linus with Zhenzhong Duan's advice.

> Acked-by: Al Viro <viro@zeniv.linux.org.uk>
> and I can take it through vfs.git or you guys can pick in csky tree;
> up to you.

--
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Zhenzhong Duan]

On Wed, Sep 23, 2020 at 12:52 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Sep 23, 2020 at 10:37:31AM +0800, Guo Ren wrote:
>
> > > What's going on there?  The mapping is really weird - assuming
> > > you had v0..v31 in the first 32 elements of regs->vr[], you
> > > end up with
> > >
> > > v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> > > v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
> > >
> > > in the beginning of the output.  Assuming it is the intended
> > > behaviour, it's probably worth some comments...
> > FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
> > regs' width is 128b.
> >
> > vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
> > ...
> > vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
> > vr[64], vr[65] = fp[16]
> > vr[66], vr[67] = fp[17]
> > ...
> > vr[94], vr[95] = fp[31]
> >
> > Yeah, this is confusing and I'll add a comment later.
>
> Umm...  It would help if you described these 3 layouts:
>         1) kernel-side with VDSP
>         2) userland (identical to (1)?)
>         3) kernel-side without VDSP
> Still confused...
>
> PS: my apologies re commit message - I left a note to myself when doing
> that series and then forgot about it ;-/
>
> Anyway, which tree should it go through?  In any case, that fix is
> Acked-by: Al Viro <viro@zeniv.linux.org.uk>
> and I can take it through vfs.git or you guys can pick in csky tree;
> up to you.

Hi Al, Guo

Seems this patch is still pending， could you help check it？Thanks

Zhenzhong

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Guo Ren]

Hi Zhengzhong,

I'll take it, thx.

On Wed, Dec 23, 2020 at 10:31 AM Zhenzhong Duan
<zhenzhong.duan@gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 12:52 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Wed, Sep 23, 2020 at 10:37:31AM +0800, Guo Ren wrote:
> >
> > > > What's going on there?  The mapping is really weird - assuming
> > > > you had v0..v31 in the first 32 elements of regs->vr[], you
> > > > end up with
> > > >
> > > > v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> > > > v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
> > > >
> > > > in the beginning of the output.  Assuming it is the intended
> > > > behaviour, it's probably worth some comments...
> > > FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
> > > regs' width is 128b.
> > >
> > > vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
> > > ...
> > > vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
> > > vr[64], vr[65] = fp[16]
> > > vr[66], vr[67] = fp[17]
> > > ...
> > > vr[94], vr[95] = fp[31]
> > >
> > > Yeah, this is confusing and I'll add a comment later.
> >
> > Umm...  It would help if you described these 3 layouts:
> >         1) kernel-side with VDSP
> >         2) userland (identical to (1)?)
> >         3) kernel-side without VDSP
> > Still confused...
> >
> > PS: my apologies re commit message - I left a note to myself when doing
> > that series and then forgot about it ;-/
> >
> > Anyway, which tree should it go through?  In any case, that fix is
> > Acked-by: Al Viro <viro@zeniv.linux.org.uk>
> > and I can take it through vfs.git or you guys can pick in csky tree;
> > up to you.
>
> Hi Al, Guo
>
> Seems this patch is still pending， could you help check it？Thanks
>
> Zhenzhong



-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Re: [PATCH] csky: Fix a size determination in gpr_get()

[Zhenzhong Duan]

On Wed, Dec 23, 2020 at 10:57 PM Guo Ren <guoren@kernel.org> wrote:
>
> Hi Zhengzhong,
>
> I'll take it, thx.

Thanks Guo.

Zhenzhong