[PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Mike Frysinger]

From: Jie Zhang <jie.zhang@analog.com>

The mmu code uses the copy_*_user_page() variants in access_process_vm()
rather than copy_*_user() as the former includes an icache flush.  This is
important when doing things like setting software breakpoints with gdb.
So switch the nommu code over to do the same.

Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 mm/nommu.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/nommu.c b/mm/nommu.c
index 9876fa0..51ae9be 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1889,9 +1889,11 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
 
 		/* only read or write mappings where it is permitted */
 		if (write && vma->vm_flags & VM_MAYWRITE)
-			len -= copy_to_user((void *) addr, buf, len);
+			copy_to_user_page(vma, NULL, NULL,
+					  (void *) addr, buf, len);
 		else if (!write && vma->vm_flags & VM_MAYREAD)
-			len -= copy_from_user(buf, (void *) addr, len);
+			copy_from_user_page(vma, NULL, NULL,
+					    buf, (void *) addr, len);
 		else
 			len = 0;
 	} else {
-- 
1.6.5.3

Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Jamie Lokier]

Mike Frysinger wrote:
> From: Jie Zhang <jie.zhang@analog.com>
> 
> The mmu code uses the copy_*_user_page() variants in access_process_vm()
> rather than copy_*_user() as the former includes an icache flush.  This is
> important when doing things like setting software breakpoints with gdb.
> So switch the nommu code over to do the same.

Reasonable, but it's a bit subtle don't you think?
How about a one-line comment saying why it's using copy_*_user_page()?

(If it was called copy_*_user_flush_icache() I wouldn't say anything,
but it isn't).

> Signed-off-by: Jie Zhang <jie.zhang@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>  mm/nommu.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 9876fa0..51ae9be 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1889,9 +1889,11 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
>  
>  		/* only read or write mappings where it is permitted */
>  		if (write && vma->vm_flags & VM_MAYWRITE)
> -			len -= copy_to_user((void *) addr, buf, len);
> +			copy_to_user_page(vma, NULL, NULL,
> +					  (void *) addr, buf, len);
>  		else if (!write && vma->vm_flags & VM_MAYREAD)
> -			len -= copy_from_user(buf, (void *) addr, len);
> +			copy_from_user_page(vma, NULL, NULL,
> +					    buf, (void *) addr, len);
>  		else
>  			len = 0;
>  	} else {

Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Jie Zhang]

On 11/25/2009 02:16 PM, Jamie Lokier wrote:
> Mike Frysinger wrote:
>> From: Jie Zhang<jie.zhang@analog.com>
>>
>> The mmu code uses the copy_*_user_page() variants in access_process_vm()
>> rather than copy_*_user() as the former includes an icache flush.  This is
>> important when doing things like setting software breakpoints with gdb.
>> So switch the nommu code over to do the same.
>
> Reasonable, but it's a bit subtle don't you think?
> How about a one-line comment saying why it's using copy_*_user_page()?
>
> (If it was called copy_*_user_flush_icache() I wouldn't say anything,
> but it isn't).
>
But I think it's well known in Linux kernel developers that 
copy_to_user_page and copy_from_user_page should do cache flushing. It's 
documented in Documentation/cachetlb.txt. I don't think it's necessary 
to replicate it here.


Jie

Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Paul Mundt]

On Wed, Nov 25, 2009 at 02:27:22PM +0800, Jie Zhang wrote:
> On 11/25/2009 02:16 PM, Jamie Lokier wrote:
> >Mike Frysinger wrote:
> >>From: Jie Zhang<jie.zhang@analog.com>
> >>
> >>The mmu code uses the copy_*_user_page() variants in access_process_vm()
> >>rather than copy_*_user() as the former includes an icache flush.  This is
> >>important when doing things like setting software breakpoints with gdb.
> >>So switch the nommu code over to do the same.
> >
> >Reasonable, but it's a bit subtle don't you think?
> >How about a one-line comment saying why it's using copy_*_user_page()?
> >
> >(If it was called copy_*_user_flush_icache() I wouldn't say anything,
> >but it isn't).
> >
> But I think it's well known in Linux kernel developers that 
> copy_to_user_page and copy_from_user_page should do cache flushing. It's 
> documented in Documentation/cachetlb.txt. I don't think it's necessary 
> to replicate it here.
> 
Documenting it in the changelog is sufficient I think. Platforms that
need the I-cache flush can deal with it there, and those that don't
aren't going to notice any difference regardless. The change in semantics
is subtle, but as it's bringing it in line with MMU behaviour it's not
really worth noting the fact that NOMMU behaviour used to be different
for no particular reason.

Acked-by: Paul Mundt <lethal@linux-sh.org>

Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Jamie Lokier]

