quoted

quoted

--- 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.

quoted

quoted

+	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;
}

quoted

quoted

+
+#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.

quoted

+			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

quoted

quoted

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