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

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

[Bharat Bhushan]

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

First patch is a typo fix where book3e define _PAGE_LENDIAN while it
should be defined as _PAGE_ENDIAN. This seems to show that this is never exercised :-)

Second and third patch is to allow guest controlling "G"-Guarded and "E"-Endian TLB attributes respectively.

Fourth patch is moving functions/logic in common code so they can be used on booke also.

Fifth and Sixth patch is actually setting caching attributes (TLB.WIMGE) using corresponding Linux pte.

v3->v5
 - Fix tlb-reference-flag clearing issue (patch 4/6)
 - There was a patch (4/6 powerpc: move linux pte/hugepte search to more generic file)
   in the last series of this patchset which was moving pte/hugepte search functions to
   generic file. That patch is no more needed as some other patch is already applied to fix that :)

v2->v3
 - now lookup_linux_pte() only have pte search logic and it does not
   set any access flags in pte. There is already a function for setting
   access flag which will be called explicitly where needed.
   On booke we only need to search for pte to get WIMG.

v1->v2
 - Earlier caching attributes (WIMGE) were set based of page is RAM or not
   But now we get these attributes from corresponding Linux PTE.

Bharat Bhushan (6):
  powerpc: book3e: _PAGE_LENDIAN must be _PAGE_ENDIAN
  kvm: powerpc: allow guest control "E" attribute in mas2
  kvm: powerpc: allow guest control "G" attribute in mas2
  kvm: powerpc: keep only pte search logic in lookup_linux_pte
  kvm: booke: clear host tlb reference flag on guest tlb invalidation
  kvm: powerpc: use caching attributes as per linux pte

 arch/powerpc/include/asm/kvm_host.h   |    2 +-
 arch/powerpc/include/asm/pgtable.h    |   24 ++++++++++++++++
 arch/powerpc/include/asm/pte-book3e.h |    2 +-
 arch/powerpc/kvm/book3s_hv_rm_mmu.c   |   36 ++++++++----------------
 arch/powerpc/kvm/booke.c              |    2 +-
 arch/powerpc/kvm/e500.h               |   10 ++++--
 arch/powerpc/kvm/e500_mmu_host.c      |   50 +++++++++++++++++++--------------
 7 files changed, 74 insertions(+), 52 deletions(-)

[PATCH 1/6 v5] powerpc: book3e: _PAGE_LENDIAN must be _PAGE_ENDIAN

[Bharat Bhushan]

For booke3e _PAGE_ENDIAN is not defined. Infact what is defined
is "_PAGE_LENDIAN" which is wrong and that should be _PAGE_ENDIAN.
There are no compilation errors as
arch/powerpc/include/asm/pte-common.h defines _PAGE_ENDIAN to 0
as it is not defined anywhere.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v1->v5
 - no change

 arch/powerpc/include/asm/pte-book3e.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/pte-book3e.h b/arch/powerpc/include/asm/pte-book3e.h
index 0156702..576ad88 100644
--- a/arch/powerpc/include/asm/pte-book3e.h
+++ b/arch/powerpc/include/asm/pte-book3e.h
@@ -40,7 +40,7 @@
 #define _PAGE_U1	0x010000
 #define _PAGE_U0	0x020000
 #define _PAGE_ACCESSED	0x040000
-#define _PAGE_LENDIAN	0x080000
+#define _PAGE_ENDIAN	0x080000
 #define _PAGE_GUARDED	0x100000
 #define _PAGE_COHERENT	0x200000 /* M: enforce memory coherence */
 #define _PAGE_NO_CACHE	0x400000 /* I: cache inhibit */
-- 
1.7.0.4

[PATCH 2/6 v5] kvm: powerpc: allow guest control "E" attribute in mas2

[Bharat Bhushan]

"E" bit in MAS2 bit indicates whether the page is accessed
in Little-Endian or Big-Endian byte order.
There is no reason to stop guest setting  "E", so allow him."

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v1->v5
 - no change
 arch/powerpc/kvm/e500.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kvm/e500.h b/arch/powerpc/kvm/e500.h
index c2e5e98..277cb18 100644
--- a/arch/powerpc/kvm/e500.h
+++ b/arch/powerpc/kvm/e500.h
@@ -117,7 +117,7 @@ static inline struct kvmppc_vcpu_e500 *to_e500(struct kvm_vcpu *vcpu)
 #define E500_TLB_USER_PERM_MASK (MAS3_UX|MAS3_UR|MAS3_UW)
 #define E500_TLB_SUPER_PERM_MASK (MAS3_SX|MAS3_SR|MAS3_SW)
 #define MAS2_ATTRIB_MASK \
-	  (MAS2_X0 | MAS2_X1)
+	  (MAS2_X0 | MAS2_X1 | MAS2_E)
 #define MAS3_ATTRIB_MASK \
 	  (MAS3_U0 | MAS3_U1 | MAS3_U2 | MAS3_U3 \
 	   | E500_TLB_USER_PERM_MASK | E500_TLB_SUPER_PERM_MASK)
-- 
1.7.0.4

[PATCH 3/6 v5] kvm: powerpc: allow guest control "G" attribute in mas2

[Bharat Bhushan]

"G" bit in MAS2 indicates whether the page is Guarded.
There is no reason to stop guest setting  "G", so allow him.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v1->v5
 - no change
 arch/powerpc/kvm/e500.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kvm/e500.h b/arch/powerpc/kvm/e500.h
index 277cb18..4fd9650 100644
--- a/arch/powerpc/kvm/e500.h
+++ b/arch/powerpc/kvm/e500.h
@@ -117,7 +117,7 @@ static inline struct kvmppc_vcpu_e500 *to_e500(struct kvm_vcpu *vcpu)
 #define E500_TLB_USER_PERM_MASK (MAS3_UX|MAS3_UR|MAS3_UW)
 #define E500_TLB_SUPER_PERM_MASK (MAS3_SX|MAS3_SR|MAS3_SW)
 #define MAS2_ATTRIB_MASK \
-	  (MAS2_X0 | MAS2_X1 | MAS2_E)
+	  (MAS2_X0 | MAS2_X1 | MAS2_E | MAS2_G)
 #define MAS3_ATTRIB_MASK \
 	  (MAS3_U0 | MAS3_U1 | MAS3_U2 | MAS3_U3 \
 	   | E500_TLB_USER_PERM_MASK | E500_TLB_SUPER_PERM_MASK)
-- 
1.7.0.4

[PATCH 4/6 v5] kvm: powerpc: keep only pte search logic in lookup_linux_pte

[Bharat Bhushan]

lookup_linux_pte() was searching for a pte and also sets access
flags is writable. This function now searches only pte while
access flag setting is done explicitly.

