linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iommu/sun50i: Convert to map_pages/unmap_pages
@ 2023-09-12 18:19 Jernej Skrabec
  2023-09-13 20:20 ` Robin Murphy
  2023-09-25 16:19 ` Robin Murphy
  0 siblings, 2 replies; 4+ messages in thread
From: Jernej Skrabec @ 2023-09-12 18:19 UTC (permalink / raw)
  To: joro, will, robin.murphy
  Cc: wens, samuel, iommu, linux-arm-kernel, linux-sunxi, linux-kernel,
	Jernej Skrabec

Convert driver to use map_pages and unmap_pages. Since functions operate
on page table, extend them to be able to operate on whole table, not
just one entry.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
Hi!

I'm not sure if it makes sense to check validity of page table entry when
unmaping pages makes sense. What do you think?

Best regards,
Jernej

 drivers/iommu/sun50i-iommu.c | 55 +++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 74c5cb93e900..be6102730a56 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -589,11 +589,12 @@ static u32 *sun50i_dte_get_page_table(struct sun50i_iommu_domain *sun50i_domain,
 }
 
 static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
-			    phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+			    phys_addr_t paddr, size_t pgsize, size_t pgcount,
+			    int prot, gfp_t gfp, size_t *mapped)
 {
 	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
 	struct sun50i_iommu *iommu = sun50i_domain->iommu;
-	u32 pte_index;
+	u32 pte_index, count, i;
 	u32 *page_table, *pte_addr;
 	int ret = 0;
 
@@ -604,45 +605,55 @@ static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
 	}
 
 	pte_index = sun50i_iova_get_pte_index(iova);
-	pte_addr = &page_table[pte_index];
-	if (unlikely(sun50i_pte_is_page_valid(*pte_addr))) {
-		phys_addr_t page_phys = sun50i_pte_get_page_address(*pte_addr);
-		dev_err(iommu->dev,
-			"iova %pad already mapped to %pa cannot remap to %pa prot: %#x\n",
-			&iova, &page_phys, &paddr, prot);
-		ret = -EBUSY;
-		goto out;
+	count = min(pgcount, (size_t)NUM_PT_ENTRIES - pte_index);
+	for (i = 0; i < count; i++) {
+		pte_addr = &page_table[pte_index + i];
+		if (unlikely(sun50i_pte_is_page_valid(*pte_addr))) {
+			phys_addr_t page_phys = sun50i_pte_get_page_address(*pte_addr);
+
+			dev_err(iommu->dev,
+				"iova %pad already mapped to %pa cannot remap to %pa prot: %#x\n",
+				&iova, &page_phys, &paddr, prot);
+			ret = -EBUSY;
+			goto out;
+		}
+		*pte_addr = sun50i_mk_pte(paddr, prot);
+		paddr += SPAGE_SIZE;
 	}
 
-	*pte_addr = sun50i_mk_pte(paddr, prot);
-	sun50i_table_flush(sun50i_domain, pte_addr, 1);
+	sun50i_table_flush(sun50i_domain, &page_table[pte_index], i);
+	*mapped = i * SPAGE_SIZE;
 
 out:
 	return ret;
 }
 
 static size_t sun50i_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
