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

login
register
mail settings
Submitter Bharat Bhushan
Date July 18, 2013, 1:19 p.m.
Message ID <1374153543-10734-2-git-send-email-Bharat.Bhushan@freescale.com>
Download mbox | patch
Permalink /patch/260058/
State New
Headers show

Comments

Bharat Bhushan - July 18, 2013, 1:19 p.m.
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(-)
Alexander Graf - July 18, 2013, 2:53 p.m.
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

--
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
Bharat Bhushan - July 18, 2013, 3:15 p.m.
> -----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


--
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
Alexander Graf - July 18, 2013, 3:19 p.m.
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

--
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
Bharat Bhushan - July 23, 2013, 11:13 a.m.
> -----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

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

Patch

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);