This pte lookup is not kvm specific, so moved to common code (asm/pgtable.h)
My Followup patch will use this on booke.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v4->v5
 - No change

 arch/powerpc/include/asm/pgtable.h  |   24 +++++++++++++++++++++++
 arch/powerpc/kvm/book3s_hv_rm_mmu.c |   36 +++++++++++-----------------------
 2 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 7d6eacf..3a5de5c 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -223,6 +223,30 @@ 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;
+	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);
+
+	if (!pte_present(*ptep))
+		return __pte(0);
+
+	return ptep;
+}
 #endif /* __ASSEMBLY__ */
 
 #endif /* __KERNEL__ */
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 45e30d6..74fa7f8 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -134,25 +134,6 @@ 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,
-			      int writing, unsigned long *pte_sizep)
-{
-	pte_t *ptep;
-	unsigned long ps = *pte_sizep;
-	unsigned int hugepage_shift;
-
-	ptep = find_linux_pte_or_hugepte(pgdir, hva, &hugepage_shift);
-	if (!ptep)
-		return __pte(0);
-	if (hugepage_shift)
-		*pte_sizep = 1ul << hugepage_shift;
-	else
-		*pte_sizep = PAGE_SIZE;
-	if (ps > *pte_sizep)
-		return __pte(0);
-	return kvmppc_read_update_linux_pte(ptep, writing, hugepage_shift);
-}
-
 static inline void unlock_hpte(unsigned long *hpte, unsigned long hpte_v)
 {
 	asm volatile(PPC_RELEASE_BARRIER "" : : : "memory");
@@ -173,6 +154,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
 	unsigned long is_io;
 	unsigned long *rmap;
 	pte_t pte;
+	pte_t *ptep;
 	unsigned int writing;
 	unsigned long mmu_seq;
 	unsigned long rcbits;
@@ -231,8 +213,9 @@ 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);
-		if (pte_present(pte)) {
+		ptep = lookup_linux_pte(pgdir, hva, &pte_size);
+		if (pte_present(pte_val(*ptep))) {
+			pte = kvmppc_read_update_linux_pte(ptep, writing);
 			if (writing && !pte_write(pte))
 				/* make the actual HPTE be read-only */
 				ptel = hpte_make_readonly(ptel);
@@ -661,15 +644,20 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
 			struct kvm_memory_slot *memslot;
 			pgd_t *pgdir = vcpu->arch.pgdir;
 			pte_t pte;
+			pte_t *ptep;
 
 			psize = hpte_page_size(v, r);
 			gfn = ((r & HPTE_R_RPN) & ~(psize - 1)) >> PAGE_SHIFT;
 			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);
-				if (pte_present(pte) && !pte_write(pte))
-					r = hpte_make_readonly(r);
+				ptep = lookup_linux_pte(pgdir, hva, &psize);
+				if (pte_present(pte_val(*ptep))) {
+					pte = kvmppc_read_update_linux_pte(ptep,
+									   1);
+					if (pte_present(pte) && !pte_write(pte))
+						r = hpte_make_readonly(r);
+				}
 			}
 		}
 	}
-- 
1.7.0.4

Re: [PATCH 4/6 v5] kvm: powerpc: keep only pte search logic in lookup_linux_pte

[Alexander Graf]

On 19.09.2013, at 08:02, Bharat Bhushan wrote:

> lookup_linux_pte() was searching for a pte and also sets access
> flags is writable. This function now searches only pte while
> access flag setting is done explicitly.
>=20
> This pte lookup is not kvm specific, so moved to common code =
(asm/pgtable.h)
> My Followup patch will use this on booke.
>=20
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> v4->v5
> - No change
>=20
> arch/powerpc/include/asm/pgtable.h  |   24 +++++++++++++++++++++++
> arch/powerpc/kvm/book3s_hv_rm_mmu.c |   36 =
+++++++++++-----------------------
> 2 files changed, 36 insertions(+), 24 deletions(-)
>=20
> diff --git a/arch/powerpc/include/asm/pgtable.h =
b/arch/powerpc/include/asm/pgtable.h
> index 7d6eacf..3a5de5c 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -223,6 +223,30 @@ 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;
> +	unsigned long ps =3D *pte_sizep;
> +	unsigned int shift;
> +
> +	ptep =3D find_linux_pte_or_hugepte(pgdir, hva, &shift);
> +	if (!ptep)
> +		return __pte(0);

This returns a struct pte_t, but your return value of the function is a =
struct pte_t *. So this code will fail compiling with =
STRICT_MM_TYPECHECKS set. Any reason you don't just return NULL here?

That way callers could simply check on if (ptep) ... or you leave the =
return value as struct pte_t.


Alex

> +	if (shift)
> +		*pte_sizep =3D 1ul << shift;
> +	else
> +		*pte_sizep =3D PAGE_SIZE;
> +
> +	if (ps > *pte_sizep)
> +		return __pte(0);
> +
> +	if (!pte_present(*ptep))
> +		return __pte(0);

