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

[Scott Wood <scottwood@freescale.com>]

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