iommu/exynos: fix page table maintenance

PATCH In review Cho KyongHo 3 participants 5 messages Jul 5, 2013 – Jul 16, 2013

Subsystems (3)

Arm Samsung Exynos IOMMU Exynos sysmmu (IOMMU) driver

lore

Share & tools

lore

Cho KyongHo

author

2013-07-05 12:29 UTC #0 lore raw

PATCH v7 3/9 iommu/exynos: fix page table maintenance

This prevents allocating lv2 page table for the lv1 page table entry that already has 1MB page mapping. In addition some BUG_ON() is changed to WARN_ON().

Signed-off-by Cho KyongHo <[email protected]>

---
 drivers/iommu/exynos-iommu.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

drivers/iommu/exynos-iommu.c +26 -8

index e3be3e5..2bfe9fa 100644
@@ -862,12 +862,14 @@ static unsigned long *alloc_lv2entry(unsigned long *sent, unsigned long iova,
 		pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
 		BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
 		if (!pent)
-			return NULL;
+			return ERR_PTR(-ENOMEM);
 
 		*sent = mk_lv1ent_page(__pa(pent));
 		*pgcounter = NUM_LV2ENTRIES;
 		pgtable_flush(pent, pent + NUM_LV2ENTRIES);
 		pgtable_flush(sent, sent + 1);
+	} else if (lv1ent_section(sent)) {
+		return ERR_PTR(-EADDRINUSE);
 	}
 
 	return page_entry(sent, iova);

@@ -944,16 +946,16 @@ static int exynos_iommu_map(struct iommu_domain *domain, unsigned long iova,
 		pent = alloc_lv2entry(entry, iova,
 					&priv->lv2entcnt[lv1ent_offset(iova)]);
 
-		if (!pent)
-			ret = -ENOMEM;
+		if (IS_ERR(pent))
+			ret = PTR_ERR(pent);
 		else
 			ret = lv2set_page(pent, paddr, size,
 					&priv->lv2entcnt[lv1ent_offset(iova)]);
 	}
 
 	if (ret) {
-		pr_debug("%s: Failed to map iova 0x%lx/0x%x bytes\n",
-							__func__, iova, size);
+		pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n",
+						__func__, ret, size, iova);
 	}
 
 	spin_unlock_irqrestore(&priv->pgtablelock, flags);

@@ -968,6 +970,7 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain,
 	struct sysmmu_drvdata *data;
 	unsigned long flags;
 	unsigned long *ent;
+	size_t err_page;
 
 	BUG_ON(priv->pgtable == NULL);

@@ -976,7 +979,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain,
 	ent = section_entry(priv->pgtable, iova);
 
 	if (lv1ent_section(ent)) {
-		BUG_ON(size < SECT_SIZE);
+		if (WARN_ON(size < SECT_SIZE))
+			goto err;
 
 		*ent = 0;
 		pgtable_flush(ent, ent + 1);

@@ -1008,7 +1012,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain,
 	}
 
 	/* lv1ent_large(ent) == true here */
-	BUG_ON(size < LPAGE_SIZE);
+	if (WARN_ON(size < LPAGE_SIZE))
+		goto err;
 
 	memset(ent, 0, sizeof(*ent) * SPAGES_PER_LPAGE);
 	pgtable_flush(ent, ent + SPAGES_PER_LPAGE);

@@ -1023,8 +1028,21 @@ done:
 		sysmmu_tlb_invalidate_entry(data->dev, iova);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-
 	return size;
+err:
+	spin_unlock_irqrestore(&priv->pgtablelock, flags);
+
+	err_page = (
+		((unsigned long)ent - (unsigned long)priv->pgtable)
+			< (NUM_LV1ENTRIES * sizeof(long))
+			   ) ?  SECT_SIZE : LPAGE_SIZE;
+
+	pr_err("%s: Failed due to size(%#lx) @ %#x is"\
+		" smaller than page size %#x\n",
+		__func__, iova, size, err_page);
+
+	       return 0;
+
 }
 
 static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain,

Signature · 1 line

1.7.2.5

Bartlomiej Zolnierkiewicz

2013-07-11 17:37 UTC #1 lore raw

Hi,

Some minor nitpicks below.

Quoted 38 lines · Cho KyongHo

This prevents allocating lv2 page table for the lv1 page table entry that already has 1MB page mapping. In addition some BUG_ON() is changed to WARN_ON().

Signed-off-by Cho KyongHo <[email protected]>

---
 drivers/iommu/exynos-iommu.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

