[PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2

[PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2

[Bharat Bhushan]

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v2:
 - 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 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Bharat Bhushan]

If there is a struct page for the requested mapping then it's
normal RAM and the mapping is set to "M" bit (coherent, cacheable)
otherwise this is treated as I/O and we set  "I + G"  (cache inhibited, guarded)

This helps setting proper TLB mapping for direct assigned device

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v2: some cleanup and added comment
 - 
 arch/powerpc/kvm/e500_mmu_host.c |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
index 1c6a9d7..02eb973 100644
--- a/arch/powerpc/kvm/e500_mmu_host.c
+++ b/arch/powerpc/kvm/e500_mmu_host.c
@@ -64,13 +64,26 @@ static inline u32 e500_shadow_mas3_attrib(u32 mas3, int usermode)
 	return mas3;
 }
 
-static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
+static inline u32 e500_shadow_mas2_attrib(u32 mas2, pfn_t pfn)
 {
+	u32 mas2_attr;
+
+	mas2_attr = mas2 & MAS2_ATTRIB_MASK;
+
+	/*
+	 * RAM is always mappable on e500 systems, so this is identical
+	 * to kvm_is_mmio_pfn(), just without its overhead.
+	 */
+	if (!pfn_valid(pfn)) {
+		/* Pages not managed by Kernel are treated as I/O, set I + G */
+		mas2_attr |= MAS2_I | MAS2_G;
 #ifdef CONFIG_SMP
-	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
-#else
-	return mas2 & MAS2_ATTRIB_MASK;
+	} else {
+		/* Kernel managed pages are actually RAM so set  "M" */
+		mas2_attr |= MAS2_M;
 #endif
+	}
+	return mas2_attr;
 }
 
 /*
@@ -313,7 +326,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);
+		      e500_shadow_mas2_attrib(gtlbe->mas2, pfn);
 	stlbe->mas7_3 = ((u64)pfn << PAGE_SHIFT) |
 			e500_shadow_mas3_attrib(gtlbe->mas7_3, pr);
 
-- 
1.7.0.4

Re: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2

[Alexander Graf]

This needs a description. Why shouldn't we ignore E?


Alex

On 18.07.2013, at 15:19, Bharat Bhushan wrote:

> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> v2:
> - 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
> 
> 

Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Alexander Graf]

On 18.07.2013, at 15:19, Bharat Bhushan wrote:

> If there is a struct page for the requested mapping then it's
> normal RAM and the mapping is set to "M" bit (coherent, cacheable)
> otherwise this is treated as I/O and we set  "I + G"  (cache inhibited, guarded)
> 
> This helps setting proper TLB mapping for direct assigned device
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> v2: some cleanup and added comment
> - 
> arch/powerpc/kvm/e500_mmu_host.c |   23 ++++++++++++++++++-----
> 1 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
> index 1c6a9d7..02eb973 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -64,13 +64,26 @@ static inline u32 e500_shadow_mas3_attrib(u32 mas3, int usermode)
> 	return mas3;
> }
> 
> -static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
> +static inline u32 e500_shadow_mas2_attrib(u32 mas2, pfn_t pfn)
> {
> +	u32 mas2_attr;
> +
> +	mas2_attr = mas2 & MAS2_ATTRIB_MASK;
> +
> +	/*
> +	 * RAM is always mappable on e500 systems, so this is identical
> +	 * to kvm_is_mmio_pfn(), just without its overhead.
> +	 */
> +	if (!pfn_valid(pfn)) {
> +		/* Pages not managed by Kernel are treated as I/O, set I + G */

Please also document the intermediate thought that I/O should be mapped non-cached.

> +		mas2_attr |= MAS2_I | MAS2_G;
> #ifdef CONFIG_SMP

Please separate the SMP case out of the branch.

> -	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
> -#else
> -	return mas2 & MAS2_ATTRIB_MASK;
> +	} else {
> +		/* Kernel managed pages are actually RAM so set  "M" */

This comment doesn't tell me why M can be set ;).


Alex

> +		mas2_attr |= MAS2_M;
> #endif
> +	}
> +	return mas2_attr;
> }
> 
> /*
> @@ -313,7 +326,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);
> +		      e500_shadow_mas2_attrib(gtlbe->mas2, pfn);
> 	stlbe->mas7_3 = ((u64)pfn << PAGE_SHIFT) |
> 			e500_shadow_mas3_attrib(gtlbe->mas7_3, pr);
> 
> -- 
> 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

RE: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On
> Behalf Of Alexander Graf
> Sent: Thursday, July 18, 2013 8:18 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421; Bhushan
> Bharat-R65777
> Subject: Re: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2
> 
> 
> This needs a description. Why shouldn't we ignore E?

What I understood is that "there is no reason to stop guest setting "E", so allow him."

-Bharat

> 
> 
> Alex
> 
> On 18.07.2013, at 15:19, Bharat Bhushan wrote:
> 
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > v2:
> > - 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
> >
> >
> 
> --
> 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 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On
> Behalf Of Alexander Graf
> Sent: Thursday, July 18, 2013 8:23 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421; Bhushan
> Bharat-R65777
> Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel
> managed pages
> 
> 
> On 18.07.2013, at 15:19, Bharat Bhushan wrote:
> 
> > If there is a struct page for the requested mapping then it's normal
> > RAM and the mapping is set to "M" bit (coherent, cacheable) otherwise
> > this is treated as I/O and we set  "I + G"  (cache inhibited, guarded)
> >
> > This helps setting proper TLB mapping for direct assigned device
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > v2: some cleanup and added comment
> > -
> > arch/powerpc/kvm/e500_mmu_host.c |   23 ++++++++++++++++++-----
> > 1 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/e500_mmu_host.c
> > b/arch/powerpc/kvm/e500_mmu_host.c
> > index 1c6a9d7..02eb973 100644
> > --- a/arch/powerpc/kvm/e500_mmu_host.c
> > +++ b/arch/powerpc/kvm/e500_mmu_host.c
> > @@ -64,13 +64,26 @@ static inline u32 e500_shadow_mas3_attrib(u32 mas3, int
> usermode)
> > 	return mas3;
> > }
> >
> > -static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
> > +static inline u32 e500_shadow_mas2_attrib(u32 mas2, pfn_t pfn)
> > {
> > +	u32 mas2_attr;
> > +
> > +	mas2_attr = mas2 & MAS2_ATTRIB_MASK;
> > +
> > +	/*
> > +	 * RAM is always mappable on e500 systems, so this is identical
> > +	 * to kvm_is_mmio_pfn(), just without its overhead.
> > +	 */
> > +	if (!pfn_valid(pfn)) {
> > +		/* Pages not managed by Kernel are treated as I/O, set I + G */
> 
> Please also document the intermediate thought that I/O should be mapped non-
> cached.

I did not get what you mean to document?

> 
> > +		mas2_attr |= MAS2_I | MAS2_G;
> > #ifdef CONFIG_SMP
> 
> Please separate the SMP case out of the branch.

Really :) this was looking simple to me.

