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

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

On Tue, Mar 01, 2016 at 09:13:23AM -0800, David Daney wrote:
> On 02/29/2016 06:37 PM, Paul Burton wrote:
> [...]
> >@@ -234,6 +237,22 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
> >  }
> >  #endif
> >
> >+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.

> What if you ...
> 
> 
> >  /*
> >   * (pmds are folded into puds so this doesn't get actually called,
> >   * but the define is needed for a generic inline function.)
> >@@ -430,15 +449,12 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
> >
> >  extern void __update_tlb(struct vm_area_struct *vma, unsigned long address,
> >  	pte_t pte);
> >-extern void __update_cache(struct vm_area_struct *vma, unsigned long address,
> >-	pte_t pte);
> >
> >  static inline void update_mmu_cache(struct vm_area_struct *vma,
> >  	unsigned long address, pte_t *ptep)
> >  {
> >  	pte_t pte = *ptep;
> >  	__update_tlb(vma, address, pte);
> >-	__update_cache(vma, address, pte);
> 
> ... Reversed the order of these two operations?

It would make no difference. The window for the race exists between
flush_dcache_page & set_pte_at. update_mmu_cache isn't called until
later than set_pte_at, so cannot possibly avoid the race. The commit
message walks through where the race exists - I don't think you've read
it.

Thanks,
    Paul