-				 size_t size, struct iommu_iotlb_gather *gather)
+				 size_t pgsize, size_t pgcount,
+				 struct iommu_iotlb_gather *gather)
 {
 	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
+	u32 dte, count, i, pte_index;
 	phys_addr_t pt_phys;
 	u32 *pte_addr;
-	u32 dte;
 
 	dte = sun50i_domain->dt[sun50i_iova_get_dte_index(iova)];
 	if (!sun50i_dte_is_pt_valid(dte))
 		return 0;
 
 	pt_phys = sun50i_dte_get_pt_address(dte);
-	pte_addr = (u32 *)phys_to_virt(pt_phys) + sun50i_iova_get_pte_index(iova);
+	pte_index = sun50i_iova_get_pte_index(iova);
+	pte_addr = (u32 *)phys_to_virt(pt_phys) + pte_index;
 
-	if (!sun50i_pte_is_page_valid(*pte_addr))
-		return 0;
+	count = min(pgcount, (size_t)NUM_PT_ENTRIES - pte_index);
+	for (i = 0; i < count; i++)
+		if (!sun50i_pte_is_page_valid(pte_addr[i]))
+			break;
 
-	memset(pte_addr, 0, sizeof(*pte_addr));
-	sun50i_table_flush(sun50i_domain, pte_addr, 1);
+	memset(pte_addr, 0, sizeof(*pte_addr) * i);
+	sun50i_table_flush(sun50i_domain, pte_addr, i);
 
-	return SZ_4K;
+	return i * SPAGE_SIZE;
 }
 
 static phys_addr_t sun50i_iommu_iova_to_phys(struct iommu_domain *domain,
@@ -838,8 +849,8 @@ static const struct iommu_ops sun50i_iommu_ops = {
 		.iotlb_sync_map = sun50i_iommu_iotlb_sync_map,
 		.iotlb_sync	= sun50i_iommu_iotlb_sync,
 		.iova_to_phys	= sun50i_iommu_iova_to_phys,
-		.map		= sun50i_iommu_map,
-		.unmap		= sun50i_iommu_unmap,
+		.map_pages	= sun50i_iommu_map,
+		.unmap_pages	= sun50i_iommu_unmap,
 		.free		= sun50i_iommu_domain_free,
 	}
 };
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] iommu/sun50i: Convert to map_pages/unmap_pages
  2023-09-12 18:19 [PATCH] iommu/sun50i: Convert to map_pages/unmap_pages Jernej Skrabec
