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

Ira Weiny
2022-02-22T22:05:46Z

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