Linux-csky Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] csky: Fix a size determination in gpr_get()
@ 2020-09-22  9:15 Zhenzhong Duan
  2020-09-22 16:29 ` Al Viro
  0 siblings, 1 reply; 8+ messages in thread
From: Zhenzhong Duan @ 2020-09-22  9:15 UTC (permalink / raw)
  To: linux-kernel, linux-csky; +Cc: oleg, guoren, viro, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] csky: Fix a size determination in gpr_get()
  2020-09-22  9:15 [PATCH] csky: Fix a size determination in gpr_get() Zhenzhong Duan
@ 2020-09-22 16:29 ` Al Viro
  2020-09-23  0:03   ` Guo Ren
  2020-09-23  2:23   ` Zhenzhong Duan
  0 siblings, 2 replies; 8+ messages in thread
From: Al Viro @ 2020-09-22 16:29 UTC (permalink / raw)
  To: Zhenzhong Duan; +Cc: linux-kernel, linux-csky, oleg, guoren

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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] csky: Fix a size determination in gpr_get()
  2020-09-22 16:29 ` Al Viro
@ 2020-09-23  0:03   ` Guo Ren
  2020-09-23  0:23     ` Al Viro
  2020-09-23  2:23   ` Zhenzhong Duan
  1 sibling, 1 reply; 8+ messages in thread
From: Guo Ren @ 2020-09-23  0:03 UTC (permalink / raw)
  To: Al Viro
  Cc: Zhenzhong Duan, Linux Kernel Mailing List, linux-csky, Oleg Nesterov

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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] csky: Fix a size determination in gpr_get()
  2020-09-23  0:03   ` Guo Ren
@ 2020-09-23  0:23     ` Al Viro
  2020-09-23  2:37       ` Guo Ren
  0 siblings, 1 reply; 8+ messages in thread
From: Al Viro @ 2020-09-23  0:23 UTC (permalink / raw)
  To: Guo Ren
  Cc: Zhenzhong Duan, Linux Kernel Mailing List, linux-csky, Oleg Nesterov

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...

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] csky: Fix a size determination in gpr_get()
  2020-09-22 16:29 ` Al Viro
  2020-09-23  0:03   ` Guo Ren
@ 2020-09-23  2:23   ` Zhenzhong Duan
  1 sibling, 0 replies; 8+ messages in thread
From: Zhenzhong Duan @ 2020-09-23  2:23 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-kernel, linux-csky, oleg, guoren

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] csky: Fix a size determination in gpr_get()
  2020-09-23  0:23     ` Al Viro
@ 2020-09-23  2:37       ` Guo Ren
  2020-09-23  4:52         ` Al Viro
  0 siblings, 1 reply; 8+ messages in thread
From: Guo Ren @ 2020-09-23  2:37 UTC (permalink / raw)
  To: Al Viro
  Cc: Zhenzhong Duan, Linux Kernel Mailing List, linux-csky, Oleg Nesterov

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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] csky: Fix a size determination in gpr_get()
  2020-09-23  2:37       ` Guo Ren
@ 2020-09-23  4:52         ` Al Viro
  2020-09-25  6:33           ` Guo Ren
  0 siblings, 1 reply; 8+ messages in thread
From: Al Viro @ 2020-09-23  4:52 UTC (permalink / raw)
  To: Guo Ren
  Cc: Zhenzhong Duan, Linux Kernel Mailing List, linux-csky, Oleg Nesterov

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] csky: Fix a size determination in gpr_get()
  2020-09-23  4:52         ` Al Viro
@ 2020-09-25  6:33           ` Guo Ren
  0 siblings, 0 replies; 8+ messages in thread
From: Guo Ren @ 2020-09-25  6:33 UTC (permalink / raw)
  To: Al Viro
  Cc: Zhenzhong Duan, Linux Kernel Mailing List, linux-csky, Oleg Nesterov

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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22  9:15 [PATCH] csky: Fix a size determination in gpr_get() Zhenzhong Duan
2020-09-22 16:29 ` Al Viro
2020-09-23  0:03   ` Guo Ren
2020-09-23  0:23     ` Al Viro
2020-09-23  2:37       ` Guo Ren
2020-09-23  4:52         ` Al Viro
2020-09-25  6:33           ` Guo Ren
2020-09-23  2:23   ` Zhenzhong Duan

Linux-csky Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-csky/0 linux-csky/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-csky linux-csky/ https://lore.kernel.org/linux-csky \
		linux-csky@vger.kernel.org
	public-inbox-index linux-csky

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-csky


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git