> >> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> >> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> >> @@ -162,33 +162,40 @@ static inline int hpte_cache_flags_ok(unsigned long
> >> ptel, unsigned long io_type)
> >> * Lock and read a linux PTE. If it's present and writable, atomically
> >> * set dirty and referenced bits and return the PTE, otherwise return 0.
> >
> > This is comment still valid now the ldarx/stdcx is gone?
>
> In a way yes. Instead of lock and read as it was before, it is now done
> via cmpxchg which still use ldarx/stdcx
OK, maybe you can update to reflect that.
> >> + pte_t old_pte, new_pte = __pte(0);
> >> +repeat:
> >> + do {
> >> + old_pte = pte_val(*ptep);
> >> + /*
> >> + * wait until _PAGE_BUSY is clear then set it atomically
> >> + */
> >> + if (unlikely(old_pte & _PAGE_BUSY))
> >> + goto repeat;
> >
> > continue here? Please don't create looping primitives.
>
> No that would be wrong. (I did that in an earlier version :).We really
> don't want the below cmpxchg to run if we find _PAGE_BUSY.
How about something like this then?
while (1) {
if (unlikely(old_pte & _PAGE_BUSY))
continue;
.....
if cmpxchg(foo)
break;
}
>
> >
> >> +
> >> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >> + /* If hugepage and is trans splitting return None */
> >> + if (unlikely(hugepage &&
> >> + pmd_trans_splitting(pte_pmd(old_pte))))
> >
> > Comment looks much like the code... seems redundant.
> >
> >> + return __pte(0);
> >> +#endif
> >>
> >> - *p = pte; /* clears _PAGE_BUSY */
> >> + /* If pte is not present return None */
> >> + if (unlikely(!(old_pte & _PAGE_PRESENT)))
> >> + return __pte(0);
> >>
> >> - return pte;
> >> + new_pte = pte_mkyoung(old_pte);
> >> + if (writing && pte_write(old_pte))
> >> + new_pte = pte_mkdirty(new_pte);
> >> +
> >> + } while (old_pte != __cmpxchg_u64((unsigned long *)ptep,
> >> + old_pte, new_pte));
> >> + return new_pte;
> >> }
> >>
> >> +
> >
> > Whitespace
> >> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> >> b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> >> index dcf892d..39ae723 100644
> >> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> >> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> >> @@ -150,9 +150,7 @@ static pte_t lookup_linux_pte(pgd_t *pgdir, unsigned
> >> long hva,
> >> *pte_sizep = PAGE_SIZE;
> >> if (ps > *pte_sizep)
> >> return __pte(0);
> >> - if (!pte_present(*ptep))
> >> - return __pte(0);
> >> - return kvmppc_read_update_linux_pte(ptep, writing);
> >> + return kvmppc_read_update_linux_pte(ptep, writing, shift);
> >
> > 'shift' goes into the new 'hugepage' parameter? Doesn't seem logical?
> > Can we harmonise the name to make it less confusing?
> >
>
> it is actually the shift bits represending hugepage size. We set it to 0
> if we don't find hugepage in find_linux_pte_or_hugepte. May be something
> like hugepage_shift is better ?
Sure.
Mikey
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