> 
> > -	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
> > -#else
> > -	return mas2 & MAS2_ATTRIB_MASK;
> > +	} else {
> > +		/* Kernel managed pages are actually RAM so set  "M" */
> 
> This comment doesn't tell me why M can be set ;).

RAM in SMP, so setting coherent, is not that obvious?

-Bharat

> 
> 
> Alex
> 
> > +		mas2_attr |= MAS2_M;
> > #endif
> > +	}
> > +	return mas2_attr;
> > }
> >
> > /*
> > @@ -313,7 +326,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);
> > +		      e500_shadow_mas2_attrib(gtlbe->mas2, pfn);
> > 	stlbe->mas7_3 = ((u64)pfn << PAGE_SHIFT) |
> > 			e500_shadow_mas3_attrib(gtlbe->mas7_3, pr);
> >
> > --
> > 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
> 
> --
> 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 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Alexander Graf]

On 18.07.2013, at 17:15, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On
>> Behalf Of Alexander Graf
>> Sent: Thursday, July 18, 2013 8:23 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421; Bhushan
>> Bharat-R65777
>> Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel
>> managed pages
>> 
>> 
>> On 18.07.2013, at 15:19, Bharat Bhushan wrote:
>> 
>>> If there is a struct page for the requested mapping then it's normal
>>> RAM and the mapping is set to "M" bit (coherent, cacheable) otherwise
>>> this is treated as I/O and we set  "I + G"  (cache inhibited, guarded)
>>> 
>>> This helps setting proper TLB mapping for direct assigned device
>>> 
>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>> ---
>>> v2: some cleanup and added comment
>>> -
>>> arch/powerpc/kvm/e500_mmu_host.c |   23 ++++++++++++++++++-----
>>> 1 files changed, 18 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/kvm/e500_mmu_host.c
>>> b/arch/powerpc/kvm/e500_mmu_host.c
>>> index 1c6a9d7..02eb973 100644
>>> --- a/arch/powerpc/kvm/e500_mmu_host.c
>>> +++ b/arch/powerpc/kvm/e500_mmu_host.c
>>> @@ -64,13 +64,26 @@ static inline u32 e500_shadow_mas3_attrib(u32 mas3, int
>> usermode)
>>> 	return mas3;
>>> }
>>> 
>>> -static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
>>> +static inline u32 e500_shadow_mas2_attrib(u32 mas2, pfn_t pfn)
>>> {
>>> +	u32 mas2_attr;
>>> +
>>> +	mas2_attr = mas2 & MAS2_ATTRIB_MASK;
>>> +
>>> +	/*
>>> +	 * RAM is always mappable on e500 systems, so this is identical
>>> +	 * to kvm_is_mmio_pfn(), just without its overhead.
>>> +	 */
>>> +	if (!pfn_valid(pfn)) {
>>> +		/* Pages not managed by Kernel are treated as I/O, set I + G */
>> 
>> Please also document the intermediate thought that I/O should be mapped non-
>> cached.
> 
> I did not get what you mean to document?

Page snot managed by the kernel are treated as I/O. Map it
with disabled cache.

> 
>> 
>>> +		mas2_attr |= MAS2_I | MAS2_G;
>>> #ifdef CONFIG_SMP
>> 
>> Please separate the SMP case out of the branch.
> 
> Really :) this was looking simple to me.

Two branches intertwined never look simple :).

> 
>> 
>>> -	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
>>> -#else
>>> -	return mas2 & MAS2_ATTRIB_MASK;
>>> +	} else {
>>> +		/* Kernel managed pages are actually RAM so set  "M" */
>> 
>> This comment doesn't tell me why M can be set ;).
> 
> RAM in SMP, so setting coherent, is not that obvious?

No.


Alex

Re: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2

[Alexander Graf]

On 18.07.2013, at 17:12, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On
>> Behalf Of Alexander Graf
>> Sent: Thursday, July 18, 2013 8:18 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421; Bhushan
>> Bharat-R65777
>> Subject: Re: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2
>> 
>> 
>> This needs a description. Why shouldn't we ignore E?
> 
> What I understood is that "there is no reason to stop guest setting "E", so allow him."

Please add that to the patch description. Also explain what the bit means.


Alex

RE: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 18, 2013 8:50 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421
> Subject: Re: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2
> 
> 
> On 18.07.2013, at 17:12, Bhushan Bharat-R65777 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: kvm-ppc-owner@vger.kernel.org
> >> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
> >> Sent: Thursday, July 18, 2013 8:18 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421;
> >> Bhushan
> >> Bharat-R65777
> >> Subject: Re: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute
> >> in mas2
> >>
> >>
> >> This needs a description. Why shouldn't we ignore E?
> >
> > What I understood is that "there is no reason to stop guest setting "E", so
> allow him."
> 
> Please add that to the patch description. Also explain what the bit means.

Ok :)

-Bharat
> 
> 
> Alex
> 

Re: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E" attribute in mas2

[Scott Wood]

On 07/18/2013 10:12:30 AM, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: kvm-ppc-owner@vger.kernel.org  
> [mailto:kvm-ppc-owner@vger.kernel.org] On
> > Behalf Of Alexander Graf
> > Sent: Thursday, July 18, 2013 8:18 PM
> > To: Bhushan Bharat-R65777
> > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood  
> Scott-B07421; Bhushan
> > Bharat-R65777
> > Subject: Re: [PATCH 1/2 v2] kvm: powerpc: Do not ignore "E"  
> attribute in mas2
> >
> >
> > This needs a description. Why shouldn't we ignore E?
> 
> What I understood is that "there is no reason to stop guest setting  
> "E", so allow him."

