[PATCH 0/4] kvm: powerpc: use cache attributes from linux pte

[PATCH 0/4] kvm: powerpc: use cache attributes from linux pte

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

kvm: powerpc: use cache attributes from linux pte
 - 1st Patch fixes a bug in booke (detail in patch)
 - 2nd patch is renaming the linux_pte_lookup_function() just for clarity.
   There is not functional change.
 - 3nd Patch adds a Linux pte lookup function.
 - 4th Patch uses the above defined function and setup TLB.wimg accordingly

More details are there is individual patches.
  
Bharat Bhushan (4):
  kvm: booke: clear host tlb reference flag on guest tlb invalidation
  kvm: book3s: rename lookup_linux_pte() to
    lookup_linux_pte_and_update()
  kvm: powerpc: define a linux pte lookup function
  kvm: powerpc: use caching attributes as per linux pte

 arch/powerpc/include/asm/kvm_host.h |    2 +-
 arch/powerpc/include/asm/pgtable.h  |   35 ++++++++++++++++++++++
 arch/powerpc/kvm/book3s_hv_rm_mmu.c |    8 +++--
 arch/powerpc/kvm/booke.c            |    2 +-
 arch/powerpc/kvm/e500.h             |    8 +++--
 arch/powerpc/kvm/e500_mmu_host.c    |   55 +++++++++++++++++++---------------
 6 files changed, 78 insertions(+), 32 deletions(-)

[PATCH 1/4] kvm: booke: clear host tlb reference flag on guest tlb invalidation

[Bharat Bhushan]

On booke, "struct tlbe_ref" contains host tlb mapping information
(pfn: for guest-pfn to pfn, flags: attribute associated with this mapping)
for a guest tlb entry. So when a guest creates a TLB entry then
"struct tlbe_ref" is set to point to valid "pfn" and set attributes in
"flags" field of the above said structure. When a guest TLB entry is
invalidated then flags field of corresponding "struct tlbe_ref" is
updated to point that this is no more valid, also we selectively clear
some other attribute bits, example: if E500_TLB_BITMAP was set then we clear
E500_TLB_BITMAP, if E500_TLB_TLB0 is set then we clear this.

Ideally we should clear complete "flags" as this entry is invalid and does not
have anything to re-used. The other part of the problem is that when we use
the same entry again then also we do not clear (started doing or-ing etc).

So far it was working because the selectively clearing mentioned above
actually clears "flags" what was set during TLB mapping. But the problem
starts coming when we add more attributes to this then we need to selectively
clear them and which is not needed.

This patch we do both
        - Clear "flags" when invalidating;
        - Clear "flags" when reusing same entry later

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/kvm/e500_mmu_host.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
index 1c6a9d7..7a41a93 100644
--- a/arch/powerpc/kvm/e500_mmu_host.c
+++ b/arch/powerpc/kvm/e500_mmu_host.c
@@ -230,15 +230,15 @@ void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 *vcpu_e500, int tlbsel,
 		ref->flags &= ~(E500_TLB_TLB0 | E500_TLB_VALID);
 	}
 
-	/* Already invalidated in between */
-	if (!(ref->flags & E500_TLB_VALID))
-		return;
-
-	/* Guest tlbe is backed by at most one host tlbe per shadow pid. */
-	kvmppc_e500_tlbil_one(vcpu_e500, gtlbe);
+	/*
+	 * If TLB entry is still valid then it's a TLB0 entry, and thus
+	 * backed by at most one host tlbe per shadow pid
+	 */
+	if (ref->flags & E500_TLB_VALID)
+		kvmppc_e500_tlbil_one(vcpu_e500, gtlbe);
 
 	/* Mark the TLB as not backed by the host anymore */
-	ref->flags &= ~E500_TLB_VALID;
+	ref->flags = 0;
 }
 
 static inline int tlbe_is_writable(struct kvm_book3e_206_tlb_entry *tlbe)