> +
> +	return ptep;
> +}
> #endif /* __ASSEMBLY__ */
>=20
> #endif /* __KERNEL__ */
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c =
b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index 45e30d6..74fa7f8 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -134,25 +134,6 @@ static void remove_revmap_chain(struct kvm *kvm, =
long pte_index,
> 	unlock_rmap(rmap);
> }
>=20
> -static pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
> -			      int writing, unsigned long *pte_sizep)
> -{
> -	pte_t *ptep;
> -	unsigned long ps =3D *pte_sizep;
> -	unsigned int hugepage_shift;
> -
> -	ptep =3D find_linux_pte_or_hugepte(pgdir, hva, &hugepage_shift);
> -	if (!ptep)
> -		return __pte(0);
> -	if (hugepage_shift)
> -		*pte_sizep =3D 1ul << hugepage_shift;
> -	else
> -		*pte_sizep =3D PAGE_SIZE;
> -	if (ps > *pte_sizep)
> -		return __pte(0);
> -	return kvmppc_read_update_linux_pte(ptep, writing, =
hugepage_shift);
> -}
> -
> static inline void unlock_hpte(unsigned long *hpte, unsigned long =
hpte_v)
> {
> 	asm volatile(PPC_RELEASE_BARRIER "" : : : "memory");
> @@ -173,6 +154,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned =
long flags,
> 	unsigned long is_io;
> 	unsigned long *rmap;
> 	pte_t pte;
> +	pte_t *ptep;
> 	unsigned int writing;
> 	unsigned long mmu_seq;
> 	unsigned long rcbits;
> @@ -231,8 +213,9 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned =
long flags,
>=20
> 		/* Look up the Linux PTE for the backing page */
> 		pte_size =3D psize;
> -		pte =3D lookup_linux_pte(pgdir, hva, writing, =
&pte_size);
> -		if (pte_present(pte)) {
> +		ptep =3D lookup_linux_pte(pgdir, hva, &pte_size);
> +		if (pte_present(pte_val(*ptep))) {
> +			pte =3D kvmppc_read_update_linux_pte(ptep, =
writing);
> 			if (writing && !pte_write(pte))
> 				/* make the actual HPTE be read-only */
> 				ptel =3D hpte_make_readonly(ptel);
> @@ -661,15 +644,20 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, =
unsigned long flags,
> 			struct kvm_memory_slot *memslot;
> 			pgd_t *pgdir =3D vcpu->arch.pgdir;
> 			pte_t pte;
> +			pte_t *ptep;
>=20
> 			psize =3D hpte_page_size(v, r);
> 			gfn =3D ((r & HPTE_R_RPN) & ~(psize - 1)) >> =
PAGE_SHIFT;
> 			memslot =3D __gfn_to_memslot(kvm_memslots(kvm), =
gfn);
> 			if (memslot) {
> 				hva =3D __gfn_to_hva_memslot(memslot, =
gfn);
> -				pte =3D lookup_linux_pte(pgdir, hva, 1, =
&psize);
> -				if (pte_present(pte) && !pte_write(pte))
> -					r =3D hpte_make_readonly(r);
> +				ptep =3D lookup_linux_pte(pgdir, hva, =
&psize);
> +				if (pte_present(pte_val(*ptep))) {
> +					pte =3D =
kvmppc_read_update_linux_pte(ptep,
> +									 =
  1);
> +					if (pte_present(pte) && =
!pte_write(pte))
> +						r =3D =
hpte_make_readonly(r);
> +				}
> 			}
> 		}
> 	}
> --=20
> 1.7.0.4
>=20
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/6 v5] kvm: powerpc: keep only pte search logic in lookup_linux_pte

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Friday, October 04, 2013 6:57 PM
> To: Bhushan Bharat-R65777
> Cc: benh@kernel.crashing.org; paulus@samba.org; kvm@vger.kernel.org; kvm-
> ppc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Bh=
ushan
> Bharat-R65777
> Subject: Re: [PATCH 4/6 v5] kvm: powerpc: keep only pte search logic in
> lookup_linux_pte
>=20
>=20
> On 19.09.2013, at 08:02, Bharat Bhushan wrote:
>=20
> > lookup_linux_pte() was searching for a pte and also sets access flags
> > is writable. This function now searches only pte while access flag
> > setting is done explicitly.
> >
> > This pte lookup is not kvm specific, so moved to common code
> > (asm/pgtable.h) My Followup patch will use this on booke.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > v4->v5
> > - No change
> >
> > arch/powerpc/include/asm/pgtable.h  |   24 +++++++++++++++++++++++
> > arch/powerpc/kvm/book3s_hv_rm_mmu.c |   36 +++++++++++-----------------=
------
> > 2 files changed, 36 insertions(+), 24 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/pgtable.h
> > b/arch/powerpc/include/asm/pgtable.h
> > index 7d6eacf..3a5de5c 100644
> > --- a/arch/powerpc/include/asm/pgtable.h
> > +++ b/arch/powerpc/include/asm/pgtable.h
> > @@ -223,6 +223,30 @@ 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;
> > +	unsigned long ps =3D *pte_sizep;
> > +	unsigned int shift;
> > +
> > +	ptep =3D find_linux_pte_or_hugepte(pgdir, hva, &shift);
> > +	if (!ptep)
> > +		return __pte(0);
>=20
> This returns a struct pte_t, but your return value of the function is a s=
truct
> pte_t *. So this code will fail compiling with STRICT_MM_TYPECHECKS set. =
Any
> reason you don't just return NULL here?

I want to return the ptep (pte pointer) , so yes this should be NULL.
Will correct this.

Thanks
-Bharat

>=20
> That way callers could simply check on if (ptep) ... or you leave the ret=
urn
> value as struct pte_t.
>=20
>=20
> Alex
>=20
> > +	if (shift)
> > +		*pte_sizep =3D 1ul << shift;
> > +	else
> > +		*pte_sizep =3D PAGE_SIZE;
> > +
> > +	if (ps > *pte_sizep)
> > +		return __pte(0);
> > +
> > +	if (!pte_present(*ptep))
> > +		return __pte(0);
>=20
> > +
> > +	return ptep;
> > +}
> > #endif /* __ASSEMBLY__ */
> >
> > #endif /* __KERNEL__ */
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index 45e30d6..74fa7f8 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -134,25 +134,6 @@ static void remove_revmap_chain(struct kvm *kvm, l=
ong
> pte_index,
> > 	unlock_rmap(rmap);
> > }
> >
> > -static pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
> > -			      int writing, unsigned long *pte_sizep)
> > -{
> > -	pte_t *ptep;
> > -	unsigned long ps =3D *pte_sizep;
> > -	unsigned int hugepage_shift;
> > -
> > -	ptep =3D find_linux_pte_or_hugepte(pgdir, hva, &hugepage_shift);
> > -	if (!ptep)
> > -		return __pte(0);
> > -	if (hugepage_shift)
> > -		*pte_sizep =3D 1ul << hugepage_shift;
> > -	else
> > -		*pte_sizep =3D PAGE_SIZE;
> > -	if (ps > *pte_sizep)
> > -		return __pte(0);
> > -	return kvmppc_read_update_linux_pte(ptep, writing, hugepage_shift);
> > -}
> > -
> > static inline void unlock_hpte(unsigned long *hpte, unsigned long
> > hpte_v) {
> > 	asm volatile(PPC_RELEASE_BARRIER "" : : : "memory"); @@ -173,6 +154,7
> > @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
> > 	unsigned long is_io;
> > 	unsigned long *rmap;
> > 	pte_t pte;
> > +	pte_t *ptep;
> > 	unsigned int writing;
> > 	unsigned long mmu_seq;
> > 	unsigned long rcbits;
> > @@ -231,8 +213,9 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned
> > long flags,
> >
> > 		/* Look up the Linux PTE for the backing page */
> > 		pte_size =3D psize;
> > -		pte =3D lookup_linux_pte(pgdir, hva, writing, &pte_size);
> > -		if (pte_present(pte)) {
> > +		ptep =3D lookup_linux_pte(pgdir, hva, &pte_size);
> > +		if (pte_present(pte_val(*ptep))) {
> > +			pte =3D kvmppc_read_update_linux_pte(ptep, writing);
> > 			if (writing && !pte_write(pte))
> > 				/* make the actual HPTE be read-only */
> > 				ptel =3D hpte_make_readonly(ptel);
> > @@ -661,15 +644,20 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsi=
gned
> long flags,
> > 			struct kvm_memory_slot *memslot;
> > 			pgd_t *pgdir =3D vcpu->arch.pgdir;
> > 			pte_t pte;
> > +			pte_t *ptep;
> >
> > 			psize =3D hpte_page_size(v, r);
> > 			gfn =3D ((r & HPTE_R_RPN) & ~(psize - 1)) >> PAGE_SHIFT;
> > 			memslot =3D __gfn_to_memslot(kvm_memslots(kvm), gfn);
> > 			if (memslot) {
> > 				hva =3D __gfn_to_hva_memslot(memslot, gfn);
> > -				pte =3D lookup_linux_pte(pgdir, hva, 1, &psize);
> > -				if (pte_present(pte) && !pte_write(pte))
> > -					r =3D hpte_make_readonly(r);
> > +				ptep =3D lookup_linux_pte(pgdir, hva, &psize);
> > +				if (pte_present(pte_val(*ptep))) {
> > +					pte =3D kvmppc_read_update_linux_pte(ptep,
> > +									   1);
> > +					if (pte_present(pte) && !pte_write(pte))
> > +						r =3D hpte_make_readonly(r);
> > +				}
> > 			}
> > 		}
> > 	}
> > --
> > 1.7.0.4
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> > the body of a message to majordomo@vger.kernel.org More majordomo info
> > at  http://vger.kernel.org/majordomo-info.html
>=20

[PATCH 5/6 v5] 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>
---
v3-> v5
 - New patch (found this issue when doing vfio-pci development)

 arch/powerpc/kvm/e500_mmu_host.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
index 1c6a9d7..60f5a3c 100644
--- a/arch/powerpc/kvm/e500_mmu_host.c
+++ b/arch/powerpc/kvm/e500_mmu_host.c
@@ -217,7 +217,8 @@ void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 *vcpu_e500, int tlbsel,
 		}
 		mb();
 		vcpu_e500->g2h_tlb1_map[esel] = 0;