FWIW, it'd probably be safe to let the guest control the G bit as well.

-Scott

Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Scott Wood]

On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> If there is a struct page for the requested mapping then it's
> normal RAM and the mapping is set to "M" bit (coherent, cacheable)
> otherwise this is treated as I/O and we set  "I + G"  (cache  
> inhibited, guarded)
> 
> This helps setting proper TLB mapping for direct assigned device
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> v2: some cleanup and added comment
>  -
>  arch/powerpc/kvm/e500_mmu_host.c |   23 ++++++++++++++++++-----
>  1 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c  
> b/arch/powerpc/kvm/e500_mmu_host.c
> index 1c6a9d7..02eb973 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -64,13 +64,26 @@ static inline u32 e500_shadow_mas3_attrib(u32  
> mas3, int usermode)
>  	return mas3;
>  }
> 
> -static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
> +static inline u32 e500_shadow_mas2_attrib(u32 mas2, pfn_t pfn)
>  {
> +	u32 mas2_attr;
> +
> +	mas2_attr = mas2 & MAS2_ATTRIB_MASK;
> +
> +	/*
> +	 * RAM is always mappable on e500 systems, so this is identical
> +	 * to kvm_is_mmio_pfn(), just without its overhead.
> +	 */
> +	if (!pfn_valid(pfn)) {

Please use page_is_ram(), which is what gets used when setting the WIMG  
for the host userspace mapping.  We want to make sure the two are  
consistent.

> +		/* Pages not managed by Kernel are treated as I/O, set  
> I + G */
> +		mas2_attr |= MAS2_I | MAS2_G;
>  #ifdef CONFIG_SMP
> -	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
> -#else
> -	return mas2 & MAS2_ATTRIB_MASK;
> +	} else {
> +		/* Kernel managed pages are actually RAM so set  "M" */
> +		mas2_attr |= MAS2_M;
>  #endif

Likewise, we want to make sure this matches the host entry.   
Unfortunately, this is a bit of a mess already.  64-bit booke appears  
to always set MAS2_M for TLB0 mappings.  The initial KERNELBASE mapping  
on boot uses M_IF_SMP, and the settlbcam() that (IIRC) replaces it uses  
_PAGE_COHERENT.  32-bit always uses _PAGE_COHERENT, except that initial  
KERNELBASE mapping.  _PAGE_COHERENT appears to be set based on  
CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter config clears  
_PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT case).

As for what we actually want to happen, there are cases when we want M  
to be set for non-SMP.  One such case is AMP, where CPUs may be sharing  
memory even if the Linux instance only runs on one CPU (this is not  
hypothetical, BTW).  It's also possible that we encounter a hardware  
bug that requires MAS2_M, similar to what some of our non-booke chips  
require.

-Scott

Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Alexander Graf]

On 18.07.2013, at 19:17, Scott Wood wrote:

> On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
>> If there is a struct page for the requested mapping then it's
>> normal RAM and the mapping is set to "M" bit (coherent, cacheable)
>> otherwise this is treated as I/O and we set  "I + G"  (cache inhibited, guarded)
>> This helps setting proper TLB mapping for direct assigned device
>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>> ---
>> v2: some cleanup and added comment
>> -
>> arch/powerpc/kvm/e500_mmu_host.c |   23 ++++++++++++++++++-----
>> 1 files changed, 18 insertions(+), 5 deletions(-)
>> diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
>> index 1c6a9d7..02eb973 100644
>> --- a/arch/powerpc/kvm/e500_mmu_host.c
>> +++ b/arch/powerpc/kvm/e500_mmu_host.c
>> @@ -64,13 +64,26 @@ static inline u32 e500_shadow_mas3_attrib(u32 mas3, int usermode)
>> 	return mas3;
>> }
>> -static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
>> +static inline u32 e500_shadow_mas2_attrib(u32 mas2, pfn_t pfn)
>> {
>> +	u32 mas2_attr;
>> +
>> +	mas2_attr = mas2 & MAS2_ATTRIB_MASK;
>> +
>> +	/*
>> +	 * RAM is always mappable on e500 systems, so this is identical
>> +	 * to kvm_is_mmio_pfn(), just without its overhead.
>> +	 */
>> +	if (!pfn_valid(pfn)) {
> 
> Please use page_is_ram(), which is what gets used when setting the WIMG for the host userspace mapping.  We want to make sure the two are consistent.
> 
>> +		/* Pages not managed by Kernel are treated as I/O, set I + G */
>> +		mas2_attr |= MAS2_I | MAS2_G;
>> #ifdef CONFIG_SMP
>> -	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
>> -#else
>> -	return mas2 & MAS2_ATTRIB_MASK;
>> +	} else {
>> +		/* Kernel managed pages are actually RAM so set  "M" */
>> +		mas2_attr |= MAS2_M;
>> #endif
> 
> Likewise, we want to make sure this matches the host entry.  Unfortunately, this is a bit of a mess already.  64-bit booke appears to always set MAS2_M for TLB0 mappings.  The initial KERNELBASE mapping on boot uses M_IF_SMP, and the settlbcam() that (IIRC) replaces it uses _PAGE_COHERENT.  32-bit always uses _PAGE_COHERENT, except that initial KERNELBASE mapping.  _PAGE_COHERENT appears to be set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter config clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT case).
> 
> As for what we actually want to happen, there are cases when we want M to be set for non-SMP.  One such case is AMP, where CPUs may be sharing memory even if the Linux instance only runs on one CPU (this is not hypothetical, BTW).  It's also possible that we encounter a hardware bug that requires MAS2_M, similar to what some of our non-booke chips require.

How about we always set M then for RAM?


Alex

Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Scott Wood]

On 07/18/2013 12:32:18 PM, Alexander Graf wrote:
> 
> On 18.07.2013, at 19:17, Scott Wood wrote:
> 
> > On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > Likewise, we want to make sure this matches the host entry.   
> Unfortunately, this is a bit of a mess already.  64-bit booke appears  
> to always set MAS2_M for TLB0 mappings.  The initial KERNELBASE  
> mapping on boot uses M_IF_SMP, and the settlbcam() that (IIRC)  
> replaces it uses _PAGE_COHERENT.  32-bit always uses _PAGE_COHERENT,  
> except that initial KERNELBASE mapping.  _PAGE_COHERENT appears to be  
> set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter config  
> clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT case).
> >
> > As for what we actually want to happen, there are cases when we  
> want M to be set for non-SMP.  One such case is AMP, where CPUs may  
> be sharing memory even if the Linux instance only runs on one CPU  
> (this is not hypothetical, BTW).  It's also possible that we  
> encounter a hardware bug that requires MAS2_M, similar to what some  
> of our non-booke chips require.
> 
> How about we always set M then for RAM?

