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@xxxxxxxxxx>
>> >>
>> >>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/