Messages in this thread | | | Date | Tue, 22 Feb 2022 14:05:42 -0800 | From | Ira Weiny <> | Subject | Re: [PATCH V8 38/44] memremap_pages: Define pgmap_mk_{readwrite|noaccess}() calls |
| |
On Fri, Feb 04, 2022 at 10:35:59AM -0800, Dan Williams wrote: > On Thu, Jan 27, 2022 at 9:55 AM <ira.weiny@intel.com> wrote: > > > > From: Ira Weiny <ira.weiny@intel.com> > > > > Users will need a way to flag valid access to pages which have been > > protected with PGMAP protections. Provide this by defining pgmap_mk_*() > > accessor functions. > > I find the ambiguous use of "Users" not helpful to set the context. How about: > > A thread that wants to access memory protected by PGMAP protections > must first enable access, and then disable access when it is done. > > > > > pgmap_mk_{readwrite|noaccess}() take a struct page for convenience. > > They determine if the page is protected by dev_pagemap protections. If > > so, they perform the requested operation. > > > > In addition, the lower level __pgmap_* functions are exported. They > > take the dev_pagemap object directly for internal users who have > > knowledge of the of the dev_pagemap. > > > > All changes in the protections must be through the above calls. They > > abstract the protection implementation (currently the PKS api) from the > > upper layer users. > > > > Furthermore, the calls are nestable by the use of a per task reference > > count. This ensures that the first call to re-enable protection does > > not 'break' the last access of the device memory. > > > > Access to device memory during exceptions (#PF) is expected only from > > user faults. Therefore there is no need to maintain the reference count > > when entering or exiting exceptions. However, reference counting will > > occur during the exception. Recall that protection is automatically > > enabled during exceptions by the PKS core.[1] > > > > NOTE: It is not anticipated that any code paths will directly nest these > > calls. For this reason multiple reviewers, including Dan and Thomas, > > asked why this reference counting was needed at this level rather than > > in a higher level call such as kmap_{atomic,local_page}(). The reason > > is that pgmap_mk_readwrite() could nest with regards to other callers of > > pgmap_mk_*() such as kmap_{atomic,local_page}(). Therefore push this > > reference counting to the lower level and just ensure that these calls > > are nestable. > > I still don't think that explains why task struct has a role to play > here, see below. > > Another missing bit of clarification, maybe I missed it, is why are > the protections toggled between read-write and noaccess. For > stray-write protection toggling between read-write and read-only is > sufficient. I can imagine speculative execution and debug rationales > for noaccess, but those should be called out explicitly. >
I'll clarify in the commit message but it is very simply providing consistent behavior for kmap'ing a page before and after this series. kmap's allows for both read and write access.
I know it was discussed to introduce the complexity of different mappings for read vs write. But I think that is something which could be added later rather than being a requirement of this series.
[snip]
> > The naming, which I had a hand in, is not aging well. When I see "mk" > I expect it to be building some value like a page table entry that > will be installed later. These helpers are directly enabling and > disabling access and are meant to be called symmetrically. So I would > expect symmetric names like: > > pgmap_enable_access() > pgmap_disable_access()
For this Dave requested s/pks_mk_*/pks_set_*/. So I've followed that convention here. New names are pgmap_set_*(). Although I'm not sure I'm happy with that name now...
Enable may sound better but we had used 'enable_access' before and it got all confusing for some reason... :-/
pgmap_set_noaccess() pgmap_set_readwrite()
Seems good I think.
Ira
| |