M is like I in that bad things happen if you mix them.  So we really  
want to match exactly what the rest of the kernel is doing.

Plus, the performance penalty on some single-core chips can be pretty  
bad.

-Scott

RE: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, July 18, 2013 11:09 PM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
> Bharat-R65777
> Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel
> managed pages
> 
> On 07/18/2013 12:32:18 PM, Alexander Graf wrote:
> >
> > On 18.07.2013, at 19:17, Scott Wood wrote:
> >
> > > On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > > Likewise, we want to make sure this matches the host entry.
> > Unfortunately, this is a bit of a mess already.  64-bit booke appears
> > to always set MAS2_M for TLB0 mappings.  The initial KERNELBASE
> > mapping on boot uses M_IF_SMP, and the settlbcam() that (IIRC)
> > replaces it uses _PAGE_COHERENT.  32-bit always uses _PAGE_COHERENT,
> > except that initial KERNELBASE mapping.  _PAGE_COHERENT appears to be
> > set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter config
> > clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT case).
> > >
> > > As for what we actually want to happen, there are cases when we
> > want M to be set for non-SMP.  One such case is AMP, where CPUs may be
> > sharing memory even if the Linux instance only runs on one CPU (this
> > is not hypothetical, BTW).  It's also possible that we encounter a
> > hardware bug that requires MAS2_M, similar to what some of our
> > non-booke chips require.
> >
> > How about we always set M then for RAM?
> 
> M is like I in that bad things happen if you mix them.

I am trying to list the invalid mixing of WIMG:

 1) I & M
 2) W & I
 3) W & M (Scott mentioned that he observed issues when  mixing these two)
 4) is there any other?

So it mean it is safe to let guest control G and E.

>  So we really want to
> match exactly what the rest of the kernel is doing.

How the rest of kernel is doing is a bit complex. IIUC, if we forget about the boot state then this is how kernel set WIMG bits:
 1) For Memory always set M if CONFIG_SMP set.
	- So KVM can do same. "M" will not be mixed with "W" and "I". G and E are guest control.
 2) For I/O , drivers can pass flags to set M or "I + G".
	- For KVM; if not memory then it is I/O. For now we can always set "I + G".
	- Later we can design some mechanism in VFIO interface to let KVM somehow know whether to set "M" or "I+G".

-Bharat

> 
> Plus, the performance penalty on some single-core chips can be pretty bad.
> 
> -Scott

Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Scott Wood]

On 07/21/2013 11:39:45 PM, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, July 18, 2013 11:09 PM
> > To: Alexander Graf
> > Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;  
> kvm@vger.kernel.org; Bhushan
> > Bharat-R65777
> > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only  
> for kernel
> > managed pages
> >
> > On 07/18/2013 12:32:18 PM, Alexander Graf wrote:
> > >
> > > On 18.07.2013, at 19:17, Scott Wood wrote:
> > >
> > > > On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > > > Likewise, we want to make sure this matches the host entry.
> > > Unfortunately, this is a bit of a mess already.  64-bit booke  
> appears
> > > to always set MAS2_M for TLB0 mappings.  The initial KERNELBASE
> > > mapping on boot uses M_IF_SMP, and the settlbcam() that (IIRC)
> > > replaces it uses _PAGE_COHERENT.  32-bit always uses  
> _PAGE_COHERENT,
> > > except that initial KERNELBASE mapping.  _PAGE_COHERENT appears  
> to be
> > > set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter config
> > > clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT case).
> > > >
> > > > As for what we actually want to happen, there are cases when we
> > > want M to be set for non-SMP.  One such case is AMP, where CPUs  
> may be
> > > sharing memory even if the Linux instance only runs on one CPU  
> (this
> > > is not hypothetical, BTW).  It's also possible that we encounter a
> > > hardware bug that requires MAS2_M, similar to what some of our
> > > non-booke chips require.
> > >
> > > How about we always set M then for RAM?
> >
> > M is like I in that bad things happen if you mix them.
> 
> I am trying to list the invalid mixing of WIMG:
> 
>  1) I & M
>  2) W & I
>  3) W & M (Scott mentioned that he observed issues when  mixing these  
> two)
>  4) is there any other?

That's not what I was talking about (and I don't think I mentioned W at  
all, though it is also potentially problematic).  I'm talking about  
mixing I with not-I (on two different virtual addresses pointing to the  
same physical), M with not-M, etc.

> >  So we really want to
> > match exactly what the rest of the kernel is doing.
> 
> How the rest of kernel is doing is a bit complex. IIUC, if we forget  
> about the boot state then this is how kernel set WIMG bits:
>  1) For Memory always set M if CONFIG_SMP set.
> 	- So KVM can do same. "M" will not be mixed with "W" and "I". G  
> and E are guest control.

I don't think this is accurate for 64-bit.  And what about the AMP case?

-Scott

