On Fri, Dec 18, 2020 at 1:06 PM Thomas Gleixner tglx@linutronix.de wrote:
On Fri, Dec 18 2020 at 11:20, Dan Williams wrote:
On Fri, Dec 18, 2020 at 5:58 AM Thomas Gleixner tglx@linutronix.de wrote: [..]
The DAX case which you made "work" with dev_access_enable() and dev_access_disable(), i.e. with yet another lazy approach of avoiding to change a handful of usage sites.
The use cases are strictly context local which means the global magic is not used at all. Why does it exist in the first place?
Aside of that this global thing would never work at all because the refcounting is per thread and not global.
So that DAX use case is just a matter of:
grant/revoke_access(DEV_PKS_KEY, READ/WRITE)
which is effective for the current execution context and really wants to be a distinct READ/WRITE protection and not the magic global thing which just has on/off. All usage sites know whether they want to read or write.
I was tracking and nodding until this point. Yes, kill the global / kmap() support, but if grant/revoke_access is not integrated behind kmap_{local,atomic}() then it's not a "handful" of sites that need to be instrumented it's 100s. Are you suggesting that "relaxed" mode enforcement is a way to distribute the work of teaching driver writers that they need to incorporate explicit grant/revoke-read/write in addition to kmap? The entire reason PTE_DEVMAP exists was to allow get_user_pages() for PMEM and not require every downstream-GUP code path to specifically consider whether it was talking to PMEM or RAM pages, and certainly not whether they were reading or writing to it.
kmap_local() is fine. That can work automatically because it's strict local to the context which does the mapping.
kmap() is dubious because it's a 'global' mapping as dictated per HIGHMEM. So doing the RELAXED mode for kmap() is sensible I think to identify cases where the mapped address is really handed to a different execution context. We want to see those cases and analyse whether this can't be solved in a different way. That's why I suggested to do a warning in that case.
Also vs. the DAX use case I really meant the code in fs/dax and drivers/dax/ itself which is handling this via dax_read_[un]lock.
Does that make more sense?
Yup, got it. The dax code can be precise wrt to PKS in a way that kmap_local() cannot.