Re: [PATCH V8 38/44] memremap_pages: Define pgmap_mk_{readwrite|noaccess}() calls

[Ira Weiny <ira.weiny@intel.com>]

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