-		ref->flags &= ~(E500_TLB_BITMAP | E500_TLB_VALID);
+		/* Clear flags as TLB is not backed by the host anymore */
+		ref->flags = 0;
 		local_irq_restore(flags);
 	}
 
@@ -227,7 +228,8 @@ void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 *vcpu_e500, int tlbsel,
 		 * rarely and is not worth optimizing. Invalidate everything.
 		 */
 		kvmppc_e500_tlbil_all(vcpu_e500);
-		ref->flags &= ~(E500_TLB_TLB0 | E500_TLB_VALID);
+		/* Clear flags as TLB is not backed by the host anymore */
+		ref->flags = 0;
 	}
 
 	/* Already invalidated in between */
@@ -237,8 +239,8 @@ void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 *vcpu_e500, int tlbsel,
 	/* Guest tlbe is backed by at most one host tlbe per shadow pid. */
 	kvmppc_e500_tlbil_one(vcpu_e500, gtlbe);
 
-	/* Mark the TLB as not backed by the host anymore */
-	ref->flags &= ~E500_TLB_VALID;
+	/* Clear flags as TLB is not backed by the host anymore */
+	ref->flags = 0;
 }
 
 static inline int tlbe_is_writable(struct kvm_book3e_206_tlb_entry *tlbe)
@@ -251,7 +253,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

Re: [PATCH 5/6 v5] kvm: booke: clear host tlb reference flag on guest tlb invalidation

[Scott Wood]

On Thu, 2013-09-19 at 11:32 +0530, Bharat Bhushan wrote:
> 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>
> ---
> v3-> v5
>  - New patch (found this issue when doing vfio-pci development)
> 
>  arch/powerpc/kvm/e500_mmu_host.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
> index 1c6a9d7..60f5a3c 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -217,7 +217,8 @@ void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 *vcpu_e500, int tlbsel,
>  		}
>  		mb();
>  		vcpu_e500->g2h_tlb1_map[esel] = 0;
> -		ref->flags &= ~(E500_TLB_BITMAP | E500_TLB_VALID);
> +		/* Clear flags as TLB is not backed by the host anymore */
> +		ref->flags = 0;
>  		local_irq_restore(flags);
>  	}

This breaks when you have both E500_TLB_BITMAP and E500_TLB_TLB0 set.

Instead, just convert the final E500_TLB_VALID clearing at the end into
ref->flags = 0, and convert the early return a few lines earlier into
conditional execution of the tlbil_one().

-Scott

RE: [PATCH 5/6 v5] kvm: booke: clear host tlb reference flag on guest tlb invalidation