Jie Zhang wrote:
> On 11/25/2009 02:16 PM, Jamie Lokier wrote:
> >Mike Frysinger wrote:
> >>From: Jie Zhang<jie.zhang@analog.com>
> >>
> >>The mmu code uses the copy_*_user_page() variants in access_process_vm()
> >>rather than copy_*_user() as the former includes an icache flush.  This is
> >>important when doing things like setting software breakpoints with gdb.
> >>So switch the nommu code over to do the same.
> >
> >Reasonable, but it's a bit subtle don't you think?
> >How about a one-line comment saying why it's using copy_*_user_page()?
> >
> >(If it was called copy_*_user_flush_icache() I wouldn't say anything,
> >but it isn't).
> >
> But I think it's well known in Linux kernel developers that 
> copy_to_user_page and copy_from_user_page should do cache flushing. It's 
> documented in Documentation/cachetlb.txt. I don't think it's necessary 
> to replicate it here.

You're right, however I now think the commit message is misleading.

Since this is the *only place in the entire kernel* where these
functions are used (plus the mmu equivalent), I'm not sure I'd agree
about well known, and the name could be better (copy_*_user_ptrace()),
but I agree now, it doesn't need a comment.

It was the talk of icache flush which bothered me, as I (wrongly)
assumed copy_*_user_page() was used elsewhere, without knowledge of
icache vs non-icache differences - which are often the responsibility
of userspace to get right, so often the kernel does not care.

In fact, it's not just icache.  copy_*_user_page() has to do some
*data* cache flushing too, on some architecures.  For example, see
arch/sparc/include/asm/cacheflush_64.h:

    #define copy_to_user_page(vma, page, vaddr, dst, src, len)              \
            do {                                                            \
                    flush_cache_page(vma, vaddr, page_to_pfn(page));        \
                    memcpy(dst, src, len);                                  \
                    flush_ptrace_access(vma, page, vaddr, src, len, 0);     \
            } while (0)

    #define copy_from_user_page(vma, page, vaddr, dst, src, len)            \
            do {                                                            \
                    flush_cache_page(vma, vaddr, page_to_pfn(page));        \
                    memcpy(dst, src, len);                                  \
                    flush_ptrace_access(vma, page, vaddr, dst, len, 1);     \
            } while (0)

I'm not sure why I don't see the same dcache flushing on ARM, so I
wonder if the ARM implementation of these buggy.

Anyway, that means the commit message is a little misleading, saying
it's for the icache flush.  It is for whatever icache and dcache
flushes are needed for a ptrace access.

Which is why, given they are only used for ptrace (have just grepped),
I'm inclined to think it'd be clearer to rename the functions to
copy_*_user_ptrace().  And make your no-mmu change of course :-)
Any thoughts on the rename?

-- Jamie

Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Jie Zhang]