@@ -251,7 +251,7 @@ static inline void kvmppc_e500_ref_setup(struct tlbe_ref *ref,
 					 pfn_t pfn)
 {
 	ref->pfn = pfn;
-	ref->flags |= E500_TLB_VALID;
+	ref->flags = E500_TLB_VALID;
 
 	if (tlbe_is_writable(gtlbe))
 		kvm_set_pfn_dirty(pfn);
-- 
1.7.0.4

[PATCH 2/4] kvm: book3s: rename lookup_linux_pte() to lookup_linux_pte_and_update()

[Bharat Bhushan]

lookup_linux_pte() is doing more than lookup, updating the pte,
so for clarity it is renamed to lookup_linux_pte_and_update()

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/kvm/book3s_hv_rm_mmu.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 45e30d6..1743ddd 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -134,7 +134,7 @@ static void remove_revmap_chain(struct kvm *kvm, long pte_index,
 	unlock_rmap(rmap);
 }
 
-static pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
+static pte_t lookup_linux_pte_and_update(pgd_t *pgdir, unsigned long hva,
 			      int writing, unsigned long *pte_sizep)
 {
 	pte_t *ptep;
@@ -231,7 +231,8 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
 
 		/* Look up the Linux PTE for the backing page */
 		pte_size = psize;
-		pte = lookup_linux_pte(pgdir, hva, writing, &pte_size);
+		pte = lookup_linux_pte_and_update(pgdir, hva, writing,
+						  &pte_size);
 		if (pte_present(pte)) {
 			if (writing && !pte_write(pte))
 				/* make the actual HPTE be read-only */
@@ -667,7 +668,8 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
 			memslot = __gfn_to_memslot(kvm_memslots(kvm), gfn);
 			if (memslot) {
 				hva = __gfn_to_hva_memslot(memslot, gfn);
-				pte = lookup_linux_pte(pgdir, hva, 1, &psize);
+				pte = lookup_linux_pte_and_update(pgdir, hva,
+								  1, &psize);
 				if (pte_present(pte) && !pte_write(pte))
 					r = hpte_make_readonly(r);
 			}
-- 
1.7.0.4

[PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Bharat Bhushan]

We need to search linux "pte" to get "pte" attributes for
setting TLB in KVM.
This patch defines a linux_pte_lookup() function for same.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/pgtable.h |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 7d6eacf..fd26c04 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -223,6 +223,41 @@ extern int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
 #endif
 pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea,
 				 unsigned *shift);
+
+static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
+				     unsigned long *pte_sizep)
+{
+	pte_t *ptep;
+	pte_t pte;
+	unsigned long ps = *pte_sizep;
+	unsigned int shift;
+
+	ptep = find_linux_pte_or_hugepte(pgdir, hva, &shift);
+	if (!ptep)
+		return __pte(0);
+	if (shift)
+		*pte_sizep = 1ul << shift;
+	else
+		*pte_sizep = PAGE_SIZE;
+
+	if (ps > *pte_sizep)
+		return __pte(0);
+
+	/* wait until _PAGE_BUSY is clear */
+	while (1) {
+		pte = pte_val(*ptep);
+		if (unlikely(pte & _PAGE_BUSY)) {
+			cpu_relax();
+			continue;
+		}
+	}
+
+	/* If pte is not present return None */
+	if (unlikely(!(pte & _PAGE_PRESENT)))
+		return __pte(0);
+
+	return pte;
+}
 #endif /* __ASSEMBLY__ */
 
 #endif /* __KERNEL__ */
-- 
1.7.0.4

Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Scott Wood]

On Tue, 2013-10-08 at 11:33 +0530, Bharat Bhushan wrote:
> We need to search linux "pte" to get "pte" attributes for
> setting TLB in KVM.
> This patch defines a linux_pte_lookup() function for same.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
>  arch/powerpc/include/asm/pgtable.h |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
> index 7d6eacf..fd26c04 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -223,6 +223,41 @@ extern int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
>  #endif
>  pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea,
>  				 unsigned *shift);
> +
> +static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
> +				     unsigned long *pte_sizep)
> +{
> +	pte_t *ptep;
> +	pte_t pte;
> +	unsigned long ps = *pte_sizep;
> +	unsigned int shift;
> +
> +	ptep = find_linux_pte_or_hugepte(pgdir, hva, &shift);
> +	if (!ptep)
> +		return __pte(0);
> +	if (shift)
> +		*pte_sizep = 1ul << shift;
> +	else
> +		*pte_sizep = PAGE_SIZE;
> +
> +	if (ps > *pte_sizep)
> +		return __pte(0);
> +
> +	/* wait until _PAGE_BUSY is clear */
> +	while (1) {
> +		pte = pte_val(*ptep);
> +		if (unlikely(pte & _PAGE_BUSY)) {
> +			cpu_relax();
> +			continue;
> +		}
> +	}
> +
> +	/* If pte is not present return None */
> +	if (unlikely(!(pte & _PAGE_PRESENT)))
> +		return __pte(0);
> +
> +	return pte;
> +}

Can lookup_linux_pte_and_update() call lookup_linux_pte()?

-Scott