[Bhushan Bharat-R65777]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogRnJpZGF5LCBTZXB0ZW1iZXIgMjAsIDIwMTMgMjozOCBBTQ0KPiBUbzogQmh1
c2hhbiBCaGFyYXQtUjY1Nzc3DQo+IENjOiBiZW5oQGtlcm5lbC5jcmFzaGluZy5vcmc7IGFncmFm
QHN1c2UuZGU7IHBhdWx1c0BzYW1iYS5vcmc7DQo+IGt2bUB2Z2VyLmtlcm5lbC5vcmc7IGt2bS1w
cGNAdmdlci5rZXJuZWwub3JnOyBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZzsNCj4gQmh1
c2hhbiBCaGFyYXQtUjY1Nzc3DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggNS82IHY1XSBrdm06IGJv
b2tlOiBjbGVhciBob3N0IHRsYiByZWZlcmVuY2UgZmxhZyBvbiBndWVzdA0KPiB0bGIgaW52YWxp
ZGF0aW9uDQo+IA0KPiBPbiBUaHUsIDIwMTMtMDktMTkgYXQgMTE6MzIgKzA1MzAsIEJoYXJhdCBC
aHVzaGFuIHdyb3RlOg0KPiA+IE9uIGJvb2tlLCAic3RydWN0IHRsYmVfcmVmIiBjb250YWlucyBo
b3N0IHRsYiBtYXBwaW5nIGluZm9ybWF0aW9uDQo+ID4gKHBmbjogZm9yIGd1ZXN0LXBmbiB0byBw
Zm4sIGZsYWdzOiBhdHRyaWJ1dGUgYXNzb2NpYXRlZCB3aXRoIHRoaXMNCj4gPiBtYXBwaW5nKSBm
b3IgYSBndWVzdCB0bGIgZW50cnkuIFNvIHdoZW4gYSBndWVzdCBjcmVhdGVzIGEgVExCIGVudHJ5
DQo+ID4gdGhlbiAic3RydWN0IHRsYmVfcmVmIiBpcyBzZXQgdG8gcG9pbnQgdG8gdmFsaWQgInBm
biIgYW5kIHNldA0KPiA+IGF0dHJpYnV0ZXMgaW4gImZsYWdzIiBmaWVsZCBvZiB0aGUgYWJvdmUg
c2FpZCBzdHJ1Y3R1cmUuIFdoZW4gYSBndWVzdA0KPiA+IFRMQiBlbnRyeSBpcyBpbnZhbGlkYXRl
ZCB0aGVuIGZsYWdzIGZpZWxkIG9mIGNvcnJlc3BvbmRpbmcgInN0cnVjdA0KPiA+IHRsYmVfcmVm
IiBpcyB1cGRhdGVkIHRvIHBvaW50IHRoYXQgdGhpcyBpcyBubyBtb3JlIHZhbGlkLCBhbHNvIHdl
DQo+ID4gc2VsZWN0aXZlbHkgY2xlYXIgc29tZSBvdGhlciBhdHRyaWJ1dGUgYml0cywgZXhhbXBs
ZTogaWYNCj4gPiBFNTAwX1RMQl9CSVRNQVAgd2FzIHNldCB0aGVuIHdlIGNsZWFyIEU1MDBfVExC
X0JJVE1BUCwgaWYgRTUwMF9UTEJfVExCMCBpcyBzZXQNCj4gdGhlbiB3ZSBjbGVhciB0aGlzLg0K
PiA+DQo+ID4gSWRlYWxseSB3ZSBzaG91bGQgY2xlYXIgY29tcGxldGUgImZsYWdzIiBhcyB0aGlz
IGVudHJ5IGlzIGludmFsaWQgYW5kDQo+ID4gZG9lcyBub3QgaGF2ZSBhbnl0aGluZyB0byByZS11
c2VkLiBUaGUgb3RoZXIgcGFydCBvZiB0aGUgcHJvYmxlbSBpcw0KPiA+IHRoYXQgd2hlbiB3ZSB1
c2UgdGhlIHNhbWUgZW50cnkgYWdhaW4gdGhlbiBhbHNvIHdlIGRvIG5vdCBjbGVhciAoc3RhcnRl
ZCBkb2luZw0KPiBvci1pbmcgZXRjKS4NCj4gPg0KPiA+IFNvIGZhciBpdCB3YXMgd29ya2luZyBi
ZWNhdXNlIHRoZSBzZWxlY3RpdmVseSBjbGVhcmluZyBtZW50aW9uZWQgYWJvdmUNCj4gPiBhY3R1
YWxseSBjbGVhcnMgImZsYWdzIiB3aGF0IHdhcyBzZXQgZHVyaW5nIFRMQiBtYXBwaW5nLiBCdXQg
dGhlDQo+ID4gcHJvYmxlbSBzdGFydHMgY29taW5nIHdoZW4gd2UgYWRkIG1vcmUgYXR0cmlidXRl
cyB0byB0aGlzIHRoZW4gd2UgbmVlZA0KPiA+IHRvIHNlbGVjdGl2ZWx5IGNsZWFyIHRoZW0gYW5k
IHdoaWNoIGlzIG5vdCBuZWVkZWQuDQo+ID4NCj4gPiBUaGlzIHBhdGNoIHdlIGRvIGJvdGgNCj4g
PiAgICAgICAgIC0gQ2xlYXIgImZsYWdzIiB3aGVuIGludmFsaWRhdGluZzsNCj4gPiAgICAgICAg
IC0gQ2xlYXIgImZsYWdzIiB3aGVuIHJldXNpbmcgc2FtZSBlbnRyeSBsYXRlcg0KPiA+DQo+ID4g
U2lnbmVkLW9mZi1ieTogQmhhcmF0IEJodXNoYW4gPGJoYXJhdC5iaHVzaGFuQGZyZWVzY2FsZS5j
b20+DQo+ID4gLS0tDQo+ID4gdjMtPiB2NQ0KPiA+ICAtIE5ldyBwYXRjaCAoZm91bmQgdGhpcyBp
c3N1ZSB3aGVuIGRvaW5nIHZmaW8tcGNpIGRldmVsb3BtZW50KQ0KPiA+DQo+ID4gIGFyY2gvcG93
ZXJwYy9rdm0vZTUwMF9tbXVfaG9zdC5jIHwgICAxMiArKysrKysrLS0tLS0NCj4gPiAgMSBmaWxl
cyBjaGFuZ2VkLCA3IGluc2VydGlvbnMoKyksIDUgZGVsZXRpb25zKC0pDQo+ID4NCj4gPiBkaWZm
IC0tZ2l0IGEvYXJjaC9wb3dlcnBjL2t2bS9lNTAwX21tdV9ob3N0LmMNCj4gPiBiL2FyY2gvcG93
ZXJwYy9rdm0vZTUwMF9tbXVfaG9zdC5jDQo+ID4gaW5kZXggMWM2YTlkNy4uNjBmNWEzYyAxMDA2
NDQNCj4gPiAtLS0gYS9hcmNoL3Bvd2VycGMva3ZtL2U1MDBfbW11X2hvc3QuYw0KPiA+ICsrKyBi
L2FyY2gvcG93ZXJwYy9rdm0vZTUwMF9tbXVfaG9zdC5jDQo+ID4gQEAgLTIxNyw3ICsyMTcsOCBA
QCB2b2lkIGludmFsX2d0bGJlX29uX2hvc3Qoc3RydWN0IGt2bXBwY192Y3B1X2U1MDANCj4gKnZj
cHVfZTUwMCwgaW50IHRsYnNlbCwNCj4gPiAgCQl9DQo+ID4gIAkJbWIoKTsNCj4gPiAgCQl2Y3B1
X2U1MDAtPmcyaF90bGIxX21hcFtlc2VsXSA9IDA7DQo+ID4gLQkJcmVmLT5mbGFncyAmPSB+KEU1
MDBfVExCX0JJVE1BUCB8IEU1MDBfVExCX1ZBTElEKTsNCj4gPiArCQkvKiBDbGVhciBmbGFncyBh
cyBUTEIgaXMgbm90IGJhY2tlZCBieSB0aGUgaG9zdCBhbnltb3JlICovDQo+ID4gKwkJcmVmLT5m
bGFncyA9IDA7DQo+ID4gIAkJbG9jYWxfaXJxX3Jlc3RvcmUoZmxhZ3MpOw0KPiA+ICAJfQ0KPiAN
Cj4gVGhpcyBicmVha3Mgd2hlbiB5b3UgaGF2ZSBib3RoIEU1MDBfVExCX0JJVE1BUCBhbmQgRTUw
MF9UTEJfVExCMCBzZXQuDQoNCkkgZG8gbm90IHNlZSBhbnkgY2FzZSB3aGVyZSB3ZSBzZXQgYm90
aCBFNTAwX1RMQl9CSVRNQVAgYW5kIEU1MDBfVExCX1RMQjAuIEFsc28gd2UgaGF2ZSBub3Qgb3B0
aW1pemVkIHRoYXQgeWV0IChrZWVwaW5nIHRyYWNrIG9mIG11bHRpcGxlIHNoYWRvdyBUTEIwIGVu
dHJpZXMgZm9yIG9uZSBndWVzdCBUTEIxIGVudHJ5KQ0KDQpXZSB1c2VzIHRoZXNlIGJpdCBmbGFn
cyBvbmx5IGZvciBUTEIxIGFuZCBpZiBzaXplIG9mIHN0bGJlIGlzIDRLIHRoZW4gd2Ugc2V0IEU1
MDBfVExCX1RMQjAgIG90aGVyd2lzZSB3ZSBzZXQgRTUwMF9UTEJfQklUTUFQLiBBbHRob3VnaCBJ
IHRoaW5rIHRoYXQgRTUwMF9UTEJfQklUTUFQIHNob3VsZCBiZSBzZXQgb25seSBpZiBzdGxiZSBz
aXplIGlzIGxlc3MgdGhhbiBndGxiZSBzaXplLg0KDQo+IA0KPiBJbnN0ZWFkLCBqdXN0IGNvbnZl
cnQgdGhlIGZpbmFsIEU1MDBfVExCX1ZBTElEIGNsZWFyaW5nIGF0IHRoZSBlbmQgaW50bw0KPiBy
ZWYtPmZsYWdzID0gMCwgYW5kIGNvbnZlcnQgdGhlIGVhcmx5IHJldHVybiBhIGZldyBsaW5lcyBl
YXJsaWVyIGludG8NCj4gY29uZGl0aW9uYWwgZXhlY3V0aW9uIG9mIHRoZSB0bGJpbF9vbmUoKS4N
Cg0KVGhpcyBsb29rcyBiZXR0ZXIsIHdpbGwgc2VuZCB0aGUgcGF0Y2ggc2hvcnRseS4NCg0KVGhh
bmtzDQotQmhhcmF0DQoNCj4gDQo+IC1TY290dA0KPiANCg0K