On 11/25/2009 07:49 PM, Jamie Lokier wrote:
> Jie Zhang wrote:
>> On 11/25/2009 02:16 PM, Jamie Lokier wrote:
>>> Mike Frysinger wrote:
>>>> From: Jie Zhang<jie.zhang@analog.com>
>>>>
>>>> The mmu code uses the copy_*_user_page() variants in access_process_vm()
>>>> rather than copy_*_user() as the former includes an icache flush.  This is
>>>> important when doing things like setting software breakpoints with gdb.
>>>> So switch the nommu code over to do the same.
>>>
>>> Reasonable, but it's a bit subtle don't you think?
>>> How about a one-line comment saying why it's using copy_*_user_page()?
>>>
>>> (If it was called copy_*_user_flush_icache() I wouldn't say anything,
>>> but it isn't).
>>>
>> But I think it's well known in Linux kernel developers that
>> copy_to_user_page and copy_from_user_page should do cache flushing. It's
>> documented in Documentation/cachetlb.txt. I don't think it's necessary
>> to replicate it here.
>
> You're right, however I now think the commit message is misleading.
>
> Since this is the *only place in the entire kernel* where these
> functions are used (plus the mmu equivalent), I'm not sure I'd agree
> about well known, and the name could be better (copy_*_user_ptrace()),
> but I agree now, it doesn't need a comment.
>
> It was the talk of icache flush which bothered me, as I (wrongly)
> assumed copy_*_user_page() was used elsewhere, without knowledge of
> icache vs non-icache differences - which are often the responsibility
> of userspace to get right, so often the kernel does not care.
>
> In fact, it's not just icache.  copy_*_user_page() has to do some
> *data* cache flushing too, on some architecures.  For example, see

You are right. We needs dcache flushing here, too. However, for harvard 
architecture, flushing icache implies you have to flush dcache. So in 
implementation, there is dcache flushing in flush_icache_range, as we do 
for Blackfin, as well as they do for ARM. So when we say icache 
flushing, dcache flushing is implied.

> I'm not sure why I don't see the same dcache flushing on ARM, so I
> wonder if the ARM implementation of these buggy.
>
I'm not familiar with ARM. But I believe they do dcache flushing after 
some grepping in arch/arm/*.

> Which is why, given they are only used for ptrace (have just grepped),
> I'm inclined to think it'd be clearer to rename the functions to
> copy_*_user_ptrace().  And make your no-mmu change of course :-)
> Any thoughts on the rename?
>
I have no opinion on renaming things. I can live with the current naming.


Jie

Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Mike Frysinger]

On Wed, Nov 25, 2009 at 06:49, Jamie Lokier wrote:
> Jie Zhang wrote:
>> On 11/25/2009 02:16 PM, Jamie Lokier wrote:
>> >Mike Frysinger wrote:
>> >>From: Jie Zhang<jie.zhang@analog.com>
>> >>
>> >>The mmu code uses the copy_*_user_page() variants in access_process_vm()
>> >>rather than copy_*_user() as the former includes an icache flush.  This is
>> >>important when doing things like setting software breakpoints with gdb.
>> >>So switch the nommu code over to do the same.
>> >
>> >Reasonable, but it's a bit subtle don't you think?
>> >How about a one-line comment saying why it's using copy_*_user_page()?
>> >
>> >(If it was called copy_*_user_flush_icache() I wouldn't say anything,
>> >but it isn't).
>> >
>> But I think it's well known in Linux kernel developers that
>> copy_to_user_page and copy_from_user_page should do cache flushing. It's
>> documented in Documentation/cachetlb.txt. I don't think it's necessary
>> to replicate it here.
>
> You're right, however I now think the commit message is misleading.
>
> Since this is the *only place in the entire kernel* where these
> functions are used (plus the mmu equivalent), I'm not sure I'd agree
> about well known, and the name could be better (copy_*_user_ptrace()),
> but I agree now, it doesn't need a comment.
>
> It was the talk of icache flush which bothered me, as I (wrongly)
> assumed copy_*_user_page() was used elsewhere, without knowledge of
> icache vs non-icache differences - which are often the responsibility
> of userspace to get right, so often the kernel does not care.
>
> In fact, it's not just icache.  copy_*_user_page() has to do some
> *data* cache flushing too, on some architecures.  For example, see
> arch/sparc/include/asm/cacheflush_64.h:
>
>    #define copy_to_user_page(vma, page, vaddr, dst, src, len)              \
>            do {                                                            \
>                    flush_cache_page(vma, vaddr, page_to_pfn(page));        \
>                    memcpy(dst, src, len);                                  \
>                    flush_ptrace_access(vma, page, vaddr, src, len, 0);     \
>            } while (0)
>
>    #define copy_from_user_page(vma, page, vaddr, dst, src, len)            \
>            do {                                                            \
>                    flush_cache_page(vma, vaddr, page_to_pfn(page));        \
>                    memcpy(dst, src, len);                                  \
>                    flush_ptrace_access(vma, page, vaddr, dst, len, 1);     \
>            } while (0)
>
> I'm not sure why I don't see the same dcache flushing on ARM, so I
> wonder if the ARM implementation of these buggy.
>
> Anyway, that means the commit message is a little misleading, saying
> it's for the icache flush.  It is for whatever icache and dcache
> flushes are needed for a ptrace access.
>
> Which is why, given they are only used for ptrace (have just grepped),
> I'm inclined to think it'd be clearer to rename the functions to
> copy_*_user_ptrace().  And make your no-mmu change of course :-)
> Any thoughts on the rename?

these are all good points, but i think unrelated to the patch in
question ;).  they can be done as a follow up.
-mike

Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[David McCullough]

Jivin Mike Frysinger lays it down ...
> From: Jie Zhang <jie.zhang@analog.com>
> 
> The mmu code uses the copy_*_user_page() variants in access_process_vm()
> rather than copy_*_user() as the former includes an icache flush.  This is
> important when doing things like setting software breakpoints with gdb.
> So switch the nommu code over to do the same.
> 
> Signed-off-by: Jie Zhang <jie.zhang@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Acked-by: David McCullough <david_mccullough@mcafee.com>

Cheers,
Davidm

> ---
>  mm/nommu.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 9876fa0..51ae9be 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1889,9 +1889,11 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
>  
>  		/* only read or write mappings where it is permitted */
>  		if (write && vma->vm_flags & VM_MAYWRITE)
> -			len -= copy_to_user((void *) addr, buf, len);
> +			copy_to_user_page(vma, NULL, NULL,
> +					  (void *) addr, buf, len);
>  		else if (!write && vma->vm_flags & VM_MAYREAD)
> -			len -= copy_from_user(buf, (void *) addr, len);
> +			copy_from_user_page(vma, NULL, NULL,
> +					    buf, (void *) addr, len);
>  		else
>  			len = 0;
>  	} else {
> -- 
> 1.6.5.3
> 
> _______________________________________________
> uClinux-dev mailing list
> uClinux-dev@uclinux.org
> http://mailman.uclinux.org/mailman/listinfo/uclinux-dev
> This message was resent by uclinux-dev@uclinux.org
> To unsubscribe see:
> http://mailman.uclinux.org/mailman/options/uclinux-dev
> 
> 

-- 
David McCullough,  david_mccullough@securecomputing.com,  Ph:+61 734352815
McAfee - SnapGear  http://www.snapgear.com                http://www.uCdot.org

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Greg Ungerer]

