diff mbox series

powerpc/npu: Cleanup MMIO ATSD flushing

Message ID 20171113081611.27374-1-bsingharora@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series powerpc/npu: Cleanup MMIO ATSD flushing | expand

Commit Message

Balbir Singh Nov. 13, 2017, 8:16 a.m. UTC 
While reviewing the code I found that the flush assumes all
pages are of mmu_linear_psize, which is not correct. The patch
uses find_linux_pte to find the right page size and uses that
for launching the ATSD invalidation. A new helper is added
to abstract the invalidation from the various notifiers.

The patch also cleans up a bit by removing AP size from PID
flushes.

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---

Changelog since RFC
 - Implement review comments from Aneesh

 arch/powerpc/platforms/powernv/npu-dma.c | 66 ++++++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 17 deletions(-)

Comments

Aneesh Kumar K.V Nov. 16, 2017, 6:24 a.m. UTC | #1 
Balbir Singh <bsingharora@gmail.com> writes:

 +	address = start;
> +	do {
> +		local_irq_disable();
> +		find_linux_pte(mm->pgd, address, &is_thp, &hshift);
> +		if (!is_thp)
> +			shift = PAGE_SHIFT;

It can still be hugetlb if is_thp is false.

> +		else if (hshift && !is_thp)
> +			shift = hshift;
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +		else
> +			shift = HPAGE_PMD_SIZE;

That is wrong. I guess it should be shift = HPAGE_PMD_SHIFT. But i am
not sure we need to make it this complex at all. See below.

> +#else
> +		else {
> +			shift = PAGE_SHIFT;
> +			pr_warn_once("unsupport page size for mm %p,addr %lx\n",
> +					mm, start);
> +		}
> +#endif

I am still not sure this is correct from a pure page table walking
point. Why not

       if (hshift)
          shift = hshift;
       else
          shift = PAGE_SHIFT;

if you didn't want to differentiate between thp and hugetlb mapping you
can drop is_thp completely.

-aneesh
Balbir Singh Nov. 17, 2017, 2:49 a.m. UTC | #2 
On Thu, Nov 16, 2017 at 5:24 PM, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
>
>  +      address = start;
>> +     do {
>> +             local_irq_disable();
>> +             find_linux_pte(mm->pgd, address, &is_thp, &hshift);
>> +             if (!is_thp)
>> +                     shift = PAGE_SHIFT;
>
> It can still be hugetlb if is_thp is false.
>

Yep, the hshift check needs to be upfront, the version below makes sense.

>> +             else if (hshift && !is_thp)
>> +                     shift = hshift;
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +             else
>> +                     shift = HPAGE_PMD_SIZE;
>
> That is wrong. I guess it should be shift = HPAGE_PMD_SHIFT. But i am
> not sure we need to make it this complex at all. See below.
>
>> +#else
>> +             else {
>> +                     shift = PAGE_SHIFT;
>> +                     pr_warn_once("unsupport page size for mm %p,addr %lx\n",
>> +                                     mm, start);
>> +             }
>> +#endif
>
> I am still not sure this is correct from a pure page table walking
> point. Why not
>
>        if (hshift)
>           shift = hshift;
>        else
>           shift = PAGE_SHIFT;
>

I don't think I care about THP at this point

I'll respin

Balbir
diff mbox series

Patch

diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index 2cb6cbea4b3b..bf12a023a8de 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -17,6 +17,7 @@ 
 #include <linux/pci.h>
 #include <linux/memblock.h>
 #include <linux/iommu.h>
+#include <linux/huge_mm.h>
 
 #include <asm/tlb.h>
 #include <asm/powernv.h>
@@ -27,6 +28,7 @@ 
 #include <asm/pnv-pci.h>
 #include <asm/msi_bitmap.h>
 #include <asm/opal.h>
+#include <asm/pte-walk.h>
 
 #include "powernv.h"
 #include "pci.h"
@@ -459,9 +461,6 @@  static int mmio_invalidate_pid(struct npu *npu, unsigned long pid, bool flush)
 	/* PRS set to process-scoped */
 	launch |= PPC_BIT(13);
 
-	/* AP */
-	launch |= (u64) mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
-
 	/* PID */
 	launch |= pid << PPC_BITLSHIFT(38);
 
@@ -473,7 +472,8 @@  static int mmio_invalidate_pid(struct npu *npu, unsigned long pid, bool flush)
 }
 
 static int mmio_invalidate_va(struct npu *npu, unsigned long va,
-			unsigned long pid, bool flush)
+			unsigned long pid, bool flush,
+			unsigned int shift)
 {
 	unsigned long launch;
 
@@ -484,9 +484,8 @@  static int mmio_invalidate_va(struct npu *npu, unsigned long va,
 	launch |= PPC_BIT(13);
 
 	/* AP */
-	launch |= (u64) mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
+	launch |= (u64) mmu_get_ap(shift) << PPC_BITLSHIFT(17);
 
-	/* PID */
 	launch |= pid << PPC_BITLSHIFT(38);
 
 	/* No flush */
@@ -503,7 +502,8 @@  struct mmio_atsd_reg {
 };
 
 static void mmio_invalidate_wait(
-	struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS], bool flush)
+	struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS], bool flush,
+	unsigned int shift)
 {
 	struct npu *npu;
 	int i, reg;
@@ -536,7 +536,8 @@  static void mmio_invalidate_wait(
  * the value of va.
  */
 static void mmio_invalidate(struct npu_context *npu_context, int va,
-			unsigned long address, bool flush)
+			unsigned long address, bool flush,
+			unsigned int shift)
 {
 	int i, j;
 	struct npu *npu;
@@ -569,7 +570,7 @@  static void mmio_invalidate(struct npu_context *npu_context, int va,
 			if (va)
 				mmio_atsd_reg[i].reg =
 					mmio_invalidate_va(npu, address, pid,
-							flush);
+							flush, shift);
 			else
 				mmio_atsd_reg[i].reg =
 					mmio_invalidate_pid(npu, pid, flush);
@@ -582,10 +583,43 @@  static void mmio_invalidate(struct npu_context *npu_context, int va,
 		}
 	}
 
-	mmio_invalidate_wait(mmio_atsd_reg, flush);
+	mmio_invalidate_wait(mmio_atsd_reg, flush, shift);
 	if (flush)
 		/* Wait for the flush to complete */
-		mmio_invalidate_wait(mmio_atsd_reg, false);
+		mmio_invalidate_wait(mmio_atsd_reg, false, shift);
+}
+
+static void pnv_npu2_invalidate_helper(struct npu_context *npu_context,
+		struct mm_struct *mm, unsigned long start,
+		unsigned long end, bool flush)
+{
+	unsigned long address;
+	bool is_thp = false;
+	unsigned int hshift = 0, shift;
+
+	address = start;
+	do {
+		local_irq_disable();
+		find_linux_pte(mm->pgd, address, &is_thp, &hshift);
+		if (!is_thp)
+			shift = PAGE_SHIFT;
+		else if (hshift && !is_thp)
+			shift = hshift;
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+		else
+			shift = HPAGE_PMD_SIZE;
+#else
+		else {
+			shift = PAGE_SHIFT;
+			pr_warn_once("unsupport page size for mm %p,addr %lx\n",
+					mm, start);
+		}
+#endif
+		mmio_invalidate(npu_context, address > 0, address, flush,
+				shift);
+		local_irq_enable();
+		address += (1ull << shift);
+	} while (address < end);
 }
 
 static void pnv_npu2_mn_release(struct mmu_notifier *mn,
@@ -601,7 +635,7 @@  static void pnv_npu2_mn_release(struct mmu_notifier *mn,
 	 * There should be no more translation requests for this PID, but we
 	 * need to ensure any entries for it are removed from the TLB.
 	 */
-	mmio_invalidate(npu_context, 0, 0, true);
+	pnv_npu2_invalidate_helper(npu_context, mm, 0, PAGE_SIZE, true);
 }
 
 static void pnv_npu2_mn_change_pte(struct mmu_notifier *mn,
@@ -611,7 +645,7 @@  static void pnv_npu2_mn_change_pte(struct mmu_notifier *mn,
 {
 	struct npu_context *npu_context = mn_to_npu_context(mn);
 
-	mmio_invalidate(npu_context, 1, address, true);
+	pnv_npu2_invalidate_helper(npu_context, mm, address, address, true);
 }
 
 static void pnv_npu2_mn_invalidate_range(struct mmu_notifier *mn,
@@ -619,13 +653,11 @@  static void pnv_npu2_mn_invalidate_range(struct mmu_notifier *mn,
 					unsigned long start, unsigned long end)
 {
 	struct npu_context *npu_context = mn_to_npu_context(mn);
-	unsigned long address;
 
-	for (address = start; address < end; address += PAGE_SIZE)
-		mmio_invalidate(npu_context, 1, address, false);
+	pnv_npu2_invalidate_helper(npu_context, mm, start, end, false);
 
 	/* Do the flush only on the final addess == end */
-	mmio_invalidate(npu_context, 1, address, true);
+	pnv_npu2_invalidate_helper(npu_context, mm, end, end, true);
 }
 
 static const struct mmu_notifier_ops nv_nmmu_notifier_ops = {