Re: [PATCH kernel v4 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

On Fri, Jul 06, 2018 at 04:00:30PM +1000, Michael Ellerman wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
> > diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
> > index abb4364..11e1029 100644
> > --- a/arch/powerpc/mm/mmu_context_iommu.c
> > +++ b/arch/powerpc/mm/mmu_context_iommu.c
> > @@ -159,6 +161,7 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
> >  		goto unlock_exit;
> >  	}
> >  
> > +	mem->pageshift = __builtin_ctzl(ua | (entries << PAGE_SHIFT));
> 
> __builtin_ctzl(0) is undefined, so are we guaranteed that
> (ua | (entries << PAGE_SHIFT)) is never zero? I couldn't convince
> myself.

We certainly don't need to care about that case, since registering a
zero-length region (entries == 0) is no-op.  But maybe we need to
filter it above.

> > @@ -199,9 +202,17 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
> >  			}
> >  		}
> >  populate:
> > +		pageshift = PAGE_SHIFT;
> > +		if (PageCompound(page))
> > +			pageshift += compound_order(compound_head(page));
> > +		mem->pageshift = min(mem->pageshift, pageshift);
> >  		mem->hpas[i] = page_to_pfn(page) << PAGE_SHIFT;
> >  	}
> >  
> > +	/* We have an incomplete huge page, default to PAGE_SHIFT */
> > +	if (head)
> > +		mem->pageshift = PAGE_SHIFT;
> > +
> 
> You never set head AFIACS? (other than in the initialiser)

That looks like a leftover from the previous version.

> 
> cheers
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]