Re: [PATCH 5/6 v5] kvm: booke: clear host tlb reference flag on guest tlb invalidation

[Scott Wood]

On Thu, 2013-09-19 at 23:19 -0500, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Friday, September 20, 2013 2:38 AM
> > To: Bhushan Bharat-R65777
> > Cc: benh@kernel.crashing.org; agraf@suse.de; paulus@samba.org;
> > kvm@vger.kernel.org; kvm-ppc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > Bhushan Bharat-R65777
> > Subject: Re: [PATCH 5/6 v5] kvm: booke: clear host tlb reference flag on guest
> > tlb invalidation
> > 
> > This breaks when you have both E500_TLB_BITMAP and E500_TLB_TLB0 set.
> 
> I do not see any case where we set both E500_TLB_BITMAP and
> E500_TLB_TLB0.

This would happen if you have a guest TLB1 entry that is backed by some
4K pages and some larger pages (e.g. if the guest maps CCSR with one big
TLB1 and there are varying I/O passthrough regions mapped).  It's not
common, but it's possible.

>  Also we have not optimized that yet (keeping track of
> multiple shadow TLB0 entries for one guest TLB1 entry)

This is about correctness, not optimization.

> We uses these bit flags only for TLB1 and if size of stlbe is 4K then
> we set E500_TLB_TLB0  otherwise we set E500_TLB_BITMAP. Although I
> think that E500_TLB_BITMAP should be set only if stlbe size is less
> than gtlbe size.

Why?  Even if there's only one bit set in the map, we need it to keep
track of which entry was used.

-Scott

RE: [PATCH 5/6 v5] kvm: booke: clear host tlb reference flag on guest tlb invalidation

[Bhushan Bharat-R65777]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogRnJpZGF5LCBTZXB0ZW1iZXIgMjAsIDIwMTMgOTo0OCBQTQ0KPiBUbzogQmh1
c2hhbiBCaGFyYXQtUjY1Nzc3DQo+IENjOiBXb29kIFNjb3R0LUIwNzQyMTsgYmVuaEBrZXJuZWwu
Y3Jhc2hpbmcub3JnOyBhZ3JhZkBzdXNlLmRlOw0KPiBwYXVsdXNAc2FtYmEub3JnOyBrdm1Admdl
ci5rZXJuZWwub3JnOyBrdm0tcHBjQHZnZXIua2VybmVsLm9yZzsgbGludXhwcGMtDQo+IGRldkBs
aXN0cy5vemxhYnMub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggNS82IHY1XSBrdm06IGJvb2tl
OiBjbGVhciBob3N0IHRsYiByZWZlcmVuY2UgZmxhZyBvbiBndWVzdA0KPiB0bGIgaW52YWxpZGF0
aW9uDQo+IA0KPiBPbiBUaHUsIDIwMTMtMDktMTkgYXQgMjM6MTkgLTA1MDAsIEJodXNoYW4gQmhh
cmF0LVI2NTc3NyB3cm90ZToNCj4gPg0KPiA+ID4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0N
Cj4gPiA+IEZyb206IFdvb2QgU2NvdHQtQjA3NDIxDQo+ID4gPiBTZW50OiBGcmlkYXksIFNlcHRl
bWJlciAyMCwgMjAxMyAyOjM4IEFNDQo+ID4gPiBUbzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3DQo+
ID4gPiBDYzogYmVuaEBrZXJuZWwuY3Jhc2hpbmcub3JnOyBhZ3JhZkBzdXNlLmRlOyBwYXVsdXNA
c2FtYmEub3JnOw0KPiA+ID4ga3ZtQHZnZXIua2VybmVsLm9yZzsga3ZtLXBwY0B2Z2VyLmtlcm5l
bC5vcmc7DQo+ID4gPiBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZzsgQmh1c2hhbiBCaGFy
YXQtUjY1Nzc3DQo+ID4gPiBTdWJqZWN0OiBSZTogW1BBVENIIDUvNiB2NV0ga3ZtOiBib29rZTog
Y2xlYXIgaG9zdCB0bGIgcmVmZXJlbmNlDQo+ID4gPiBmbGFnIG9uIGd1ZXN0IHRsYiBpbnZhbGlk
YXRpb24NCj4gPiA+DQo+ID4gPiBUaGlzIGJyZWFrcyB3aGVuIHlvdSBoYXZlIGJvdGggRTUwMF9U
TEJfQklUTUFQIGFuZCBFNTAwX1RMQl9UTEIwIHNldC4NCj4gPg0KPiA+IEkgZG8gbm90IHNlZSBh
bnkgY2FzZSB3aGVyZSB3ZSBzZXQgYm90aCBFNTAwX1RMQl9CSVRNQVAgYW5kDQo+ID4gRTUwMF9U
TEJfVExCMC4NCj4gDQo+IFRoaXMgd291bGQgaGFwcGVuIGlmIHlvdSBoYXZlIGEgZ3Vlc3QgVExC
MSBlbnRyeSB0aGF0IGlzIGJhY2tlZCBieSBzb21lIDRLIHBhZ2VzDQo+IGFuZCBzb21lIGxhcmdl
ciBwYWdlcyAoZS5nLiBpZiB0aGUgZ3Vlc3QgbWFwcyBDQ1NSIHdpdGggb25lIGJpZw0KPiBUTEIx
IGFuZCB0aGVyZSBhcmUgdmFyeWluZyBJL08gcGFzc3Rocm91Z2ggcmVnaW9ucyBtYXBwZWQpLiAg
SXQncyBub3QgY29tbW9uLA0KPiBidXQgaXQncyBwb3NzaWJsZS4NCg0KQWdyZWUNCg0KPiANCj4g
PiAgQWxzbyB3ZSBoYXZlIG5vdCBvcHRpbWl6ZWQgdGhhdCB5ZXQgKGtlZXBpbmcgdHJhY2sgb2Yg
bXVsdGlwbGUgc2hhZG93DQo+ID4gVExCMCBlbnRyaWVzIGZvciBvbmUgZ3Vlc3QgVExCMSBlbnRy
eSkNCj4gDQo+IFRoaXMgaXMgYWJvdXQgY29ycmVjdG5lc3MsIG5vdCBvcHRpbWl6YXRpb24uDQo+
IA0KPiA+IFdlIHVzZXMgdGhlc2UgYml0IGZsYWdzIG9ubHkgZm9yIFRMQjEgYW5kIGlmIHNpemUg
b2Ygc3RsYmUgaXMgNEsgdGhlbg0KPiA+IHdlIHNldCBFNTAwX1RMQl9UTEIwICBvdGhlcndpc2Ug
d2Ugc2V0IEU1MDBfVExCX0JJVE1BUC4gQWx0aG91Z2ggSQ0KPiA+IHRoaW5rIHRoYXQgRTUwMF9U
TEJfQklUTUFQIHNob3VsZCBiZSBzZXQgb25seSBpZiBzdGxiZSBzaXplIGlzIGxlc3MNCj4gPiB0
aGFuIGd0bGJlIHNpemUuDQo+IA0KPiBXaHk/ICBFdmVuIGlmIHRoZXJlJ3Mgb25seSBvbmUgYml0
IHNldCBpbiB0aGUgbWFwLCB3ZSBuZWVkIGl0IHRvIGtlZXAgdHJhY2sgb2YNCj4gd2hpY2ggZW50
cnkgd2FzIHVzZWQuDQoNCklmIHRoZXJlIGlzIG9uZSBlbnRyeSB0aGVuIHdpbGwgbm90IHRoaXMg
YmUgc2ltcGxlL2Zhc3RlciB0byBub3QgbG9va3VwIGJpdG1hcCBhbmQgZ3Vlc3QtPmhvc3QgYXJy
YXk/DQpBIGZsYWcgaW5kaWNhdGUgaXQgaXMgMToxIG1hcCBhbmQgdGhpcyBpcyBwaHlzaWNhbCBh
ZGRyZXNzLg0KDQotQmhhcmF0DQoNCj4gDQo+IC1TY290dA0KPiANCg0K