RE: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 23, 2013 12:18 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alexander Graf; kvm-ppc@vger.kernel.org;
> kvm@vger.kernel.org
> Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel
> managed pages
> 
> On 07/21/2013 11:39:45 PM, Bhushan Bharat-R65777 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Thursday, July 18, 2013 11:09 PM
> > > To: Alexander Graf
> > > Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;
> > kvm@vger.kernel.org; Bhushan
> > > Bharat-R65777
> > > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only
> > for kernel
> > > managed pages
> > >
> > > On 07/18/2013 12:32:18 PM, Alexander Graf wrote:
> > > >
> > > > On 18.07.2013, at 19:17, Scott Wood wrote:
> > > >
> > > > > On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > > > > Likewise, we want to make sure this matches the host entry.
> > > > Unfortunately, this is a bit of a mess already.  64-bit booke
> > appears
> > > > to always set MAS2_M for TLB0 mappings.  The initial KERNELBASE
> > > > mapping on boot uses M_IF_SMP, and the settlbcam() that (IIRC)
> > > > replaces it uses _PAGE_COHERENT.  32-bit always uses
> > _PAGE_COHERENT,
> > > > except that initial KERNELBASE mapping.  _PAGE_COHERENT appears
> > to be
> > > > set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter config
> > > > clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT case).
> > > > >
> > > > > As for what we actually want to happen, there are cases when we
> > > > want M to be set for non-SMP.  One such case is AMP, where CPUs
> > may be
> > > > sharing memory even if the Linux instance only runs on one CPU
> > (this
> > > > is not hypothetical, BTW).  It's also possible that we encounter a
> > > > hardware bug that requires MAS2_M, similar to what some of our
> > > > non-booke chips require.
> > > >
> > > > How about we always set M then for RAM?
> > >
> > > M is like I in that bad things happen if you mix them.
> >
> > I am trying to list the invalid mixing of WIMG:
> >
> >  1) I & M
> >  2) W & I
> >  3) W & M (Scott mentioned that he observed issues when  mixing these
> > two)
> >  4) is there any other?
> 
> That's not what I was talking about (and I don't think I mentioned W at all,
> though it is also potentially problematic).

Here is cut paste of your one response:
"The architecture makes it illegal to mix cacheable and cache-inhibited  
mappings to the same physical page.  Mixing W or M bits is generally  
bad as well.  I've seen it cause machine checks, error interrupts, etc.  
-- not just corrupting the page in question."

So I added not mixing W & M. But at that time I missed to understood why mixing M & I for same physical address can be issue :).

>  I'm talking about mixing I with
> not-I (on two different virtual addresses pointing to the same physical), M with
> not-M, etc.

When we say all RAM (page_is_ram() is true) will be having "M" bit, then RAM physical address will not have "M" mixed with any other, right?

Similarly, For IO (which is not RAM), we will set "I+G", so "I" will not be mixed with "M". Is not that?

-Bharat

> 
> > >  So we really want to
> > > match exactly what the rest of the kernel is doing.
> >
> > How the rest of kernel is doing is a bit complex. IIUC, if we forget
> > about the boot state then this is how kernel set WIMG bits:
> >  1) For Memory always set M if CONFIG_SMP set.
> > 	- So KVM can do same. "M" will not be mixed with "W" and "I". G and E
> > are guest control.
> 
> I don't think this is accurate for 64-bit.  And what about the AMP case?
> 
> -Scott

RE: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, July 18, 2013 10:48 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; Bhushan Bharat-
> R65777
> Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel
> managed pages
> 
> On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > If there is a struct page for the requested mapping then it's normal
> > RAM and the mapping is set to "M" bit (coherent, cacheable) otherwise
> > this is treated as I/O and we set  "I + G"  (cache inhibited, guarded)
> >
> > This helps setting proper TLB mapping for direct assigned device
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > v2: some cleanup and added comment
> >  -
> >  arch/powerpc/kvm/e500_mmu_host.c |   23 ++++++++++++++++++-----
> >  1 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/e500_mmu_host.c
> > b/arch/powerpc/kvm/e500_mmu_host.c
> > index 1c6a9d7..02eb973 100644
> > --- a/arch/powerpc/kvm/e500_mmu_host.c
> > +++ b/arch/powerpc/kvm/e500_mmu_host.c
> > @@ -64,13 +64,26 @@ static inline u32 e500_shadow_mas3_attrib(u32
> > mas3, int usermode)
> >  	return mas3;
> >  }
> >
> > -static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
> > +static inline u32 e500_shadow_mas2_attrib(u32 mas2, pfn_t pfn)
> >  {
> > +	u32 mas2_attr;
> > +
> > +	mas2_attr = mas2 & MAS2_ATTRIB_MASK;
> > +
> > +	/*
> > +	 * RAM is always mappable on e500 systems, so this is identical
> > +	 * to kvm_is_mmio_pfn(), just without its overhead.
> > +	 */
> > +	if (!pfn_valid(pfn)) {
> 
> Please use page_is_ram(), which is what gets used when setting the WIMG for the
> host userspace mapping.  We want to make sure the two are consistent.
> 
> > +		/* Pages not managed by Kernel are treated as I/O, set
> > I + G */
> > +		mas2_attr |= MAS2_I | MAS2_G;
> >  #ifdef CONFIG_SMP
> > -	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
> > -#else
> > -	return mas2 & MAS2_ATTRIB_MASK;
> > +	} else {
> > +		/* Kernel managed pages are actually RAM so set  "M" */
> > +		mas2_attr |= MAS2_M;
> >  #endif
> 
> Likewise, we want to make sure this matches the host entry.
> Unfortunately, this is a bit of a mess already.  64-bit booke appears to always
> set MAS2_M for TLB0 mappings.

Scott, can you please point to the code where MAS2_M is always set for TLB0?

-Bharat

>  The initial KERNELBASE mapping on boot uses
> M_IF_SMP, and the settlbcam() that (IIRC) replaces it uses _PAGE_COHERENT.  32-
> bit always uses _PAGE_COHERENT, except that initial KERNELBASE mapping.
> _PAGE_COHERENT appears to be set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the
> latter config clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT case).
> 
> As for what we actually want to happen, there are cases when we want M to be set
> for non-SMP.  One such case is AMP, where CPUs may be sharing memory even if the
> Linux instance only runs on one CPU (this is not hypothetical, BTW).  It's also
> possible that we encounter a hardware bug that requires MAS2_M, similar to what
> some of our non-booke chips require.
> 
> -Scott

Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Scott Wood]