Mike Frysinger wrote:
> From: Jie Zhang <jie.zhang@analog.com>
> 
> The mmu code uses the copy_*_user_page() variants in access_process_vm()
> rather than copy_*_user() as the former includes an icache flush.  This is
> important when doing things like setting software breakpoints with gdb.
> So switch the nommu code over to do the same.
> 
> Signed-off-by: Jie Zhang <jie.zhang@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Acked-by: Greg Ungerer <gerg@uclinux.org>


>  mm/nommu.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 9876fa0..51ae9be 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1889,9 +1889,11 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
>  
>  		/* only read or write mappings where it is permitted */
>  		if (write && vma->vm_flags & VM_MAYWRITE)
> -			len -= copy_to_user((void *) addr, buf, len);
> +			copy_to_user_page(vma, NULL, NULL,
> +					  (void *) addr, buf, len);
>  		else if (!write && vma->vm_flags & VM_MAYREAD)
> -			len -= copy_from_user(buf, (void *) addr, len);
> +			copy_from_user_page(vma, NULL, NULL,
> +					    buf, (void *) addr, len);
>  		else
>  			len = 0;
>  	} else {


-- 
------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[David Howells]

Mike Frysinger <vapier@gentoo.org> wrote:

> -			len -= copy_to_user((void *) addr, buf, len);
> +			copy_to_user_page(vma, NULL, NULL,
> +					  (void *) addr, buf, len);
>  		else if (!write && vma->vm_flags & VM_MAYREAD)
> -			len -= copy_from_user(buf, (void *) addr, len);
> +			copy_from_user_page(vma, NULL, NULL,
> +					    buf, (void *) addr, len);

Hmmm...  With this, len isn't updated anymore, and so it alters the return
value of access_process_vm(), and means ptrace_readdata() won't now return
-EIO under some circumstances where it used to.  I'm not sure that matters,
though.

David

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Jie Zhang]

On 12/02/2009 10:36 PM, David Howells wrote:
> Mike Frysinger<vapier@gentoo.org>  wrote:
>
>> -			len -= copy_to_user((void *) addr, buf, len);
>> +			copy_to_user_page(vma, NULL, NULL,
>> +					  (void *) addr, buf, len);
>>   		else if (!write&&  vma->vm_flags&  VM_MAYREAD)
>> -			len -= copy_from_user(buf, (void *) addr, len);
>> +			copy_from_user_page(vma, NULL, NULL,
>> +					    buf, (void *) addr, len);
>
> Hmmm...  With this, len isn't updated anymore, and so it alters the return
> value of access_process_vm(), and means ptrace_readdata() won't now return
> -EIO under some circumstances where it used to.  I'm not sure that matters,
> though.
>
This keeps access_process_vm() in nommu.c align with the one in 
memory.c. If this does really matter, someone or me can write another 
patch to take care of it for both MMU and !MMU later.