@ 2023-09-13 20:20 ` Robin Murphy
  2023-09-14 15:09   ` Jernej Škrabec
  2023-09-25 16:19 ` Robin Murphy
  1 sibling, 1 reply; 4+ messages in thread
From: Robin Murphy @ 2023-09-13 20:20 UTC (permalink / raw)
  To: Jernej Skrabec, joro, will
  Cc: wens, samuel, iommu, linux-arm-kernel, linux-sunxi, linux-kernel

On 2023-09-12 19:19, Jernej Skrabec wrote:
> Convert driver to use map_pages and unmap_pages. Since functions operate
> on page table, extend them to be able to operate on whole table, not
> just one entry.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
> Hi!
> 
> I'm not sure if it makes sense to check validity of page table entry when
> unmaping pages makes sense. What do you think?

As things currently stand it's largely a matter of opinion - some 
drivers consider that unmapping something which hasn't been mapped is 
nonsensical, and almost always represents the caller having gone wrong, 
thus report it as an error; others take the view that as long as they 
can achieve the requested end result, they're not going to think too 
hard about how they got there. The same arguments apply to whether you'd 
quietly replace an existing PTE or not when mapping, so I'd say the main 
thing is to at least be self-consistent one way or the other.

Cheers,
Robin.

> 
> Best regards,
> Jernej
> 
>   drivers/iommu/sun50i-iommu.c | 55 +++++++++++++++++++++---------------
>   1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> index 74c5cb93e900..be6102730a56 100644
> --- a/drivers/iommu/sun50i-iommu.c
> +++ b/drivers/iommu/sun50i-iommu.c
> @@ -589,11 +589,12 @@ static u32 *sun50i_dte_get_page_table(struct sun50i_iommu_domain *sun50i_domain,
>   }
>   
>   static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
> -			    phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> +			    phys_addr_t paddr, size_t pgsize, size_t pgcount,
> +			    int prot, gfp_t gfp, size_t *mapped)
>   {
>   	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
>   	struct sun50i_iommu *iommu = sun50i_domain->iommu;
> -	u32 pte_index;
> +	u32 pte_index, count, i;
>   	u32 *page_table, *pte_addr;
>   	int ret = 0;
>   
> @@ -604,45 +605,55 @@ static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
>   	}
>   
>   	pte_index = sun50i_iova_get_pte_index(iova);
> -	pte_addr = &page_table[pte_index];
> -	if (unlikely(sun50i_pte_is_page_valid(*pte_addr))) {
> -		phys_addr_t page_phys = sun50i_pte_get_page_address(*pte_addr);
> -		dev_err(iommu->dev,
> -			"iova %pad already mapped to %pa cannot remap to %pa prot: %#x\n",
> -			&iova, &page_phys, &paddr, prot);
> -		ret = -EBUSY;
> -		goto out;
> +	count = min(pgcount, (size_t)NUM_PT_ENTRIES - pte_index);
> +	for (i = 0; i < count; i++) {
> +		pte_addr = &page_table[pte_index + i];
> +		if (unlikely(sun50i_pte_is_page_valid(*pte_addr))) {
> +			phys_addr_t page_phys = sun50i_pte_get_page_address(*pte_addr);
> +
> +			dev_err(iommu->dev,
> +				"iova %pad already mapped to %pa cannot remap to %pa prot: %#x\n",
> +				&iova, &page_phys, &paddr, prot);
> +			ret = -EBUSY;
> +			goto out;
> +		}
> +		*pte_addr = sun50i_mk_pte(paddr, prot);
> +		paddr += SPAGE_SIZE;
>   	}
>   
> -	*pte_addr = sun50i_mk_pte(paddr, prot);
> -	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> +	sun50i_table_flush(sun50i_domain, &page_table[pte_index], i);
> +	*mapped = i * SPAGE_SIZE;
>   
>   out:
>   	return ret;
>   }
>   
>   static size_t sun50i_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
> -				 size_t size, struct iommu_iotlb_gather *gather)
> +				 size_t pgsize, size_t pgcount,
> +				 struct iommu_iotlb_gather *gather)
>   {
>   	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
> +	u32 dte, count, i, pte_index;
>   	phys_addr_t pt_phys;
>   	u32 *pte_addr;
> -	u32 dte;
>   
>   	dte = sun50i_domain->dt[sun50i_iova_get_dte_index(iova)];
>   	if (!sun50i_dte_is_pt_valid(dte))
>   		return 0;
>   
>   	pt_phys = sun50i_dte_get_pt_address(dte);
> -	pte_addr = (u32 *)phys_to_virt(pt_phys) + sun50i_iova_get_pte_index(iova);
> +	pte_index = sun50i_iova_get_pte_index(iova);
> +	pte_addr = (u32 *)phys_to_virt(pt_phys) + pte_index;
>   
> -	if (!sun50i_pte_is_page_valid(*pte_addr))
> -		return 0;
> +	count = min(pgcount, (size_t)NUM_PT_ENTRIES - pte_index);
> +	for (i = 0; i < count; i++)
> +		if (!sun50i_pte_is_page_valid(pte_addr[i]))
> +			break;
>   
> -	memset(pte_addr, 0, sizeof(*pte_addr));
> -	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> +	memset(pte_addr, 0, sizeof(*pte_addr) * i);
> +	sun50i_table_flush(sun50i_domain, pte_addr, i);
>   
> -	return SZ_4K;
> +	return i * SPAGE_SIZE;
>   }
>   
>   static phys_addr_t sun50i_iommu_iova_to_phys(struct iommu_domain *domain,
> @@ -838,8 +849,8 @@ static const struct iommu_ops sun50i_iommu_ops = {
>   		.iotlb_sync_map = sun50i_iommu_iotlb_sync_map,
>   		.iotlb_sync	= sun50i_iommu_iotlb_sync,
>   		.iova_to_phys	= sun50i_iommu_iova_to_phys,
> -		.map		= sun50i_iommu_map,
> -		.unmap		= sun50i_iommu_unmap,
> +		.map_pages	= sun50i_iommu_map,
> +		.unmap_pages	= sun50i_iommu_unmap,
>   		.free		= sun50i_iommu_domain_free,
>   	}
>   };

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iommu/sun50i: Convert to map_pages/unmap_pages
  2023-09-13 20:20 ` Robin Murphy