On 07/22/2013 10:39:16 PM, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, July 23, 2013 12:18 AM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; Alexander Graf; kvm-ppc@vger.kernel.org;
> > kvm@vger.kernel.org
> > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only  
> for kernel
> > managed pages
> >
> > On 07/21/2013 11:39:45 PM, Bhushan Bharat-R65777 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Thursday, July 18, 2013 11:09 PM
> > > > To: Alexander Graf
> > > > Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;
> > > kvm@vger.kernel.org; Bhushan
> > > > Bharat-R65777
> > > > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency  
> only
> > > for kernel
> > > > managed pages
> > > >
> > > > On 07/18/2013 12:32:18 PM, Alexander Graf wrote:
> > > > >
> > > > > On 18.07.2013, at 19:17, Scott Wood wrote:
> > > > >
> > > > > > On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > > > > > Likewise, we want to make sure this matches the host entry.
> > > > > Unfortunately, this is a bit of a mess already.  64-bit booke
> > > appears
> > > > > to always set MAS2_M for TLB0 mappings.  The initial  
> KERNELBASE
> > > > > mapping on boot uses M_IF_SMP, and the settlbcam() that (IIRC)
> > > > > replaces it uses _PAGE_COHERENT.  32-bit always uses
> > > _PAGE_COHERENT,
> > > > > except that initial KERNELBASE mapping.  _PAGE_COHERENT  
> appears
> > > to be
> > > > > set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter  
> config
> > > > > clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT case).
> > > > > >
> > > > > > As for what we actually want to happen, there are cases  
> when we
> > > > > want M to be set for non-SMP.  One such case is AMP, where  
> CPUs
> > > may be
> > > > > sharing memory even if the Linux instance only runs on one CPU
> > > (this
> > > > > is not hypothetical, BTW).  It's also possible that we  
> encounter a
> > > > > hardware bug that requires MAS2_M, similar to what some of our
> > > > > non-booke chips require.
> > > > >
> > > > > How about we always set M then for RAM?
> > > >
> > > > M is like I in that bad things happen if you mix them.
> > >
> > > I am trying to list the invalid mixing of WIMG:
> > >
> > >  1) I & M
> > >  2) W & I
> > >  3) W & M (Scott mentioned that he observed issues when  mixing  
> these
> > > two)
> > >  4) is there any other?
> >
> > That's not what I was talking about (and I don't think I mentioned  
> W at all,
> > though it is also potentially problematic).
> 
> Here is cut paste of your one response:
> "The architecture makes it illegal to mix cacheable and  
> cache-inhibited
> mappings to the same physical page.  Mixing W or M bits is generally
> bad as well.  I've seen it cause machine checks, error interrupts,  
> etc.
> -- not just corrupting the page in question."
> 
> So I added not mixing W & M. But at that time I missed to understood  
> why mixing M & I for same physical address can be issue :).

"W or M", not "W and M".  I meant that each one, separately, is in a  
similar situation as the I bit.

None of this is about invalid combinations of attributes on a single  
TLB entry (though there are architectural restrictions there as well).

-Scott

RE: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 23, 2013 10:15 PM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alexander Graf; kvm-ppc@vger.kernel.org;
> kvm@vger.kernel.org
> Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel
> managed pages
> 
> On 07/22/2013 10:39:16 PM, Bhushan Bharat-R65777 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, July 23, 2013 12:18 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; Alexander Graf; kvm-ppc@vger.kernel.org;
> > > kvm@vger.kernel.org
> > > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only
> > for kernel
> > > managed pages
> > >
> > > On 07/21/2013 11:39:45 PM, Bhushan Bharat-R65777 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Thursday, July 18, 2013 11:09 PM
> > > > > To: Alexander Graf
> > > > > Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;
> > > > kvm@vger.kernel.org; Bhushan
> > > > > Bharat-R65777
> > > > > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency
> > only
> > > > for kernel
> > > > > managed pages
> > > > >
> > > > > On 07/18/2013 12:32:18 PM, Alexander Graf wrote:
> > > > > >
> > > > > > On 18.07.2013, at 19:17, Scott Wood wrote:
> > > > > >
> > > > > > > On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > > > > > > Likewise, we want to make sure this matches the host entry.
> > > > > > Unfortunately, this is a bit of a mess already.  64-bit booke
> > > > appears
> > > > > > to always set MAS2_M for TLB0 mappings.  The initial
> > KERNELBASE
> > > > > > mapping on boot uses M_IF_SMP, and the settlbcam() that (IIRC)
> > > > > > replaces it uses _PAGE_COHERENT.  32-bit always uses
> > > > _PAGE_COHERENT,
> > > > > > except that initial KERNELBASE mapping.  _PAGE_COHERENT
> > appears
> > > > to be
> > > > > > set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter
> > config
> > > > > > clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT case).
> > > > > > >
> > > > > > > As for what we actually want to happen, there are cases
> > when we
> > > > > > want M to be set for non-SMP.  One such case is AMP, where
> > CPUs
> > > > may be
> > > > > > sharing memory even if the Linux instance only runs on one CPU
> > > > (this
> > > > > > is not hypothetical, BTW).  It's also possible that we
> > encounter a
> > > > > > hardware bug that requires MAS2_M, similar to what some of our
> > > > > > non-booke chips require.
> > > > > >
> > > > > > How about we always set M then for RAM?
> > > > >
> > > > > M is like I in that bad things happen if you mix them.
> > > >
> > > > I am trying to list the invalid mixing of WIMG:
> > > >
> > > >  1) I & M
> > > >  2) W & I
> > > >  3) W & M (Scott mentioned that he observed issues when  mixing
> > these
> > > > two)
> > > >  4) is there any other?
> > >
> > > That's not what I was talking about (and I don't think I mentioned
> > W at all,
> > > though it is also potentially problematic).
> >
> > Here is cut paste of your one response:
> > "The architecture makes it illegal to mix cacheable and
> > cache-inhibited mappings to the same physical page.  Mixing W or M
> > bits is generally bad as well.  I've seen it cause machine checks,
> > error interrupts, etc.
> > -- not just corrupting the page in question."
> >
> > So I added not mixing W & M. But at that time I missed to understood
> > why mixing M & I for same physical address can be issue :).
> 
> "W or M", not "W and M".  I meant that each one, separately, is in a similar
> situation as the I bit.
> 
> None of this is about invalid combinations of attributes on a single TLB entry
> (though there are architectural restrictions there as well).

Ok, I misread again :(. The second part of comment was (looks like you missed so copy pasted below)

" 
When we say all RAM (page_is_ram() is true) will be having "M" bit, then same RAM physical address will not have "M" mixed with any other, right?

Similarly, For IO (which is not RAM), we will set "I+G", so "I" will not be mixed with "M". Is not that?
"

-Bharat
> 
> -Scott

Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Scott Wood]