drivers/iommu/exynos-iommu.c +5 -3

index e3be3e5..2bfe9fa 100644
@@ -862,12 +862,14 @@ static unsigned long *alloc_lv2entry(unsigned long *sent, unsigned long iova,
 		pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
 		BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
 		if (!pent)
-			return NULL;
+			return ERR_PTR(-ENOMEM);
 
 		*sent = mk_lv1ent_page(__pa(pent));
 		*pgcounter = NUM_LV2ENTRIES;
 		pgtable_flush(pent, pent + NUM_LV2ENTRIES);
 		pgtable_flush(sent, sent + 1);
+	} else if (lv1ent_section(sent)) {
+		return ERR_PTR(-EADDRINUSE);
 	}
 
 	return page_entry(sent, iova);

@@ -944,16 +946,16 @@ static int exynos_iommu_map(struct iommu_domain *domain, unsigned long iova,
 		pent = alloc_lv2entry(entry, iova,
 					&priv->lv2entcnt[lv1ent_offset(iova)]);
 
-		if (!pent)
-			ret = -ENOMEM;
+		if (IS_ERR(pent))
+			ret = PTR_ERR(pent);
 		else

drivers/iommu/exynos-iommu.c static int exynos_iommu_map(struct iommu_domain *domain, unsigned long iova, · line 959

Quoted 10 lines

drivers/iommu/exynos-iommu.c +2 -2

 			ret = lv2set_page(pent, paddr, size,
 					&priv->lv2entcnt[lv1ent_offset(iova)]);
 	}
 
 	if (ret) {
-		pr_debug("%s: Failed to map iova 0x%lx/0x%x bytes\n",
-							__func__, iova, size);
+		pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n",
+						__func__, ret, size, iova);
 	}

Intendation is a bit weird, it should be more like:

		pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n",
			__func__, ret, size, iova);

to be consistent with the rest of the driver.

You could have also removed superfluous braces while at it.

Quoted 31 lines

drivers/iommu/exynos-iommu.c +5 -2

 	spin_unlock_irqrestore(&priv->pgtablelock, flags);

@@ -968,6 +970,7 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain,
 	struct sysmmu_drvdata *data;
 	unsigned long flags;
 	unsigned long *ent;
+	size_t err_page;
 
 	BUG_ON(priv->pgtable == NULL);

@@ -976,7 +979,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain,
 	ent = section_entry(priv->pgtable, iova);
 
 	if (lv1ent_section(ent)) {
-		BUG_ON(size < SECT_SIZE);
+		if (WARN_ON(size < SECT_SIZE))
+			goto err;
 
 		*ent = 0;
 		pgtable_flush(ent, ent + 1);

@@ -1008,7 +1012,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain,
 	}
 
 	/* lv1ent_large(ent) == true here */
-	BUG_ON(size < LPAGE_SIZE);
+	if (WARN_ON(size < LPAGE_SIZE))
+		goto err;
 
 	memset(ent, 0, sizeof(*ent) * SPAGES_PER_LPAGE);
 	pgtable_flush(ent, ent + SPAGES_PER_LPAGE);

@@ -1023,8 +1028,21 @@ done:
 		sysmmu_tlb_invalidate_entry(data->dev, iova);

drivers/iommu/exynos-iommu.c done: · line 1037

Quoted 10 lines

drivers/iommu/exynos-iommu.c +6 -1

 	spin_unlock_irqrestore(&priv->lock, flags);
 
-
 	return size;
+err:
+	spin_unlock_irqrestore(&priv->pgtablelock, flags);
+
+	err_page = (
+		((unsigned long)ent - (unsigned long)priv->pgtable)
+			< (NUM_LV1ENTRIES * sizeof(long))

Maybe you could add LV1TABLE_SIZE define and use it here (there is already a LV2TABLE_SIZE define)?

drivers/iommu/exynos-iommu.c done: · line 1038

Quoted 1 line

drivers/iommu/exynos-iommu.c +1 -0
+			   ) ?  SECT_SIZE : LPAGE_SIZE;

It also seems that err_page should be of unsigned long type, no need to make it size_t one.

The above code is quite ugly currently, it could be rewritten into something prettier, i.e.:

	err_page = (unsigned long)ent - (unsigned long)priv->pgtable;
	err_page = (err_page < LV1TABLE_SIZE) ? SECT_SIZE : LPAGE_SIZE;

drivers/iommu/exynos-iommu.c done: · line 1042

Quoted 3 lines

drivers/iommu/exynos-iommu.c +3 -0