@ 2023-09-14 15:09   ` Jernej Škrabec
  0 siblings, 0 replies; 4+ messages in thread
From: Jernej Škrabec @ 2023-09-14 15:09 UTC (permalink / raw)
  To: joro, will, Robin Murphy
  Cc: wens, samuel, iommu, linux-arm-kernel, linux-sunxi, linux-kernel

Dne sreda, 13. september 2023 ob 22:20:15 CEST je Robin Murphy napisal(a):
> On 2023-09-12 19:19, Jernej Skrabec wrote:
> > Convert driver to use map_pages and unmap_pages. Since functions operate
> > on page table, extend them to be able to operate on whole table, not
> > just one entry.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> > Hi!
> > 
> > I'm not sure if it makes sense to check validity of page table entry when
> > unmaping pages makes sense. What do you think?
> 
> As things currently stand it's largely a matter of opinion - some
> drivers consider that unmapping something which hasn't been mapped is
> nonsensical, and almost always represents the caller having gone wrong,
> thus report it as an error; others take the view that as long as they
> can achieve the requested end result, they're not going to think too
> hard about how they got there. The same arguments apply to whether you'd
> quietly replace an existing PTE or not when mapping, so I'd say the main
> thing is to at least be self-consistent one way or the other.

Ok, I preserved strict checks in this update, which is consistent and same as 
before.

Best regards,
Jernej

> 
> Cheers,
> Robin.
> 
> > Best regards,
> > Jernej
> > 
> >   drivers/iommu/sun50i-iommu.c | 55 +++++++++++++++++++++---------------
> >   1 file changed, 33 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> > index 74c5cb93e900..be6102730a56 100644
> > --- a/drivers/iommu/sun50i-iommu.c
> > +++ b/drivers/iommu/sun50i-iommu.c
> > @@ -589,11 +589,12 @@ static u32 *sun50i_dte_get_page_table(struct
> > sun50i_iommu_domain *sun50i_domain,> 
> >   }
> >   
> >   static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long
> >   iova,> 
> > -			    phys_addr_t paddr, size_t size, int 
prot, gfp_t gfp)
> > +			    phys_addr_t paddr, size_t pgsize, size_t 
pgcount,
> > +			    int prot, gfp_t gfp, size_t *mapped)
> > 
> >   {
> >   
> >   	struct sun50i_iommu_domain *sun50i_domain = 
to_sun50i_domain(domain);
> >   	struct sun50i_iommu *iommu = sun50i_domain->iommu;
> > 
> > -	u32 pte_index;
> > +	u32 pte_index, count, i;
> > 
> >   	u32 *page_table, *pte_addr;
> >   	int ret = 0;
> > 
> > @@ -604,45 +605,55 @@ static int sun50i_iommu_map(struct iommu_domain
> > *domain, unsigned long iova,> 
> >   	}
> >   	
> >   	pte_index = sun50i_iova_get_pte_index(iova);
> > 
> > -	pte_addr = &page_table[pte_index];
> > -	if (unlikely(sun50i_pte_is_page_valid(*pte_addr))) {
> > -		phys_addr_t page_phys = 
sun50i_pte_get_page_address(*pte_addr);
> > -		dev_err(iommu->dev,
> > -			"iova %pad already mapped to %pa cannot 
remap to %pa prot: %#x\n",
> > -			&iova, &page_phys, &paddr, prot);
> > -		ret = -EBUSY;
> > -		goto out;
> > +	count = min(pgcount, (size_t)NUM_PT_ENTRIES - pte_index);
> > +	for (i = 0; i < count; i++) {
> > +		pte_addr = &page_table[pte_index + i];
> > +		if (unlikely(sun50i_pte_is_page_valid(*pte_addr))) {
> > +			phys_addr_t page_phys = 
sun50i_pte_get_page_address(*pte_addr);
> > +
> > +			dev_err(iommu->dev,
> > +				"iova %pad already mapped to %pa 
cannot remap to %pa prot: %#x\n",
> > +				&iova, &page_phys, &paddr, prot);
> > +			ret = -EBUSY;
> > +			goto out;
> > +		}
> > +		*pte_addr = sun50i_mk_pte(paddr, prot);
> > +		paddr += SPAGE_SIZE;
> > 
> >   	}
> > 
> > -	*pte_addr = sun50i_mk_pte(paddr, prot);
> > -	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> > +	sun50i_table_flush(sun50i_domain, &page_table[pte_index], i);
> > +	*mapped = i * SPAGE_SIZE;
> > 
> >   out:
> >   	return ret;
> >   
> >   }
> >   
> >   static size_t sun50i_iommu_unmap(struct iommu_domain *domain, unsigned
> >   long iova,> 
> > -				 size_t size, struct 
iommu_iotlb_gather *gather)
> > +				 size_t pgsize, size_t pgcount,
> > +				 struct iommu_iotlb_gather 
*gather)
> > 
> >   {
> >   
> >   	struct sun50i_iommu_domain *sun50i_domain = 
to_sun50i_domain(domain);
> > 
> > +	u32 dte, count, i, pte_index;
> > 
> >   	phys_addr_t pt_phys;
> >   	u32 *pte_addr;
> > 
> > -	u32 dte;
> > 
> >   	dte = sun50i_domain->dt[sun50i_iova_get_dte_index(iova)];
> >   	if (!sun50i_dte_is_pt_valid(dte))
> >   	
> >   		return 0;
> >   	
> >   	pt_phys = sun50i_dte_get_pt_address(dte);
> > 
> > -	pte_addr = (u32 *)phys_to_virt(pt_phys) +
> > sun50i_iova_get_pte_index(iova); +	pte_index =
> > sun50i_iova_get_pte_index(iova);
> > +	pte_addr = (u32 *)phys_to_virt(pt_phys) + pte_index;
> > 
> > -	if (!sun50i_pte_is_page_valid(*pte_addr))
> > -		return 0;
> > +	count = min(pgcount, (size_t)NUM_PT_ENTRIES - pte_index);
> > +	for (i = 0; i < count; i++)
> > +		if (!sun50i_pte_is_page_valid(pte_addr[i]))
> > +			break;
> > 
> > -	memset(pte_addr, 0, sizeof(*pte_addr));
> > -	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> > +	memset(pte_addr, 0, sizeof(*pte_addr) * i);
> > +	sun50i_table_flush(sun50i_domain, pte_addr, i);
> > 
> > -	return SZ_4K;
> > +	return i * SPAGE_SIZE;
> > 
> >   }
> >   
> >   static phys_addr_t sun50i_iommu_iova_to_phys(struct iommu_domain
> >   *domain,
> > 
> > @@ -838,8 +849,8 @@ static const struct iommu_ops sun50i_iommu_ops = {
> > 
> >   		.iotlb_sync_map = sun50i_iommu_iotlb_sync_map,
> >   		.iotlb_sync	= sun50i_iommu_iotlb_sync,
> >   		.iova_to_phys	= sun50i_iommu_iova_to_phys,
> > 
> > -		.map		= sun50i_iommu_map,
> > -		.unmap		= sun50i_iommu_unmap,
> > +		.map_pages	= sun50i_iommu_map,
> > +		.unmap_pages	= sun50i_iommu_unmap,
> > 
> >   		.free		= sun50i_iommu_domain_free,
> >   	
> >   	}
> >   
> >   };





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iommu/sun50i: Convert to map_pages/unmap_pages
  2023-09-12 18:19 [PATCH] iommu/sun50i: Convert to map_pages/unmap_pages Jernej Skrabec
  2023-09-13 20:20 ` Robin Murphy