RE: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Bhushan Bharat-R65777]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogV2VkbmVzZGF5LCBPY3RvYmVyIDA5LCAyMDEzIDM6MDcgQU0NCj4gVG86IEJo
dXNoYW4gQmhhcmF0LVI2NTc3Nw0KPiBDYzogYWdyYWZAc3VzZS5kZTsgWW9kZXIgU3R1YXJ0LUIw
ODI0ODsga3ZtQHZnZXIua2VybmVsLm9yZzsga3ZtLQ0KPiBwcGNAdmdlci5rZXJuZWwub3JnOyBw
YXVsdXNAc2FtYmEub3JnOyBCaHVzaGFuIEJoYXJhdC1SNjU3NzcNCj4gU3ViamVjdDogUmU6IFtQ
QVRDSCAzLzRdIGt2bTogcG93ZXJwYzogZGVmaW5lIGEgbGludXggcHRlIGxvb2t1cCBmdW5jdGlv
bg0KPiANCj4gT24gVHVlLCAyMDEzLTEwLTA4IGF0IDExOjMzICswNTMwLCBCaGFyYXQgQmh1c2hh
biB3cm90ZToNCj4gPiBXZSBuZWVkIHRvIHNlYXJjaCBsaW51eCAicHRlIiB0byBnZXQgInB0ZSIg
YXR0cmlidXRlcyBmb3Igc2V0dGluZyBUTEINCj4gPiBpbiBLVk0uDQo+ID4gVGhpcyBwYXRjaCBk
ZWZpbmVzIGEgbGludXhfcHRlX2xvb2t1cCgpIGZ1bmN0aW9uIGZvciBzYW1lLg0KPiA+DQo+ID4g
U2lnbmVkLW9mZi1ieTogQmhhcmF0IEJodXNoYW4gPGJoYXJhdC5iaHVzaGFuQGZyZWVzY2FsZS5j
b20+DQo+ID4gLS0tDQo+ID4gIGFyY2gvcG93ZXJwYy9pbmNsdWRlL2FzbS9wZ3RhYmxlLmggfCAg
IDM1ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrDQo+ID4gIDEgZmlsZXMgY2hh
bmdlZCwgMzUgaW5zZXJ0aW9ucygrKSwgMCBkZWxldGlvbnMoLSkNCj4gPg0KPiA+IGRpZmYgLS1n
aXQgYS9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20vcGd0YWJsZS5oDQo+ID4gYi9hcmNoL3Bvd2Vy
cGMvaW5jbHVkZS9hc20vcGd0YWJsZS5oDQo+ID4gaW5kZXggN2Q2ZWFjZi4uZmQyNmMwNCAxMDA2
NDQNCj4gPiAtLS0gYS9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20vcGd0YWJsZS5oDQo+ID4gKysr
IGIvYXJjaC9wb3dlcnBjL2luY2x1ZGUvYXNtL3BndGFibGUuaA0KPiA+IEBAIC0yMjMsNiArMjIz
LDQxIEBAIGV4dGVybiBpbnQgZ3VwX2h1Z2VwdGUocHRlX3QgKnB0ZXAsIHVuc2lnbmVkIGxvbmcN
Cj4gPiBzeiwgdW5zaWduZWQgbG9uZyBhZGRyLCAgI2VuZGlmICBwdGVfdA0KPiA+ICpmaW5kX2xp
bnV4X3B0ZV9vcl9odWdlcHRlKHBnZF90ICpwZ2RpciwgdW5zaWduZWQgbG9uZyBlYSwNCj4gPiAg
CQkJCSB1bnNpZ25lZCAqc2hpZnQpOw0KPiA+ICsNCj4gPiArc3RhdGljIGlubGluZSBwdGVfdCBs
b29rdXBfbGludXhfcHRlKHBnZF90ICpwZ2RpciwgdW5zaWduZWQgbG9uZyBodmEsDQo+ID4gKwkJ
CQkgICAgIHVuc2lnbmVkIGxvbmcgKnB0ZV9zaXplcCkNCj4gPiArew0KPiA+ICsJcHRlX3QgKnB0
ZXA7DQo+ID4gKwlwdGVfdCBwdGU7DQo+ID4gKwl1bnNpZ25lZCBsb25nIHBzID0gKnB0ZV9zaXpl
cDsNCj4gPiArCXVuc2lnbmVkIGludCBzaGlmdDsNCj4gPiArDQo+ID4gKwlwdGVwID0gZmluZF9s
aW51eF9wdGVfb3JfaHVnZXB0ZShwZ2RpciwgaHZhLCAmc2hpZnQpOw0KPiA+ICsJaWYgKCFwdGVw
KQ0KPiA+ICsJCXJldHVybiBfX3B0ZSgwKTsNCj4gPiArCWlmIChzaGlmdCkNCj4gPiArCQkqcHRl
X3NpemVwID0gMXVsIDw8IHNoaWZ0Ow0KPiA+ICsJZWxzZQ0KPiA+ICsJCSpwdGVfc2l6ZXAgPSBQ
QUdFX1NJWkU7DQo+ID4gKw0KPiA+ICsJaWYgKHBzID4gKnB0ZV9zaXplcCkNCj4gPiArCQlyZXR1
cm4gX19wdGUoMCk7DQo+ID4gKw0KPiA+ICsJLyogd2FpdCB1bnRpbCBfUEFHRV9CVVNZIGlzIGNs
ZWFyICovDQo+ID4gKwl3aGlsZSAoMSkgew0KPiA+ICsJCXB0ZSA9IHB0ZV92YWwoKnB0ZXApOw0K
PiA+ICsJCWlmICh1bmxpa2VseShwdGUgJiBfUEFHRV9CVVNZKSkgew0KPiA+ICsJCQljcHVfcmVs
YXgoKTsNCj4gPiArCQkJY29udGludWU7DQo+ID4gKwkJfQ0KPiA+ICsJfQ0KPiA+ICsNCj4gPiAr
CS8qIElmIHB0ZSBpcyBub3QgcHJlc2VudCByZXR1cm4gTm9uZSAqLw0KPiA+ICsJaWYgKHVubGlr
ZWx5KCEocHRlICYgX1BBR0VfUFJFU0VOVCkpKQ0KPiA+ICsJCXJldHVybiBfX3B0ZSgwKTsNCj4g
PiArDQo+ID4gKwlyZXR1cm4gcHRlOw0KPiA+ICt9DQo+IA0KPiBDYW4gbG9va3VwX2xpbnV4X3B0
ZV9hbmRfdXBkYXRlKCkgY2FsbCBsb29rdXBfbGludXhfcHRlKCk/DQoNCldoYXQgbG9va3VwX2xp
bnV4X3B0ZV9hbmRfdXBkYXRlKCkgZG9lczotDQogLSBmaW5kX2xpbnV4X3B0ZV9vcl9odWdlcHRl
KCkNCiAtIGRvZXMgc2l6ZSBhbmQgc29tZSBvdGhlciB0cml2aWFsIGNoZWNrcw0KIC0gVGhlbiBh
dG9taWNhbGx5IHVwZGF0ZSB0aGUgcHRlOi0NCiAgID0+IHdoaWxlKCkNCiAgID0+IHdhaXQgdGls
bCBfUEFHRV9CVVNZIGlzIGNsZWFyDQogICA9PiBhdG9taWNhbGx5IHVwZGF0ZSB0aGUgcHRlDQog
ICA9PiBpZiBub3QgdXBkYXRlZCB0aGVuIGdvIGJhY2sgdG8gd2hpbGUoKSBhYm92ZSBlbHNlIGJy
ZWFrDQoNCg0KV2hpbGUgd2hhdCBsb29rdXBfbGludXhfcHRlKCkgZG9lczotDQogLSBmaW5kX2xp
bnV4X3B0ZV9vcl9odWdlcHRlKCkNCiAtIGRvZXMgc2l6ZSBhbmQgc29tZSBvdGhlciB0cml2aWFs
IGNoZWNrcw0KIC0gd2FpdCB0aWxsIF9QQUdFX0JVU1kgaXMgY2xlYXINCiAtIHJldHVybiBwdGUN
Cg0KSSBhbSBmaW5kaW5nIGl0IGRpZmZpY3VsdCB0byBjYWxsIGxvb2t1cF9saW51eF9wdGUoKSBm
cm9tIGxvb2t1cF9saW51eF9wdGVfYW5kX3VwZGF0ZSgpLg0KDQpUaGFua3MNCi1CaGFyYXQNCg0K
PiANCj4gLVNjb3R0DQo+IA0KDQo

Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Scott Wood]