Re: [PATCH 5/6 v5] kvm: booke: clear host tlb reference flag on guest tlb invalidation

[Scott Wood]

On Fri, 2013-09-20 at 13:04 -0500, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Friday, September 20, 2013 9:48 PM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; benh@kernel.crashing.org; agraf@suse.de;
> > paulus@samba.org; kvm@vger.kernel.org; kvm-ppc@vger.kernel.org; linuxppc-
> > dev@lists.ozlabs.org
> > Subject: Re: [PATCH 5/6 v5] kvm: booke: clear host tlb reference flag on guest
> > tlb invalidation
> > 
> > On Thu, 2013-09-19 at 23:19 -0500, Bhushan Bharat-R65777 wrote:
> > > We uses these bit flags only for TLB1 and if size of stlbe is 4K then
> > > we set E500_TLB_TLB0  otherwise we set E500_TLB_BITMAP. Although I
> > > think that E500_TLB_BITMAP should be set only if stlbe size is less
> > > than gtlbe size.
> > 
> > Why?  Even if there's only one bit set in the map, we need it to keep track of
> > which entry was used.
> 
> If there is one entry then will not this be simple/faster to not lookup bitmap and guest->host array?
> A flag indicate it is 1:1 map and this is physical address.

The difference would be negligible, and you'd have added overhead (both
runtime and complexity) of making this a special case.

-Scott

RE: [PATCH 5/6 v5] kvm: booke: clear host tlb reference flag on guest tlb invalidation