@ 2023-09-25 16:19 ` Robin Murphy
  1 sibling, 0 replies; 4+ messages in thread
From: Robin Murphy @ 2023-09-25 16:19 UTC (permalink / raw)
  To: Jernej Skrabec, joro, will
  Cc: wens, samuel, iommu, linux-arm-kernel, linux-sunxi, linux-kernel

On 2023-09-12 19:19, Jernej Skrabec wrote:
> Convert driver to use map_pages and unmap_pages. Since functions operate
> on page table, extend them to be able to operate on whole table, not
> just one entry.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
> Hi!
> 
> I'm not sure if it makes sense to check validity of page table entry when
> unmaping pages makes sense. What do you think?
> 
> Best regards,
> Jernej
> 
>   drivers/iommu/sun50i-iommu.c | 55 +++++++++++++++++++++---------------
>   1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> index 74c5cb93e900..be6102730a56 100644
> --- a/drivers/iommu/sun50i-iommu.c
> +++ b/drivers/iommu/sun50i-iommu.c
> @@ -589,11 +589,12 @@ static u32 *sun50i_dte_get_page_table(struct sun50i_iommu_domain *sun50i_domain,
>   }
>   
>   static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
> -			    phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> +			    phys_addr_t paddr, size_t pgsize, size_t pgcount,
> +			    int prot, gfp_t gfp, size_t *mapped)
>   {
>   	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
>   	struct sun50i_iommu *iommu = sun50i_domain->iommu;
> -	u32 pte_index;
> +	u32 pte_index, count, i;
>   	u32 *page_table, *pte_addr;
>   	int ret = 0;
>   
> @@ -604,45 +605,55 @@ static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
>   	}
>   
>   	pte_index = sun50i_iova_get_pte_index(iova);
> -	pte_addr = &page_table[pte_index];
> -	if (unlikely(sun50i_pte_is_page_valid(*pte_addr))) {
> -		phys_addr_t page_phys = sun50i_pte_get_page_address(*pte_addr);
> -		dev_err(iommu->dev,
> -			"iova %pad already mapped to %pa cannot remap to %pa prot: %#x\n",
> -			&iova, &page_phys, &paddr, prot);
> -		ret = -EBUSY;
> -		goto out;
> +	count = min(pgcount, (size_t)NUM_PT_ENTRIES - pte_index);
> +	for (i = 0; i < count; i++) {
> +		pte_addr = &page_table[pte_index + i];
> +		if (unlikely(sun50i_pte_is_page_valid(*pte_addr))) {
> +			phys_addr_t page_phys = sun50i_pte_get_page_address(*pte_addr);
> +
> +			dev_err(iommu->dev,
> +				"iova %pad already mapped to %pa cannot remap to %pa prot: %#x\n",
> +				&iova, &page_phys, &paddr, prot);
> +			ret = -EBUSY;
> +			goto out;
> +		}
> +		*pte_addr = sun50i_mk_pte(paddr, prot);
> +		paddr += SPAGE_SIZE;
>   	}
>   
> -	*pte_addr = sun50i_mk_pte(paddr, prot);
> -	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> +	sun50i_table_flush(sun50i_domain, &page_table[pte_index], i);

I think you want the "out" label here, so that "mapped" is still updated 
and the core knows how much to undo on failure. I guess one could argue 
that the pagetables are already messed up if that happens, but it still 
doesn't seem like a good reason to deliberately compound the problem by 
leaving behind even more "unexpected" entries.

Thanks,
Robin.

> +	*mapped = i * SPAGE_SIZE;
>   
>   out:
>   	return ret;
>   }
>   
>   static size_t sun50i_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
> -				 size_t size, struct iommu_iotlb_gather *gather)
> +				 size_t pgsize, size_t pgcount,
> +				 struct iommu_iotlb_gather *gather)
>   {
>   	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
> +	u32 dte, count, i, pte_index;
>   	phys_addr_t pt_phys;
>   	u32 *pte_addr;
> -	u32 dte;
>   
>   	dte = sun50i_domain->dt[sun50i_iova_get_dte_index(iova)];
>   	if (!sun50i_dte_is_pt_valid(dte))
>   		return 0;
>   
>   	pt_phys = sun50i_dte_get_pt_address(dte);
> -	pte_addr = (u32 *)phys_to_virt(pt_phys) + sun50i_iova_get_pte_index(iova);
> +	pte_index = sun50i_iova_get_pte_index(iova);
> +	pte_addr = (u32 *)phys_to_virt(pt_phys) + pte_index;
>   
> -	if (!sun50i_pte_is_page_valid(*pte_addr))
> -		return 0;
> +	count = min(pgcount, (size_t)NUM_PT_ENTRIES - pte_index);
> +	for (i = 0; i < count; i++)
> +		if (!sun50i_pte_is_page_valid(pte_addr[i]))
> +			break;
>   
> -	memset(pte_addr, 0, sizeof(*pte_addr));
> -	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> +	memset(pte_addr, 0, sizeof(*pte_addr) * i);
> +	sun50i_table_flush(sun50i_domain, pte_addr, i);
>   
> -	return SZ_4K;
> +	return i * SPAGE_SIZE;
>   }
>   
>   static phys_addr_t sun50i_iommu_iova_to_phys(struct iommu_domain *domain,
> @@ -838,8 +849,8 @@ static const struct iommu_ops sun50i_iommu_ops = {
>   		.iotlb_sync_map = sun50i_iommu_iotlb_sync_map,
>   		.iotlb_sync	= sun50i_iommu_iotlb_sync,
>   		.iova_to_phys	= sun50i_iommu_iova_to_phys,
> -		.map		= sun50i_iommu_map,
> -		.unmap		= sun50i_iommu_unmap,
> +		.map_pages	= sun50i_iommu_map,
> +		.unmap_pages	= sun50i_iommu_unmap,
>   		.free		= sun50i_iommu_domain_free,
>   	}
>   };

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-09-25 16:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-12 18:19 [PATCH] iommu/sun50i: Convert to map_pages/unmap_pages Jernej Skrabec
2023-09-13 20:20 ` Robin Murphy
2023-09-14 15:09   ` Jernej Škrabec
2023-09-25 16:19 ` Robin Murphy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).