On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, October 09, 2013 3:07 AM
> > To: Bhushan Bharat-R65777
> > Cc: agraf@suse.de; Yoder Stuart-B08248; kvm@vger.kernel.org; kvm-
> > ppc@vger.kernel.org; paulus@samba.org; Bhushan Bharat-R65777
> > Subject: Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function
> > 
> > On Tue, 2013-10-08 at 11:33 +0530, Bharat Bhushan wrote:
> > > We need to search linux "pte" to get "pte" attributes for setting TLB
> > > in KVM.
> > > This patch defines a linux_pte_lookup() function for same.
> > >
> > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > > ---
> > >  arch/powerpc/include/asm/pgtable.h |   35 +++++++++++++++++++++++++++++++++++
> > >  1 files changed, 35 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/arch/powerpc/include/asm/pgtable.h
> > > b/arch/powerpc/include/asm/pgtable.h
> > > index 7d6eacf..fd26c04 100644
> > > --- a/arch/powerpc/include/asm/pgtable.h
> > > +++ b/arch/powerpc/include/asm/pgtable.h
> > > @@ -223,6 +223,41 @@ extern int gup_hugepte(pte_t *ptep, unsigned long
> > > sz, unsigned long addr,  #endif  pte_t
> > > *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea,
> > >  				 unsigned *shift);
> > > +
> > > +static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
> > > +				     unsigned long *pte_sizep)
> > > +{
> > > +	pte_t *ptep;
> > > +	pte_t pte;
> > > +	unsigned long ps = *pte_sizep;
> > > +	unsigned int shift;
> > > +
> > > +	ptep = find_linux_pte_or_hugepte(pgdir, hva, &shift);
> > > +	if (!ptep)
> > > +		return __pte(0);
> > > +	if (shift)
> > > +		*pte_sizep = 1ul << shift;
> > > +	else
> > > +		*pte_sizep = PAGE_SIZE;
> > > +
> > > +	if (ps > *pte_sizep)
> > > +		return __pte(0);
> > > +
> > > +	/* wait until _PAGE_BUSY is clear */
> > > +	while (1) {
> > > +		pte = pte_val(*ptep);
> > > +		if (unlikely(pte & _PAGE_BUSY)) {
> > > +			cpu_relax();
> > > +			continue;
> > > +		}
> > > +	}
> > > +
> > > +	/* If pte is not present return None */
> > > +	if (unlikely(!(pte & _PAGE_PRESENT)))
> > > +		return __pte(0);
> > > +
> > > +	return pte;
> > > +}
> > 
> > Can lookup_linux_pte_and_update() call lookup_linux_pte()?
> 
> What lookup_linux_pte_and_update() does:-
>  - find_linux_pte_or_hugepte()
>  - does size and some other trivial checks
>  - Then atomically update the pte:-
>    => while()
>    => wait till _PAGE_BUSY is clear
>    => atomically update the pte
>    => if not updated then go back to while() above else break
> 
> 
> While what lookup_linux_pte() does:-
>  - find_linux_pte_or_hugepte()
>  - does size and some other trivial checks
>  - wait till _PAGE_BUSY is clear
>  - return pte
> 
> I am finding it difficult to call lookup_linux_pte() from lookup_linux_pte_and_update().

