On Fri, Feb 27, 2015 at 11:12 PM, Benjamin Herrenschmidt
<benh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Let me know what you think of the approach.
Hmm. I'm not happy with just how many of those arch wrapper/helper
functions there are, and some of it looks a bit unportable.
For example, the code "knows" that "err_code" and "address" are the
only two architecture-specific pieces of information (in addition to
"struct pt_regs", of course.
And the fault_is_user/write() stuff seems unnecessary - we have the
generic FAULT_FLAG_USER/WRITE flags for that, but instead of passing
in the generic version to the generic function, we pass in the
arch-specific ones.
The same goes for "access_error()". Right now it's an arch-specific
helper function, but it actually does some generic tests (like
checking the vma protections), and I think it could/should be entirely
generic if we just added the necessary FAULT_FLAG_READ/EXEC/NOTPRESENT
information to the "flags" register. Just looking at the ppc version,
my gut feel is that "access_error()" is just horribly error-prone
as-is even from an architecture standpoint.
Yes, the "read/exec/notpresent" bits are a bit weird, but old x86
isn't the only architecture that doesn't necessarily know the
difference between read and exec.
So I'd like a bit more encapsulation. The generic code should never
really need to look at the arch-specific details, although it is true
that then the error handling cases will likely need it (in order to
put it on the arch-specific stack info or similar).
So my *gut* feel is that the generic code should be passed
- address and the generic flags (ie FAULT_FLAG_USER/WRITE filled in
by the caller)
These are the only things the generic code should need to use itself
- I guess we do need to pass in "struct pt_regs", because things like
generic tracing actually want it.
- an opaque "arch specific fault information structure pointer". Not
used for anything but passing back to the error functions (ie very
much *not* for helper functions for the normal case, like the current
"access_error()" - if it's actually needed for those kinds of things,
then I'm wrong about the model)
This would (for x86) contain "error_code", but I have the feeling
that other architectures might need/want more than one word of
information.
But I don't know. Maybe I'm wrong. I don't hate the patch as-is, I
just have this feeling that it coudl be more "generic", and less
"random small arch helpers".
Linus
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>