+	pr_err("%s: Failed due to size(%#lx) @ %#x is"\
+		" smaller than page size %#x\n",
+		__func__, iova, size, err_page);

Aren't iova and size arguments interchanged here?

drivers/iommu/exynos-iommu.c done: · line 1043

Quoted 2 lines

drivers/iommu/exynos-iommu.c +2 -0
+
+	       return 0;

There is an intendation issue here (extra whitespaces).

drivers/iommu/exynos-iommu.c done: · line 1048

Quoted 4 lines

drivers/iommu/exynos-iommu.c +1 -0

+
 }
 
 static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain,

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Cho KyongHo

author

2013-07-15 11:21 UTC #2 lore raw

Quoted 60 lines

-----Original Message-----
From: Bartlomiej Zolnierkiewicz [mailto:b.zolnierkie at samsung.com]
Sent: Friday, July 12, 2013 2:38 AM

Hi,

Some minor nitpicks below.

Quoted 38 lines · Cho KyongHo

This prevents allocating lv2 page table for the lv1 page table entry that already has 1MB page mapping. In addition some BUG_ON() is changed to WARN_ON().

Signed-off-by Cho KyongHo <[email protected]>

---
 drivers/iommu/exynos-iommu.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

drivers/iommu/exynos-iommu.c +5 -3

index e3be3e5..2bfe9fa 100644
@@ -862,12 +862,14 @@ static unsigned long *alloc_lv2entry(unsigned long *sent, unsigned long iova,
 		pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
 		BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
 		if (!pent)
-			return NULL;
+			return ERR_PTR(-ENOMEM);
 
 		*sent = mk_lv1ent_page(__pa(pent));
 		*pgcounter = NUM_LV2ENTRIES;
 		pgtable_flush(pent, pent + NUM_LV2ENTRIES);
 		pgtable_flush(sent, sent + 1);
+	} else if (lv1ent_section(sent)) {
+		return ERR_PTR(-EADDRINUSE);
 	}
 
 	return page_entry(sent, iova);

@@ -944,16 +946,16 @@ static int exynos_iommu_map(struct iommu_domain *domain, unsigned long iova,
 		pent = alloc_lv2entry(entry, iova,
 					&priv->lv2entcnt[lv1ent_offset(iova)]);
 
-		if (!pent)
-			ret = -ENOMEM;
+		if (IS_ERR(pent))
+			ret = PTR_ERR(pent);
 		else

drivers/iommu/exynos-iommu.c static int exynos_iommu_map(struct iommu_domain *domain, unsigned long iova, · line 959

Quoted 10 lines

drivers/iommu/exynos-iommu.c +2 -2

 			ret = lv2set_page(pent, paddr, size,
 					&priv->lv2entcnt[lv1ent_offset(iova)]);
 	}
 
 	if (ret) {
-		pr_debug("%s: Failed to map iova 0x%lx/0x%x bytes\n",
-							__func__, iova, size);
+		pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n",
+						__func__, ret, size, iova);
 	}

Intendation is a bit weird, it should be more like:

Quoted 7 lines

		pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n",
			__func__, ret, size, iova);
to be consistent with the rest of the driver.
You could have also removed superfluous braces while at it.

Do you think it is better to read the code?
Hmm, I think it is better too.
Thanks.

Quoted 33 lines

drivers/iommu/exynos-iommu.c +5 -2

 	spin_unlock_irqrestore(&priv->pgtablelock, flags);

@@ -968,6 +970,7 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain,
 	struct sysmmu_drvdata *data;
 	unsigned long flags;
 	unsigned long *ent;
+	size_t err_page;
 
 	BUG_ON(priv->pgtable == NULL);

@@ -976,7 +979,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain,
 	ent = section_entry(priv->pgtable, iova);
 
 	if (lv1ent_section(ent)) {
-		BUG_ON(size < SECT_SIZE);
+		if (WARN_ON(size < SECT_SIZE))
+			goto err;
 
 		*ent = 0;
 		pgtable_flush(ent, ent + 1);

@@ -1008,7 +1012,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain,
 	}
 
 	/* lv1ent_large(ent) == true here */
-	BUG_ON(size < LPAGE_SIZE);
+	if (WARN_ON(size < LPAGE_SIZE))
+		goto err;
 
 	memset(ent, 0, sizeof(*ent) * SPAGES_PER_LPAGE);
 	pgtable_flush(ent, ent + SPAGES_PER_LPAGE);

@@ -1023,8 +1028,21 @@ done:
 		sysmmu_tlb_invalidate_entry(data->dev, iova);
 	spin_unlock_irqrestore(&priv->lock, flags);