[Bhushan Bharat-R65777]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogRnJpZGF5LCBTZXB0ZW1iZXIgMjAsIDIwMTMgMTE6MzggUE0NCj4gVG86IEJo
dXNoYW4gQmhhcmF0LVI2NTc3Nw0KPiBDYzogV29vZCBTY290dC1CMDc0MjE7IGJlbmhAa2VybmVs
LmNyYXNoaW5nLm9yZzsgYWdyYWZAc3VzZS5kZTsNCj4gcGF1bHVzQHNhbWJhLm9yZzsga3ZtQHZn
ZXIua2VybmVsLm9yZzsga3ZtLXBwY0B2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4cHBjLQ0KPiBkZXZA
bGlzdHMub3psYWJzLm9yZw0KPiBTdWJqZWN0OiBSZTogW1BBVENIIDUvNiB2NV0ga3ZtOiBib29r
ZTogY2xlYXIgaG9zdCB0bGIgcmVmZXJlbmNlIGZsYWcgb24gZ3Vlc3QNCj4gdGxiIGludmFsaWRh
dGlvbg0KPiANCj4gT24gRnJpLCAyMDEzLTA5LTIwIGF0IDEzOjA0IC0wNTAwLCBCaHVzaGFuIEJo
YXJhdC1SNjU3Nzcgd3JvdGU6DQo+ID4NCj4gPiA+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0t
DQo+ID4gPiBGcm9tOiBXb29kIFNjb3R0LUIwNzQyMQ0KPiA+ID4gU2VudDogRnJpZGF5LCBTZXB0
ZW1iZXIgMjAsIDIwMTMgOTo0OCBQTQ0KPiA+ID4gVG86IEJodXNoYW4gQmhhcmF0LVI2NTc3Nw0K
PiA+ID4gQ2M6IFdvb2QgU2NvdHQtQjA3NDIxOyBiZW5oQGtlcm5lbC5jcmFzaGluZy5vcmc7IGFn
cmFmQHN1c2UuZGU7DQo+ID4gPiBwYXVsdXNAc2FtYmEub3JnOyBrdm1Admdlci5rZXJuZWwub3Jn
OyBrdm0tcHBjQHZnZXIua2VybmVsLm9yZzsNCj4gPiA+IGxpbnV4cHBjLSBkZXZAbGlzdHMub3ps
YWJzLm9yZw0KPiA+ID4gU3ViamVjdDogUmU6IFtQQVRDSCA1LzYgdjVdIGt2bTogYm9va2U6IGNs
ZWFyIGhvc3QgdGxiIHJlZmVyZW5jZQ0KPiA+ID4gZmxhZyBvbiBndWVzdCB0bGIgaW52YWxpZGF0
aW9uDQo+ID4gPg0KPiA+ID4gT24gVGh1LCAyMDEzLTA5LTE5IGF0IDIzOjE5IC0wNTAwLCBCaHVz
aGFuIEJoYXJhdC1SNjU3Nzcgd3JvdGU6DQo+ID4gPiA+IFdlIHVzZXMgdGhlc2UgYml0IGZsYWdz
IG9ubHkgZm9yIFRMQjEgYW5kIGlmIHNpemUgb2Ygc3RsYmUgaXMgNEsNCj4gPiA+ID4gdGhlbiB3
ZSBzZXQgRTUwMF9UTEJfVExCMCAgb3RoZXJ3aXNlIHdlIHNldCBFNTAwX1RMQl9CSVRNQVAuDQo+
ID4gPiA+IEFsdGhvdWdoIEkgdGhpbmsgdGhhdCBFNTAwX1RMQl9CSVRNQVAgc2hvdWxkIGJlIHNl
dCBvbmx5IGlmIHN0bGJlDQo+ID4gPiA+IHNpemUgaXMgbGVzcyB0aGFuIGd0bGJlIHNpemUuDQo+
ID4gPg0KPiA+ID4gV2h5PyAgRXZlbiBpZiB0aGVyZSdzIG9ubHkgb25lIGJpdCBzZXQgaW4gdGhl
IG1hcCwgd2UgbmVlZCBpdCB0bw0KPiA+ID4ga2VlcCB0cmFjayBvZiB3aGljaCBlbnRyeSB3YXMg
dXNlZC4NCj4gPg0KPiA+IElmIHRoZXJlIGlzIG9uZSBlbnRyeSB0aGVuIHdpbGwgbm90IHRoaXMg
YmUgc2ltcGxlL2Zhc3RlciB0byBub3QgbG9va3VwIGJpdG1hcA0KPiBhbmQgZ3Vlc3QtPmhvc3Qg
YXJyYXk/DQo+ID4gQSBmbGFnIGluZGljYXRlIGl0IGlzIDE6MSBtYXAgYW5kIHRoaXMgaXMgcGh5
c2ljYWwgYWRkcmVzcy4NCj4gDQo+IFRoZSBkaWZmZXJlbmNlIHdvdWxkIGJlIG5lZ2xpZ2libGUs
IGFuZCB5b3UnZCBoYXZlIGFkZGVkIG92ZXJoZWFkIChib3RoIHJ1bnRpbWUNCj4gYW5kIGNvbXBs
ZXhpdHkpIG9mIG1ha2luZyB0aGlzIGEgc3BlY2lhbCBjYXNlLg0KDQpNYXkgYmUgeW91IGFyZSBy
aWdodCAsIEkgd2lsbCBzZWUgaWYgSSBjYW4gZ2l2ZSBhIHRyeSA6KQ0KQlRXIEkgaGF2ZSBhbHJl
YWR5IHNlbnQgdjYgb2YgdGhpcyBwYXRjaC4NCg0KLUJoYXJhdA0KDQo+IA0KPiAtU2NvdHQNCj4g
DQoNCg==

[PATCH 6/6 v5] 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>
---
v4->v5
 - No change

 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    |   38 ++++++++++++++++++++--------------
 4 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 9741bf0..775f0e8 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -538,6 +538,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;
@@ -595,7 +596,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 17722d8..4171c7d 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 60f5a3c..654c368 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
  */
@@ -250,10 +241,12 @@ 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);
@@ -314,8 +307,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);
 
@@ -334,6 +326,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 *ptep;
+	unsigned int wimg = 0;
+	pgd_t *pgdir;
 
 	/*
 	 * Translate guest physical to true physical, acquiring
@@ -396,7 +392,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);
@@ -438,9 +434,10 @@ 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;
@@ -451,7 +448,16 @@ 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;
+	ptep = lookup_linux_pte(pgdir, hva, &tsize_pages);
+	if (pte_present(*ptep)) {
+		wimg = (pte_val(*ptep) >> 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

Re: [PATCH 0/6 v5] kvm: powerpc: use cache attributes from linux pte

[Alexander Graf]

On 19.09.2013, at 08:02, Bharat Bhushan wrote:

> From: Bharat Bhushan <bharat.bhushan@freescale.com>
>=20
> First patch is a typo fix where book3e define _PAGE_LENDIAN while it
> should be defined as _PAGE_ENDIAN. This seems to show that this is =
never exercised :-)
>=20
> Second and third patch is to allow guest controlling "G"-Guarded and =
"E"-Endian TLB attributes respectively.
>=20
> Fourth patch is moving functions/logic in common code so they can be =
used on booke also.
>=20
> Fifth and Sixth patch is actually setting caching attributes =
(TLB.WIMGE) using corresponding Linux pte.

Thanks, applied 1/6 - 3/6 to kvm-ppc-queue.


Alex

>=20
> v3->v5
> - Fix tlb-reference-flag clearing issue (patch 4/6)
> - There was a patch (4/6 powerpc: move linux pte/hugepte search to =
more generic file)
>   in the last series of this patchset which was moving pte/hugepte =
search functions to
>   generic file. That patch is no more needed as some other patch is =
already applied to fix that :)
>=20
> v2->v3
> - now lookup_linux_pte() only have pte search logic and it does not
>   set any access flags in pte. There is already a function for setting
>   access flag which will be called explicitly where needed.
>   On booke we only need to search for pte to get WIMG.
>=20
> v1->v2
> - Earlier caching attributes (WIMGE) were set based of page is RAM or =
not
>   But now we get these attributes from corresponding Linux PTE.
>=20
> Bharat Bhushan (6):
>  powerpc: book3e: _PAGE_LENDIAN must be _PAGE_ENDIAN
>  kvm: powerpc: allow guest control "E" attribute in mas2
>  kvm: powerpc: allow guest control "G" attribute in mas2
>  kvm: powerpc: keep only pte search logic in lookup_linux_pte
>  kvm: booke: clear host tlb reference flag on guest tlb invalidation
>  kvm: powerpc: use caching attributes as per linux pte
>=20
> arch/powerpc/include/asm/kvm_host.h   |    2 +-
> arch/powerpc/include/asm/pgtable.h    |   24 ++++++++++++++++
> arch/powerpc/include/asm/pte-book3e.h |    2 +-
> arch/powerpc/kvm/book3s_hv_rm_mmu.c   |   36 ++++++++----------------
> arch/powerpc/kvm/booke.c              |    2 +-
> arch/powerpc/kvm/e500.h               |   10 ++++--
> arch/powerpc/kvm/e500_mmu_host.c      |   50 =
+++++++++++++++++++--------------
> 7 files changed, 74 insertions(+), 52 deletions(-)
>=20
>=20