On 07/23/2013 11:50:35 AM, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, July 23, 2013 10:15 PM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; Alexander Graf; kvm-ppc@vger.kernel.org;
> > kvm@vger.kernel.org
> > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only  
> for kernel
> > managed pages
> >
> > On 07/22/2013 10:39:16 PM, Bhushan Bharat-R65777 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Tuesday, July 23, 2013 12:18 AM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: Wood Scott-B07421; Alexander Graf; kvm-ppc@vger.kernel.org;
> > > > kvm@vger.kernel.org
> > > > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency  
> only
> > > for kernel
> > > > managed pages
> > > >
> > > > On 07/21/2013 11:39:45 PM, Bhushan Bharat-R65777 wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Thursday, July 18, 2013 11:09 PM
> > > > > > To: Alexander Graf
> > > > > > Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;
> > > > > kvm@vger.kernel.org; Bhushan
> > > > > > Bharat-R65777
> > > > > > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache  
> coherency
> > > only
> > > > > for kernel
> > > > > > managed pages
> > > > > >
> > > > > > On 07/18/2013 12:32:18 PM, Alexander Graf wrote:
> > > > > > >
> > > > > > > On 18.07.2013, at 19:17, Scott Wood wrote:
> > > > > > >
> > > > > > > > On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > > > > > > > Likewise, we want to make sure this matches the host  
> entry.
> > > > > > > Unfortunately, this is a bit of a mess already.  64-bit  
> booke
> > > > > appears
> > > > > > > to always set MAS2_M for TLB0 mappings.  The initial
> > > KERNELBASE
> > > > > > > mapping on boot uses M_IF_SMP, and the settlbcam() that  
> (IIRC)
> > > > > > > replaces it uses _PAGE_COHERENT.  32-bit always uses
> > > > > _PAGE_COHERENT,
> > > > > > > except that initial KERNELBASE mapping.  _PAGE_COHERENT
> > > appears
> > > > > to be
> > > > > > > set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter
> > > config
> > > > > > > clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT  
> case).
> > > > > > > >
> > > > > > > > As for what we actually want to happen, there are cases
> > > when we
> > > > > > > want M to be set for non-SMP.  One such case is AMP, where
> > > CPUs
> > > > > may be
> > > > > > > sharing memory even if the Linux instance only runs on  
> one CPU
> > > > > (this
> > > > > > > is not hypothetical, BTW).  It's also possible that we
> > > encounter a
> > > > > > > hardware bug that requires MAS2_M, similar to what some  
> of our
> > > > > > > non-booke chips require.
> > > > > > >
> > > > > > > How about we always set M then for RAM?
> > > > > >
> > > > > > M is like I in that bad things happen if you mix them.
> > > > >
> > > > > I am trying to list the invalid mixing of WIMG:
> > > > >
> > > > >  1) I & M
> > > > >  2) W & I
> > > > >  3) W & M (Scott mentioned that he observed issues when   
> mixing
> > > these
> > > > > two)
> > > > >  4) is there any other?
> > > >
> > > > That's not what I was talking about (and I don't think I  
> mentioned
> > > W at all,
> > > > though it is also potentially problematic).
> > >
> > > Here is cut paste of your one response:
> > > "The architecture makes it illegal to mix cacheable and
> > > cache-inhibited mappings to the same physical page.  Mixing W or M
> > > bits is generally bad as well.  I've seen it cause machine checks,
> > > error interrupts, etc.
> > > -- not just corrupting the page in question."
> > >
> > > So I added not mixing W & M. But at that time I missed to  
> understood
> > > why mixing M & I for same physical address can be issue :).
> >
> > "W or M", not "W and M".  I meant that each one, separately, is in  
> a similar
> > situation as the I bit.
> >
> > None of this is about invalid combinations of attributes on a  
> single TLB entry
> > (though there are architectural restrictions there as well).
> 
> Ok, I misread again :(. The second part of comment was (looks like  
> you missed so copy pasted below)
> 
> "
> When we say all RAM (page_is_ram() is true) will be having "M" bit,  
> then same RAM physical address will not have "M" mixed with any  
> other, right?
> 
> Similarly, For IO (which is not RAM), we will set "I+G", so "I" will  
> not be mixed with "M". Is not that?
> "

I didn't miss it; it just seemed moot given the earlier confusion.  But  
yes, for now we will set all RAM to M, and all I/O to I+G.  Eventually  
that will change if/when we do vfio for QMan portals or other devices  
that require cacheable I/O.

-Scott

Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Scott Wood]