You could factor out a common lookup_linux_ptep().

-Scott

Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Paul Mackerras]

On Wed, Oct 09, 2013 at 12:47:31PM -0500, Scott Wood wrote:
> On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote:
> > 
> > What lookup_linux_pte_and_update() does:-
> >  - find_linux_pte_or_hugepte()
> >  - does size and some other trivial checks
> >  - Then atomically update the pte:-
> >    => while()
> >    => wait till _PAGE_BUSY is clear
> >    => atomically update the pte
> >    => if not updated then go back to while() above else break
> > 
> > 
> > While what lookup_linux_pte() does:-
> >  - find_linux_pte_or_hugepte()
> >  - does size and some other trivial checks
> >  - wait till _PAGE_BUSY is clear
> >  - return pte
> > 
> > I am finding it difficult to call lookup_linux_pte() from lookup_linux_pte_and_update().
> 
> You could factor out a common lookup_linux_ptep().

I don't really think it's enough code to be worth wringing out the
last drop of duplication.  However, if he removed the checks for
_PAGE_BUSY and _PAGE_PRESENT as I suggested in another mail, and made
it return the pte pointer rather than the value, it would then
essentially be a lookup_linux_ptep() as you suggest.

Paul.

RE: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Paul Mackerras [mailto:paulus@samba.org]
> Sent: Thursday, October 10, 2013 4:06 PM
> To: Wood Scott-B07421
> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; agraf@suse.de; Yoder Stuart-
> B08248; kvm@vger.kernel.org; kvm-ppc@vger.kernel.org
> Subject: Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function
> 
> On Wed, Oct 09, 2013 at 12:47:31PM -0500, Scott Wood wrote:
> > On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote:
> > >
> > > What lookup_linux_pte_and_update() does:-
> > >  - find_linux_pte_or_hugepte()
> > >  - does size and some other trivial checks
> > >  - Then atomically update the pte:-
> > >    => while()
> > >    => wait till _PAGE_BUSY is clear
> > >    => atomically update the pte
> > >    => if not updated then go back to while() above else break
> > >
> > >
> > > While what lookup_linux_pte() does:-
> > >  - find_linux_pte_or_hugepte()
> > >  - does size and some other trivial checks
> > >  - wait till _PAGE_BUSY is clear
> > >  - return pte
> > >
> > > I am finding it difficult to call lookup_linux_pte() from
> lookup_linux_pte_and_update().
> >
> > You could factor out a common lookup_linux_ptep().
> 
> I don't really think it's enough code to be worth wringing out the last drop of
> duplication.  However, if he removed the checks for _PAGE_BUSY and _PAGE_PRESENT
> as I suggested in another mail, and made it return the pte pointer rather than
> the value, it would then essentially be a lookup_linux_ptep() as you suggest.

Do we want to have lookup_linux_pte() or  lookup_linux_ptep() or both where lookup_linux_pte() and lookup_linux_pte_and_update() calls lookup_linux_ptep() ?

-Bharat

> 
> Paul.

Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Paul Mackerras]