Jie

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[David Howells]

Mike Frysinger <vapier@gentoo.org> wrote:

> The mmu code uses the copy_*_user_page() variants in access_process_vm()
> rather than copy_*_user() as the former includes an icache flush.  This is
> important when doing things like setting software breakpoints with gdb.
> So switch the nommu code over to do the same.

Note that we may only really want to do an icache flush if the target region
is mapped executable somewhere.  On the other hand, for debugging stuff on an
embedded board, it probably doesn't matter.

David

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Jie Zhang]

On 12/02/2009 10:45 PM, David Howells wrote:
> Mike Frysinger<vapier@gentoo.org>  wrote:
>
>> The mmu code uses the copy_*_user_page() variants in access_process_vm()
>> rather than copy_*_user() as the former includes an icache flush.  This is
>> important when doing things like setting software breakpoints with gdb.
>> So switch the nommu code over to do the same.
>
> Note that we may only really want to do an icache flush if the target region
> is mapped executable somewhere.  On the other hand, for debugging stuff on an
> embedded board, it probably doesn't matter.
>
It is not checked before icache flush even for MMU. This is a place we 
can do some improvement. But it's not important for debugging.


Jie

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[David Howells]

Mike Frysinger <vapier@gentoo.org> wrote:

> +			copy_to_user_page(vma, NULL, NULL,
> +					  (void *) addr, buf, len);
> ...
> +			copy_from_user_page(vma, NULL, NULL,
> +					    buf, (void *) addr, len);

I think this is not correct.  The third parameter in both cases (vaddr) is of
unsigned long type (so should be 0 not NULL), and should not be left zero in
any case.  I think it should be passed addr.  In fact, we should really pass
the second parameter too (page), though for now, I'm happy to leave that NULL.

See attached revision of the patch.

David
---
From: Jie Zhang <jie.zhang@analog.com>
Subject: [PATCH] NOMMU: Use copy_*_user_page() in access_process_vm()

The MMU code uses the copy_*_user_page() variants in access_process_vm()
rather than copy_*_user() as the former includes an icache flush.  This is
important when doing things like setting software breakpoints with gdb.
So switch the NOMMU code over to do the same.

This patch makes the reasonable assumption that copy_from_user_page() won't
fail - which is probably fine, as we've checked the VMA from which we're
copying is usable, and the copy is not allowed to cross VMAs.  The one case
where it might go wrong is if the VMA is a device rather than RAM, and that
device returns an error which - in which case rubbish will be returned rather
than EIO.

Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 mm/nommu.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)


diff --git a/mm/nommu.c b/mm/nommu.c
index af12270..953800f 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1896,9 +1896,11 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
 
 		/* only read or write mappings where it is permitted */
 		if (write && vma->vm_flags & VM_MAYWRITE)
-			len -= copy_to_user((void *) addr, buf, len);
+			copy_to_user_page(vma, NULL, addr,
+					 (void *) addr, buf, len);
 		else if (!write && vma->vm_flags & VM_MAYREAD)
-			len -= copy_from_user(buf, (void *) addr, len);
+			copy_from_user_page(vma, NULL, addr,
+					    buf, (void *) addr, len);
 		else
 			len = 0;
 	} else {

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Jie Zhang]

On 12/08/2009 06:57 PM, David Howells wrote:
> Mike Frysinger<vapier@gentoo.org>  wrote:
>
>> +			copy_to_user_page(vma, NULL, NULL,
>> +					  (void *) addr, buf, len);
>> ...
>> +			copy_from_user_page(vma, NULL, NULL,
>> +					    buf, (void *) addr, len);
>
> I think this is not correct.  The third parameter in both cases (vaddr) is of
> unsigned long type (so should be 0 not NULL), and should not be left zero in
> any case.  I think it should be passed addr.  In fact, we should really pass
> the second parameter too (page), though for now, I'm happy to leave that NULL.
>
> See attached revision of the patch.
>
I agree on using 0 instead of NULL. But for !MMU, I think, vaddr is 
always as same as addr. So we don't need to pass it?


Jie


