Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()
Jie Zhang wrote:
On 11/25/2009 02:16 PM, Jamie Lokier wrote:
Mike Frysinger wrote:
From: Jie Zhangjie.zh...@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
___
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
Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()
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 Zhangjie.zh...@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
___
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
[uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()
From: Jie Zhang jie.zh...@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.zh...@analog.com
Signed-off-by: Mike Frysinger vap...@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
___
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
Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()
Mike Frysinger wrote:
From: Jie Zhang jie.zh...@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.zh...@analog.com
Signed-off-by: Mike Frysinger vap...@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 {
___
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
Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()
Jivin Mike Frysinger lays it down ...
From: Jie Zhang jie.zh...@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.zh...@analog.com
Signed-off-by: Mike Frysinger vap...@gentoo.org
Acked-by: David McCullough david_mccullo...@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_mccullo...@securecomputing.com, Ph:+61 734352815
McAfee - SnapGear http://www.snapgear.comhttp://www.uCdot.org
___
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
Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()
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 Zhangjie.zh...@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 let...@linux-sh.org ___ 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