On Thu, Oct 10, 2013 at 10:44:54AM +0000, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: Paul Mackerras [mailto:paulus@samba.org]
> > Sent: Thursday, October 10, 2013 4:06 PM
> > To: Wood Scott-B07421
> > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; agraf@suse.de; Yoder Stuart-
> > B08248; kvm@vger.kernel.org; kvm-ppc@vger.kernel.org
> > Subject: Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function
> > 
> > On Wed, Oct 09, 2013 at 12:47:31PM -0500, Scott Wood wrote:
> > > On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote:
> > > >
> > > > What lookup_linux_pte_and_update() does:-
> > > >  - find_linux_pte_or_hugepte()
> > > >  - does size and some other trivial checks
> > > >  - Then atomically update the pte:-
> > > >    => while()
> > > >    => wait till _PAGE_BUSY is clear
> > > >    => atomically update the pte
> > > >    => if not updated then go back to while() above else break
> > > >
> > > >
> > > > While what lookup_linux_pte() does:-
> > > >  - find_linux_pte_or_hugepte()
> > > >  - does size and some other trivial checks
> > > >  - wait till _PAGE_BUSY is clear
> > > >  - return pte
> > > >
> > > > I am finding it difficult to call lookup_linux_pte() from
> > lookup_linux_pte_and_update().
> > >
> > > You could factor out a common lookup_linux_ptep().
> > 
> > I don't really think it's enough code to be worth wringing out the last drop of
> > duplication.  However, if he removed the checks for _PAGE_BUSY and _PAGE_PRESENT
> > as I suggested in another mail, and made it return the pte pointer rather than
> > the value, it would then essentially be a lookup_linux_ptep() as you suggest.
> 
> Do we want to have lookup_linux_pte() or  lookup_linux_ptep() or both where lookup_linux_pte() and lookup_linux_pte_and_update() calls lookup_linux_ptep() ?

I don't think we want both lookup_linux_pte() and lookup_linux_ptep().
Either have a lookup_linux_ptep() and call it from your book E code
and from lookup_linux_pte_and_update(), or keep a separate
lookup_linux_pte() and don't make the _and_update() version call it.
I don't really care; you decide which looks nicer in the end.  Of
course someone will then tell you to do it the other way, but at that
point it's just a matter of taste.

Paul.

Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Aneesh Kumar K.V]

Paul Mackerras <paulus@samba.org> writes:

> On Wed, Oct 09, 2013 at 12:47:31PM -0500, Scott Wood wrote:
>> On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote:
>> > 
>> > What lookup_linux_pte_and_update() does:-
>> >  - find_linux_pte_or_hugepte()
>> >  - does size and some other trivial checks
>> >  - Then atomically update the pte:-
>> >    => while()
>> >    => wait till _PAGE_BUSY is clear
>> >    => atomically update the pte
>> >    => if not updated then go back to while() above else break
>> > 
>> > 
>> > While what lookup_linux_pte() does:-
>> >  - find_linux_pte_or_hugepte()
>> >  - does size and some other trivial checks
>> >  - wait till _PAGE_BUSY is clear
>> >  - return pte
>> > 
>> > I am finding it difficult to call lookup_linux_pte() from lookup_linux_pte_and_update().
>> 
>> You could factor out a common lookup_linux_ptep().
>
> I don't really think it's enough code to be worth wringing out the
> last drop of duplication.  However, if he removed the checks for
> _PAGE_BUSY and _PAGE_PRESENT as I suggested in another mail, and made
> it return the pte pointer rather than the value, it would then
> essentially be a lookup_linux_ptep() as you suggest.

We also need to check for _PAGE_SPLITTING before going ahead and using
the pte value

-aneesh

Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Paul Mackerras]

On Tue, Oct 08, 2013 at 11:33:31AM +0530, Bharat Bhushan wrote:
> We need to search linux "pte" to get "pte" attributes for
> setting TLB in KVM.
> This patch defines a linux_pte_lookup() function for same.
> 

[snip]

> +	/* wait until _PAGE_BUSY is clear */
> +	while (1) {
> +		pte = pte_val(*ptep);
> +		if (unlikely(pte & _PAGE_BUSY)) {
> +			cpu_relax();
> +			continue;
> +		}
> +	}
> +
> +	/* If pte is not present return None */
> +	if (unlikely(!(pte & _PAGE_PRESENT)))
> +		return __pte(0);

First, this looks racy to me, since nothing stops the compiler
refetching pte from memory, and it might have become busy again in the
meantime.

However, I don't think you need to do any of this, since the caller in
your next patch checks for pte_present(pte) anyway.  On book E systems
we don't use _PAGE_BUSY, so you don't need the loop for your intended
application.  I suggest you remove the entire section quoted above.

Paul.

Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function

[Aneesh Kumar K.V]

Bharat Bhushan <r65777@freescale.com> writes:

> We need to search linux "pte" to get "pte" attributes for
> setting TLB in KVM.
> This patch defines a linux_pte_lookup() function for same.
>
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
>  arch/powerpc/include/asm/pgtable.h |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
> index 7d6eacf..fd26c04 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -223,6 +223,41 @@ extern int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
>  #endif
>  pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea,
>  				 unsigned *shift);
> +
> +static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
> +				     unsigned long *pte_sizep)
> +{
> +	pte_t *ptep;
> +	pte_t pte;
> +	unsigned long ps = *pte_sizep;
> +	unsigned int shift;
> +
> +	ptep = find_linux_pte_or_hugepte(pgdir, hva, &shift);
> +	if (!ptep)
> +		return __pte(0);
> +	if (shift)
> +		*pte_sizep = 1ul << shift;
> +	else
> +		*pte_sizep = PAGE_SIZE;
> +
> +	if (ps > *pte_sizep)
> +		return __pte(0);
> +
> +	/* wait until _PAGE_BUSY is clear */
> +	while (1) {
> +		pte = pte_val(*ptep);
> +		if (unlikely(pte & _PAGE_BUSY)) {
> +			cpu_relax();
> +			continue;
> +		}

What if we find a THP page that is splitting ?  Older function returned
__pte(0) in that case.


> +	}
> +
> +	/* If pte is not present return None */
> +	if (unlikely(!(pte & _PAGE_PRESENT)))
> +		return __pte(0);
> +
> +	return pte;
> +}
>  #endif /* __ASSEMBLY__ */
>  
>  #endif /* __KERNEL__ */
> -- 

-aneesh

[PATCH 4/4] kvm: powerpc: use caching attributes as per linux pte

[Bharat Bhushan]