Quoted 11 lines

drivers/iommu/exynos-iommu.c done: · line 1037
Quoted 8 lines
drivers/iommu/exynos-iommu.c +6 -1
-
 	return size;
+err:
+	spin_unlock_irqrestore(&priv->pgtablelock, flags);
+
+	err_page = (
+		((unsigned long)ent - (unsigned long)priv->pgtable)
+			< (NUM_LV1ENTRIES * sizeof(long))
Maybe you could add LV1TABLE_SIZE define and use it here (there is already a LV2TABLE_SIZE define)?

Yes. But, LV2TABLE_SIZE is used in more places than one.
I do not feel that it is needed to define LV1TABLE_SIZE for the single line.

Quoted 6 lines

drivers/iommu/exynos-iommu.c done: · line 1038
Quoted 2 lines
drivers/iommu/exynos-iommu.c +1 -0
+			   ) ?  SECT_SIZE : LPAGE_SIZE;
It also seems that err_page should be of unsigned long type, no need to make it size_t one.

err_page is the page size.
I agree that the name of the variable is not proper to indicate size.

Quoted 6 lines

The above code is quite ugly currently, it could be rewritten into something prettier, i.e.:
	err_page = (unsigned long)ent - (unsigned long)priv->pgtable;
	err_page = (err_page < LV1TABLE_SIZE) ? SECT_SIZE : LPAGE_SIZE;

Ah, this is the reason that you addresses err_page could be unsigned long. I agree that it is prettier.

Quoted 5 lines

drivers/iommu/exynos-iommu.c done: · line 1042
Quoted 3 lines
drivers/iommu/exynos-iommu.c +3 -0
+	pr_err("%s: Failed due to size(%#lx) @ %#x is"\
+		" smaller than page size %#x\n",
+		__func__, iova, size, err_page);
Aren't iova and size arguments interchanged here?

Oh, it is my mistake.

It will should be fixed in the next patchset with the change in calculation of err_page.

Quoted 5 lines

drivers/iommu/exynos-iommu.c done: · line 1043
Quoted 3 lines
drivers/iommu/exynos-iommu.c +2 -0
+
+	       return 0;
There is an intendation issue here (extra whitespaces).

Yes. Thanks.

Quoted 11 lines

drivers/iommu/exynos-iommu.c done: · line 1048
Quoted 5 lines
drivers/iommu/exynos-iommu.c +1 -0
+
 }
 
 static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain,
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Thank you.

Cho KyongHo.

Grant Grundler

2013-07-15 16:18 UTC #3 lore raw

On Mon, Jul 15, 2013 at 4:21 AM, Cho KyongHo <[email protected]> wrote:
...

Quoted 5 lines

Quoted 2 lines
Maybe you could add LV1TABLE_SIZE define and use it here (there is already a LV2TABLE_SIZE define)?
Yes. But, LV2TABLE_SIZE is used in more places than one.
I do not feel that it is needed to define LV1TABLE_SIZE for the single line.

Cho,
#define's are part of the code "documentation". Doesn't really matter how often it's used. I think Bartlomiej's suggestion is a good one.

Key is to use "consistent" names so they makes sense to someone not familiar with the code. In this case, we want to show this use is the same way LV2TABLE_SIZE is used and serves the same purpose.

cheers.
grant

Cho KyongHo

author

2013-07-16 13:07 UTC #4 lore raw

Quoted 11 lines

From: grundler at google.com [mailto:grundler at google.com] On Behalf Of Grant Grundler
Sent: Tuesday, July 16, 2013 1:19 AM
On Mon, Jul 15, 2013 at 4:21 AM, Cho KyongHo <[email protected]> wrote:
...
Quoted 5 lines
Quoted 2 lines
Maybe you could add LV1TABLE_SIZE define and use it here (there is already a LV2TABLE_SIZE define)?
Yes. But, LV2TABLE_SIZE is used in more places than one.
I do not feel that it is needed to define LV1TABLE_SIZE for the single line.

Quoted 8 lines

Cho,
#define's are part of the code "documentation". Doesn't really matter how often it's used. I think Bartlomiej's suggestion is a good one.
Key is to use "consistent" names so they makes sense to someone not familiar with the code. In this case, we want to show this use is the same way LV2TABLE_SIZE is used and serves the same purpose.

Ok.
I agree the error handling code is not pretty to understand. I will change it in the next patch.

Quoted 2 lines

cheers.
grant

Thank you.

Cho KyongHo.