On Wed, Mar 02, 2016 at 03:12:03PM +0100, Ralf Baechle wrote:
> On Tue, Mar 01, 2016 at 05:19:40PM +0000, Paul Burton wrote:
>
> > > >+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> > > >+ pte_t *ptep, pte_t pteval)
> > > >+{
> > > >+ extern void __update_cache(unsigned long address, pte_t pte);
> > > >+
> > > >+ if (!pte_present(pteval))
> > > >+ goto cache_sync_done;
> > > >+
> > > >+ if (pte_present(*ptep) && (pte_pfn(*ptep) == pte_pfn(pteval)))
> > > >+ goto cache_sync_done;
> > > >+
> > > >+ __update_cache(addr, pteval);
> > > >+cache_sync_done:
> > > >+ set_pte(ptep, pteval);
> > > >+}
> > > >+
> > >
> > > This seems crazy.
> >
> > Perhaps, but also correct...
> >
> > > I don't think any other architecture does this type of work in set_pte_at().
> >
> > Yes they do. As mentioned in the commit message see arm, ia64 or powerpc
> > for architectures that all do the same sort of thing in set_pte_at.
> >
> > > Can you look into finding a better way?
> >
> > Not that I can see.
>
> FYI, this is the original commit message adding set_pte_at(). The commit
> predates Linus' git history but is in the various history trees, for example
> https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/?id=ae3d0a847f4b38812241e4a5dc3371965c752a8c
>
> Or for your convenience:
>
> commit ae3d0a847f4b38812241e4a5dc3371965c752a8c
> Author: David S. Miller <davem@xxxxxxxxxxxxxxxxxx>
> Date: Tue Feb 22 23:42:56 2005 -0800
>
> [MM]: Add set_pte_at() which takes 'mm' and 'addr' args.
>
> I'm taking a slightly different approach this time around so things
> are easier to integrate. Here is the first patch which builds the
> infrastructure. Basically:
>
> 1) Add set_pte_at() which is set_pte() with 'mm' and 'addr' arguments
> added. All generic code uses set_pte_at().
>
> Most platforms simply get this define:
> #define set_pte_at(mm,addr,ptep,pteval) set_pte(ptep,pteval)
>
> I chose this method over simply changing all set_pte() call sites
> because many platforms implement this in assembler and it would
> take forever to preserve the build and stabilize things if modifying
> that was necessary.
>
> Soon, with platform maintainer's help, we can kill of set_pte() entirely.
> To be honest, there are only a handful of set_pte() call sites in the
> arch specific code.
>
> Actually, in this patch ppc64 is completely set_pte() free and does not
> define it.
>
> 2) pte_clear() gets 'mm' and 'addr' arguments now.
> This had a cascading effect on many ptep_test_and_*() routines. Specifically:
> a) ptep_test_and_clear_{young,dirty}() now take 'vma' and 'address' args.
> b) ptep_get_and_clear now take 'mm' and 'address' args.
> c) ptep_mkdirty was deleted, unused by any code.
> d) ptep_set_wrprotect now takes 'mm' and 'address' args.
>
> I've tested this patch as follows:
>
> 1) compile and run tested on sparc64/SMP
> 2) compile tested on:
> a) ppc64/SMP
> b) i386 both with and without PAE enabled
>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
>
> Which doesn't specify if the function is meant for cache management nor
> is it documented in Documentation/cachetlb.txt.
>
> Ralf
Hi Ralf,
It is, however, used in such a way by others & seems to me like the only
correct way to implement the lazy cache flushing. The alternative would
be to adjust all generic code to ensure flush_icache_page gets called
before set_pte_at, which is far more invasive.
Since you mention Documentation/cachetlb.txt that does indicate that
flush_icache_page should be removed some time & prior to this patch
we're making use of it, so I hardly think the content of that document
can be taken as defining what's acceptable.
Thanks,
Paul
