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

[Bhushan Bharat-R65777 <R65777@freescale.com>]

> -----Original Message-----
> From: Bhushan Bharat-R65777
> Sent: Thursday, July 18, 2013 1:53 PM
> To: '"“tiejun.chen”"'
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; Wood Scott-
> B07421
> Subject: RE: [PATCH 2/2] kvm: powerpc: set cache coherency only for kernel
> managed pages
> 
> 
> 
> > -----Original Message-----
> > From: "“tiejun.chen”" [mailto:tiejun.chen@windriver.com]
> > Sent: Thursday, July 18, 2013 1:52 PM
> > To: Bhushan Bharat-R65777
> > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; Wood
> > Scott-
> > B07421
> > Subject: Re: [PATCH 2/2] kvm: powerpc: set cache coherency only for
> > kernel managed pages
> >
> > On 07/18/2013 04:08 PM, Bhushan Bharat-R65777 wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: kvm-ppc-owner@vger.kernel.org
> > >> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of "“tiejun.chen”"
> > >> Sent: Thursday, July 18, 2013 1:01 PM
> > >> To: Bhushan Bharat-R65777
> > >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de;
> > >> Wood
> > >> Scott-
> > >> B07421
> > >> Subject: Re: [PATCH 2/2] kvm: powerpc: set cache coherency only for
> > >> kernel managed pages
> > >>
> > >> On 07/18/2013 03:12 PM, Bhushan Bharat-R65777 wrote:
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: "“tiejun.chen”" [mailto:tiejun.chen@windriver.com]
> > >>>> Sent: Thursday, July 18, 2013 11:56 AM
> > >>>> To: Bhushan Bharat-R65777
> > >>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de;
> > >>>> Wood
> > >>>> Scott- B07421; Bhushan Bharat-R65777
> > >>>> Subject: Re: [PATCH 2/2] kvm: powerpc: set cache coherency only
> > >>>> for kernel managed pages
> > >>>>
> > >>>> On 07/18/2013 02:04 PM, Bharat Bhushan wrote:
> > >>>>> If there is a struct page for the requested mapping then it's
> > >>>>> normal DDR and the mapping sets "M" bit (coherent, cacheable)
> > >>>>> else 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>
> > >>>>> ---
> > >>>>>     arch/powerpc/kvm/e500_mmu_host.c |   17 ++++++++++++-----
> > >>>>>     1 files changed, 12 insertions(+), 5 deletions(-)
> > >>>>>
> > >>>>> diff --git a/arch/powerpc/kvm/e500_mmu_host.c
> > >>>>> b/arch/powerpc/kvm/e500_mmu_host.c
> > >>>>> index 1c6a9d7..089c227 100644
> > >>>>> --- a/arch/powerpc/kvm/e500_mmu_host.c
> > >>>>> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> > >>>>> @@ -64,13 +64,20 @@ 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;
> > >>>>> +
> > >>>>> +	if (!pfn_valid(pfn)) {
> > >>>>
> > >>>> Why not directly use kvm_is_mmio_pfn()?
> > >>>
> > >>> What I understand from this function (someone can correct me) is
> > >>> that it
> > >> returns "false" when the page is managed by kernel and is not
> > >> marked as RESERVED (for some reason). For us it does not matter
> > >> whether the page is reserved or not, if it is kernel visible page then it
> is DDR.
> > >>>
> > >>
> > >> I think you are setting I|G by addressing all mmio pages, right? If
> > >> so,
> > >>
> > >>       KVM: direct mmio pfn check
> > >>
> > >>       Userspace may specify memory slots that are backed by mmio
> > >> pages rather than
> > >>       normal RAM.  In some cases it is not enough to identify these
> > >> mmio
> > pages
> > >>       by pfn_valid().  This patch adds checking the PageReserved as well.
> > >
> > > Do you know what are those "some cases" and how checking
> > > PageReserved helps in
> > those cases?
> >
> > No, myself didn't see these actual cases in qemu,too. But this should
> > be chronically persistent as I understand ;-)
> 
> Then I will wait till someone educate me :)

The reason is , kvm_is_mmio_pfn() function looks pretty heavy and I do not want to call this for all tlbwe operation unless it is necessary.

-Bharat

> > >>>>> +		mas2_attr |= MAS2_I | MAS2_G;
> > >>>>> +	} else {
> > >>>>>     #ifdef CONFIG_SMP
> > >>>>> -	return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
> > >>>>> -#else
> > >>>>> -	return mas2 & MAS2_ATTRIB_MASK;
> > >>>>> +		mas2_attr |= MAS2_M;
> > >>>>>     #endif
> > >>>>> +	}
> > >>>>> +	return mas2_attr;
> > >>>>>     }
> > >>>>>
> > >>>>>     /*
> > >>>>> @@ -313,7 +320,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);
> > >>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >> --
> > >> 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
> > >
> >