> David
> ---
> From: Jie Zhang<jie.zhang@analog.com>
> Subject: [PATCH] NOMMU: Use copy_*_user_page() in access_process_vm()
>
> The MMU code uses the copy_*_user_page() variants in access_process_vm()
> rather than copy_*_user() as the former includes an icache flush.  This is
> important when doing things like setting software breakpoints with gdb.
> So switch the NOMMU code over to do the same.
>
> This patch makes the reasonable assumption that copy_from_user_page() won't
> fail - which is probably fine, as we've checked the VMA from which we're
> copying is usable, and the copy is not allowed to cross VMAs.  The one case
> where it might go wrong is if the VMA is a device rather than RAM, and that
> device returns an error which - in which case rubbish will be returned rather
> than EIO.
>
> Signed-off-by: Jie Zhang<jie.zhang@analog.com>
> Signed-off-by: Mike Frysinger<vapier@gentoo.org>
> Signed-off-by: David Howells<dhowells@redhat.com>
> ---
>
>   mm/nommu.c |    6 ++++--
>   1 files changed, 4 insertions(+), 2 deletions(-)
>
>
> diff --git a/mm/nommu.c b/mm/nommu.c
> index af12270..953800f 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1896,9 +1896,11 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
>
>   		/* only read or write mappings where it is permitted */
>   		if (write&&  vma->vm_flags&  VM_MAYWRITE)
> -			len -= copy_to_user((void *) addr, buf, len);
> +			copy_to_user_page(vma, NULL, addr,
> +					 (void *) addr, buf, len);
>   		else if (!write&&  vma->vm_flags&  VM_MAYREAD)
> -			len -= copy_from_user(buf, (void *) addr, len);
> +			copy_from_user_page(vma, NULL, addr,
> +					    buf, (void *) addr, len);
>   		else
>   			len = 0;
>   	} else {

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[David Howells]

Jie Zhang <jie.zhang@analog.com> wrote:

> I agree on using 0 instead of NULL. But for !MMU, I think, vaddr is always as
> same as addr. So we don't need to pass it?

FRV flushes the vaddr because in MMU mode the cache flush instructions take
virtual addresses, so if we pass addr as vaddr, I can use the same cache flush
code for both modes.  I suspect it makes little difference to the amount of
code if we pass that rather than 0, as the value is already computed, and
either way, it's going to take one instruction to set up the argument.

Note that Blackfin assumes that it may use the dst address for flushing - an
assumption that isn't valid in MMU mode with a VIVT cache (which I presume
Blackfin isn't, but other CPUs are).

David

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Jie Zhang]

On 12/08/2009 10:19 PM, David Howells wrote:
> Jie Zhang<jie.zhang@analog.com>  wrote:
>
>> I agree on using 0 instead of NULL. But for !MMU, I think, vaddr is always as
>> same as addr. So we don't need to pass it?
>
> FRV flushes the vaddr because in MMU mode the cache flush instructions take
> virtual addresses, so if we pass addr as vaddr, I can use the same cache flush
> code for both modes.  I suspect it makes little difference to the amount of
> code if we pass that rather than 0, as the value is already computed, and
> either way, it's going to take one instruction to set up the argument.
>
> Note that Blackfin assumes that it may use the dst address for flushing - an
> assumption that isn't valid in MMU mode with a VIVT cache (which I presume
> Blackfin isn't, but other CPUs are).
>
Thanks for your explanation. Now I understand why passing add as vaddr 
is better.


Jie

Re: [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1047 bytes --]

On Tuesday 08 December 2009 09:19:27 David Howells wrote:
> Jie Zhang <jie.zhang@analog.com> wrote:
> > I agree on using 0 instead of NULL. But for !MMU, I think, vaddr is
> > always as same as addr. So we don't need to pass it?
> 
> FRV flushes the vaddr because in MMU mode the cache flush instructions take
> virtual addresses, so if we pass addr as vaddr, I can use the same cache
>  flush code for both modes.  I suspect it makes little difference to the
>  amount of code if we pass that rather than 0, as the value is already
>  computed, and either way, it's going to take one instruction to set up the
>  argument.
> 
> Note that Blackfin assumes that it may use the dst address for flushing -
>  an assumption that isn't valid in MMU mode with a VIVT cache (which I
>  presume Blackfin isn't, but other CPUs are).

the Blackfin cpu currently does not have virtual memory support at all, so i 
imagine there's a bunch of things that'll need updating when that day comes

the new patch is of course fine; thanks
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]