[PATCH 6/6 v3] kvm: powerpc: use caching attributes as per linux pte

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Saturday, August 10, 2013 6:35 AM
> To: Bhushan Bharat-R65777
> Cc: benh at kernel.crashing.org; agraf at suse.de; paulus at samba.org;
> kvm at vger.kernel.org; kvm-ppc at vger.kernel.org; linuxppc-dev at lists.ozlabs.org;
> Bhushan Bharat-R65777
> Subject: Re: [PATCH 6/6 v3] kvm: powerpc: use caching attributes as per linux
> pte
> 
> On Tue, 2013-08-06 at 17:01 +0530, Bharat Bhushan wrote:
> > @@ -449,7 +446,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 {
> > +		printk(KERN_ERR "pte not present: gfn %lx, pfn %lx\n",
> > +				(long)gfn, pfn);
> > +		return -EINVAL;
> 
> Don't let the guest spam the host kernel console by repeatedly accessing bad
> mappings (even if it requires host userspace to assist by pointing a memslot at
> a bad hva).  This should at most be printk_ratelimited(), and probably just
> pr_debug().  It should also have __func__ context.

Very good point, I will make this printk_ratelimited() in this patch. And convert this and other error prints to pr_debug() when we will send machine check on error in this flow.

> 
> Also, I don't see the return value getting checked (the immediate callers check
> it and propogate the error, but kvmppc_mmu_map() doesn't).
> We want to send a machine check to the guest if this happens (or possibly exit
> to userspace since it indicates a bad memslot, not just a guest bug).  We don't
> want to just silently retry over and over.

I completely agree with you, but this was something already missing (error return by this function is nothing new added in this patch), So I would like to take that separately.

> 
> Otherwise, this series looks good to me.

Thank you. :)
-Bharat

> 
> -Scott
>