Re: [PATCH 4/4] MIPS: Sync icache & dcache in set_pte_at

[Paul Burton <paul.burton@imgtec.com>]

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@nuts.davemloft.net>
> 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@davemloft.net>
> 
> 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