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

On Fri, Jul 06, 2018 at 04:00:30PM +1000, Michael Ellerman wrote:

Alexey Kardashevskiy [off-list ref] writes:

quoted

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.

quoted

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