On 07/23/2013 06:13:58 AM, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, July 18, 2013 10:48 PM
> > To: Bhushan Bharat-R65777
> > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de;  
> Bhushan Bharat-
> > R65777
> > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only  
> for kernel
> > managed pages
> >
> > On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > > If there is a struct page for the requested mapping then it's  
> normal
> > > RAM and the mapping is set to "M" bit (coherent, cacheable)  
> otherwise
> > > this is treated as I/O and we set  "I + G"  (cache inhibited,  
> guarded)
> > >
> > > This helps setting proper TLB mapping for direct assigned device
> > >
> > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > > ---
> > > v2: some cleanup and added comment
> > >  -
> > >  arch/powerpc/kvm/e500_mmu_host.c |   23 ++++++++++++++++++-----
> > >  1 files changed, 18 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kvm/e500_mmu_host.c
> > > b/arch/powerpc/kvm/e500_mmu_host.c
> > > index 1c6a9d7..02eb973 100644
> > > --- a/arch/powerpc/kvm/e500_mmu_host.c
> > > +++ b/arch/powerpc/kvm/e500_mmu_host.c
> > > @@ -64,13 +64,26 @@ static inline u32 e500_shadow_mas3_attrib(u32
> > > mas3, int usermode)
> > >  	return mas3;
> > >  }
> > >
> > > -static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
> > > +static inline u32 e500_shadow_mas2_attrib(u32 mas2, pfn_t pfn)
> > >  {
> > > +	u32 mas2_attr;
> > > +
> > > +	mas2_attr = mas2 & MAS2_ATTRIB_MASK;
> > > +
> > > +	/*
> > > +	 * RAM is always mappable on e500 systems, so this is identical
> > > +	 * to kvm_is_mmio_pfn(), just without its overhead.
> > > +	 */
> > > +	if (!pfn_valid(pfn)) {
> >
> > Please use page_is_ram(), which is what gets used when setting the  
> WIMG for the
> > host userspace mapping.  We want to make sure the two are  
> consistent.
> >
> > > +		/* Pages not managed by Kernel are treated as I/O, set
> > > I + G */
> > > +		mas2_attr |= MAS2_I | MAS2_G;
> > >  #ifdef CONFIG_SMP
> > > -	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
> > > -#else
> > > -	return mas2 & MAS2_ATTRIB_MASK;
> > > +	} else {
> > > +		/* Kernel managed pages are actually RAM so set  "M" */
> > > +		mas2_attr |= MAS2_M;
> > >  #endif
> >
> > Likewise, we want to make sure this matches the host entry.
> > Unfortunately, this is a bit of a mess already.  64-bit booke  
> appears to always
> > set MAS2_M for TLB0 mappings.
> 
> Scott, can you please point to the code where MAS2_M is always set  
> for TLB0?

__early_init_mmu() sets MAS4[MD], without regard to CONFIG_SMP.   
However, the TLB miss handler replaces WIMGE with the flags from the  
PTE, so I guess MAS4 is only relevant for things like indirect PTEs and  
(for non-FSL chips) linear faults.

So never mind about 64-bit.  There's still the AMP case, though.

-Scott

RE: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel managed pages

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 23, 2013 11:50 PM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alexander Graf; kvm-ppc@vger.kernel.org;
> kvm@vger.kernel.org
> Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel
> managed pages
> 
> On 07/23/2013 11:50:35 AM, Bhushan Bharat-R65777 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, July 23, 2013 10:15 PM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; Alexander Graf; kvm-ppc@vger.kernel.org;
> > > kvm@vger.kernel.org
> > > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only
> > for kernel
> > > managed pages
> > >
> > > On 07/22/2013 10:39:16 PM, Bhushan Bharat-R65777 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Tuesday, July 23, 2013 12:18 AM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: Wood Scott-B07421; Alexander Graf; kvm-ppc@vger.kernel.org;
> > > > > kvm@vger.kernel.org
> > > > > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency
> > only
> > > > for kernel
> > > > > managed pages
> > > > >
> > > > > On 07/21/2013 11:39:45 PM, Bhushan Bharat-R65777 wrote:
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Wood Scott-B07421
> > > > > > > Sent: Thursday, July 18, 2013 11:09 PM
> > > > > > > To: Alexander Graf
> > > > > > > Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;
> > > > > > kvm@vger.kernel.org; Bhushan
> > > > > > > Bharat-R65777
> > > > > > > Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache
> > coherency
> > > > only
> > > > > > for kernel
> > > > > > > managed pages
> > > > > > >
> > > > > > > On 07/18/2013 12:32:18 PM, Alexander Graf wrote:
> > > > > > > >
> > > > > > > > On 18.07.2013, at 19:17, Scott Wood wrote:
> > > > > > > >
> > > > > > > > > On 07/18/2013 08:19:03 AM, Bharat Bhushan wrote:
> > > > > > > > > Likewise, we want to make sure this matches the host
> > entry.
> > > > > > > > Unfortunately, this is a bit of a mess already.  64-bit
> > booke
> > > > > > appears
> > > > > > > > to always set MAS2_M for TLB0 mappings.  The initial
> > > > KERNELBASE
> > > > > > > > mapping on boot uses M_IF_SMP, and the settlbcam() that
> > (IIRC)
> > > > > > > > replaces it uses _PAGE_COHERENT.  32-bit always uses
> > > > > > _PAGE_COHERENT,
> > > > > > > > except that initial KERNELBASE mapping.  _PAGE_COHERENT
> > > > appears
> > > > > > to be
> > > > > > > > set based on CONFIG_SMP || CONFIG_PPC_STD_MMU (the latter
> > > > config
> > > > > > > > clears _PAGE_COHERENT in the non-CPU_FTR_NEED_COHERENT
> > case).
> > > > > > > > >
> > > > > > > > > As for what we actually want to happen, there are cases
> > > > when we
> > > > > > > > want M to be set for non-SMP.  One such case is AMP, where
> > > > CPUs
> > > > > > may be
> > > > > > > > sharing memory even if the Linux instance only runs on
> > one CPU
> > > > > > (this
> > > > > > > > is not hypothetical, BTW).  It's also possible that we
> > > > encounter a
> > > > > > > > hardware bug that requires MAS2_M, similar to what some
> > of our
> > > > > > > > non-booke chips require.
> > > > > > > >
> > > > > > > > How about we always set M then for RAM?
> > > > > > >
> > > > > > > M is like I in that bad things happen if you mix them.
> > > > > >
> > > > > > I am trying to list the invalid mixing of WIMG:
> > > > > >
> > > > > >  1) I & M
> > > > > >  2) W & I
> > > > > >  3) W & M (Scott mentioned that he observed issues when
> > mixing
> > > > these
> > > > > > two)
> > > > > >  4) is there any other?
> > > > >
> > > > > That's not what I was talking about (and I don't think I
> > mentioned
> > > > W at all,
> > > > > though it is also potentially problematic).
> > > >
> > > > Here is cut paste of your one response:
> > > > "The architecture makes it illegal to mix cacheable and
> > > > cache-inhibited mappings to the same physical page.  Mixing W or M
> > > > bits is generally bad as well.  I've seen it cause machine checks,
> > > > error interrupts, etc.
> > > > -- not just corrupting the page in question."
> > > >
> > > > So I added not mixing W & M. But at that time I missed to
> > understood
> > > > why mixing M & I for same physical address can be issue :).
> > >
> > > "W or M", not "W and M".  I meant that each one, separately, is in
> > a similar
> > > situation as the I bit.
> > >
> > > None of this is about invalid combinations of attributes on a
> > single TLB entry
> > > (though there are architectural restrictions there as well).
> >
> > Ok, I misread again :(. The second part of comment was (looks like you
> > missed so copy pasted below)
> >
> > "
> > When we say all RAM (page_is_ram() is true) will be having "M" bit,
> > then same RAM physical address will not have "M" mixed with any other,
> > right?
> >
> > Similarly, For IO (which is not RAM), we will set "I+G", so "I" will
> > not be mixed with "M". Is not that?
> > "
> 
> I didn't miss it; it just seemed moot given the earlier confusion.  But yes, for
> now we will set all RAM to M, and all I/O to I+G.  Eventually that will change
> if/when we do vfio for QMan portals or other devices that require cacheable I/O.

Agree :)

-Bharat

> 
> -Scott