KVM uses same WIM tlb attributes as the corresponding qemu pte.
For this we now search the linux pte for the requested page and
get these cache caching/coherency attributes from pte.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/kvm_host.h |    2 +-
 arch/powerpc/kvm/booke.c            |    2 +-
 arch/powerpc/kvm/e500.h             |    8 ++++--
 arch/powerpc/kvm/e500_mmu_host.c    |   39 ++++++++++++++++++++--------------
 4 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 802984e..3546b72 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -558,6 +558,7 @@ struct kvm_vcpu_arch {
 #endif
 	gpa_t paddr_accessed;
 	gva_t vaddr_accessed;
+	pgd_t *pgdir;
 
 	u8 io_gpr; /* GPR used as IO source/target */
 	u8 mmio_is_bigendian;
@@ -615,7 +616,6 @@ struct kvm_vcpu_arch {
 	struct list_head run_list;
 	struct task_struct *run_task;
 	struct kvm_run *kvm_run;
-	pgd_t *pgdir;
 
 	spinlock_t vpa_update_lock;
 	struct kvmppc_vpa vpa;
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 65fa775..0d79bf9 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -695,7 +695,7 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 
 	kvmppc_load_guest_fp(vcpu);
 #endif
-
+	vcpu->arch.pgdir = current->mm->pgd;
 	kvmppc_fix_ee_before_entry();
 
 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
diff --git a/arch/powerpc/kvm/e500.h b/arch/powerpc/kvm/e500.h
index 4fd9650..a326178 100644
--- a/arch/powerpc/kvm/e500.h
+++ b/arch/powerpc/kvm/e500.h
@@ -31,11 +31,13 @@ enum vcpu_ftr {
 #define E500_TLB_NUM   2
 
 /* entry is mapped somewhere in host TLB */
-#define E500_TLB_VALID		(1 << 0)
+#define E500_TLB_VALID		(1 << 31)
 /* TLB1 entry is mapped by host TLB1, tracked by bitmaps */
-#define E500_TLB_BITMAP		(1 << 1)
+#define E500_TLB_BITMAP		(1 << 30)
 /* TLB1 entry is mapped by host TLB0 */
-#define E500_TLB_TLB0		(1 << 2)
+#define E500_TLB_TLB0		(1 << 29)
+/* bits [6-5] MAS2_X1 and MAS2_X0 and [4-0] bits for WIMGE */
+#define E500_TLB_MAS2_ATTR	(0x7f)
 
 struct tlbe_ref {
 	pfn_t pfn;		/* valid only for TLB0, except briefly */
diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
index 7a41a93..1bb2627 100644
--- a/arch/powerpc/kvm/e500_mmu_host.c
+++ b/arch/powerpc/kvm/e500_mmu_host.c
@@ -64,15 +64,6 @@ static inline u32 e500_shadow_mas3_attrib(u32 mas3, int usermode)
 	return mas3;
 }
 
-static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
-{
-#ifdef CONFIG_SMP
-	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
-#else
-	return mas2 & MAS2_ATTRIB_MASK;
-#endif
-}
-
 /*
  * writing shadow tlb entry to host TLB
  */
@@ -248,11 +239,14 @@ static inline int tlbe_is_writable(struct kvm_book3e_206_tlb_entry *tlbe)
 
 static inline void kvmppc_e500_ref_setup(struct tlbe_ref *ref,
 					 struct kvm_book3e_206_tlb_entry *gtlbe,
-					 pfn_t pfn)
+					 pfn_t pfn, unsigned int wimg)
 {
 	ref->pfn = pfn;
 	ref->flags = E500_TLB_VALID;
 
+	/* Use guest supplied MAS2_G and MAS2_E */
+	ref->flags |= (gtlbe->mas2 & MAS2_ATTRIB_MASK) | wimg;
+
 	if (tlbe_is_writable(gtlbe))
 		kvm_set_pfn_dirty(pfn);
 }
@@ -312,8 +306,7 @@ static void kvmppc_e500_setup_stlbe(
 
 	/* Force IPROT=0 for all guest mappings. */
 	stlbe->mas1 = MAS1_TSIZE(tsize) | get_tlb_sts(gtlbe) | MAS1_VALID;
-	stlbe->mas2 = (gvaddr & MAS2_EPN) |
-		      e500_shadow_mas2_attrib(gtlbe->mas2, pr);
+	stlbe->mas2 = (gvaddr & MAS2_EPN) | (ref->flags & E500_TLB_MAS2_ATTR);
 	stlbe->mas7_3 = ((u64)pfn << PAGE_SHIFT) |
 			e500_shadow_mas3_attrib(gtlbe->mas7_3, pr);
 
@@ -332,6 +325,10 @@ static inline int kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
 	unsigned long hva;
 	int pfnmap = 0;
 	int tsize = BOOK3E_PAGESZ_4K;
+	unsigned long tsize_pages = 0;
+	pte_t pte;
+	unsigned int wimg = 0;
+	pgd_t *pgdir;
 
 	/*
 	 * Translate guest physical to true physical, acquiring
@@ -394,7 +391,7 @@ static inline int kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
 			 */
 
 			for (; tsize > BOOK3E_PAGESZ_4K; tsize -= 2) {
-				unsigned long gfn_start, gfn_end, tsize_pages;
+				unsigned long gfn_start, gfn_end;
 				tsize_pages = 1 << (tsize - 2);
 
 				gfn_start = gfn & ~(tsize_pages - 1);
@@ -436,9 +433,9 @@ static inline int kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
 	}
 
 	if (likely(!pfnmap)) {
-		unsigned long tsize_pages = 1 << (tsize + 10 - PAGE_SHIFT);
+		tsize_pages = 1 << (tsize + 10 - PAGE_SHIFT);
 		pfn = gfn_to_pfn_memslot(slot, gfn);
-		if (is_error_noslot_pfn(pfn)) {
+		if (is_error_noslot_pfn(pfn) && printk_ratelimit()) {
 			printk(KERN_ERR "Couldn't get real page for gfn %lx!\n",
 					(long)gfn);
 			return -EINVAL;
@@ -449,7 +446,17 @@ static inline int kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
 		gvaddr &= ~((tsize_pages << PAGE_SHIFT) - 1);
 	}
 
-	kvmppc_e500_ref_setup(ref, gtlbe, pfn);
+
+	pgdir = vcpu_e500->vcpu.arch.pgdir;
+	pte = lookup_linux_pte(pgdir, hva, &tsize_pages);
+	if (pte_present(pte))
+		wimg = (pte >> PTE_WIMGE_SHIFT) & MAS2_WIMGE_MASK;
+	else if (printk_ratelimit()) {
+		printk(KERN_ERR "%s: pte not present: gfn %lx, pfn %lx\n",
+				__func__, (long)gfn, pfn);
+		return -EINVAL;
+	}
+	kvmppc_e500_ref_setup(ref, gtlbe, pfn, wimg);
 
 	kvmppc_e500_setup_stlbe(&vcpu_e500->vcpu, gtlbe, tsize,
 				ref, gvaddr, stlbe);
-- 
1.7.0.4

[PATCH 0/4] kvm: powerpc: use cache attributes from linux pte

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

v1->v2
 - Removed _PAGE_BUSY loop as suggested by PaulS.
 - Added check for PAGE_SPLITTING

kvm: powerpc: use cache attributes from linux pte
 - 1st Patch fixes a bug in booke (detail in patch)
 - 2nd patch is renaming the linux_pte_lookup_function() just for clarity.
   There is not functional change.
 - 3nd Patch adds a Linux pte lookup function.
 - 4th Patch uses the above defined function and setup TLB.wimg accordingly


Bharat Bhushan (4):
  kvm: booke: clear host tlb reference flag on guest tlb invalidation
  kvm: book3s: rename lookup_linux_pte() to
    lookup_linux_pte_and_update()
  kvm: powerpc: define a linux pte lookup function
  kvm: powerpc: use caching attributes as per linux pte

 arch/powerpc/include/asm/kvm_host.h |    2 +-
 arch/powerpc/include/asm/pgtable.h  |   27 +++++++++++++++++
 arch/powerpc/kvm/book3s_hv_rm_mmu.c |    8 +++--
 arch/powerpc/kvm/booke.c            |    1 +
 arch/powerpc/kvm/e500.h             |    8 +++--
 arch/powerpc/kvm/e500_mmu_host.c    |   55 +++++++++++++++++++---------------
 6 files changed, 70 insertions(+), 31 deletions(-)