On Sat, 2015-02-28 at 11:56 -0800, Linus Torvalds wrote: > 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. I was trying to keep that fault mask local but it might indeed be a mistake. The reasoning is that if you look at more archs (the scary one is sparc), there is potentially more arch "gunk" that needs to be hooked in after find_vma()... And a lot of that need arch specific flags coming in. For example on ppc I check if the instruction is allowed to grow the stack before we take the mm sem and pass that information for use by the stack growing check. Here I could just I suppose add some arch specific fault flags but it might get messy... > 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. I was weary of going down the path of having access_error() be an arch helper because it gets even wilder with other archs and the crazy platform flags we deal with on ppc. Here. I was thinking of looking at factoring that in a second step. You are right that most of it seems to related to execute permission however (at least sparc, mips, powerpc and arm). The way to go would be to define a FAULT_FLAG_EXEC which the arch only sets if it supports enforcing exec at fault time. BTW. I fail to see how x86 checks PF_INSTR vs. VM_NOEXEC ... or it doesn't ? > 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). Right, we might still have to pass error_code around, I'll have a look. > 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. And error handling but it could be in the latter... > - 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) Well, it might be needed for stack growth check. It's also somewhat means we have yet *another* fault information structure which is a bit sad but I don't see a good way to fold them into one. For errors I was thinking instead of returning error info from the generic code and leaving the generation of oops/signal to the caller, which means less arch gunk to carry around. > 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". I don't like it that much either, it's a first draft to see what the waters are like. I'll play around more and see what It comes to. Cheers, Ben. > > 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> -- 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>