From: Robin Murphy <[email protected]>
The Mediatek driver is not the only one which might want a basic
address-based gathering behaviour, so although it's arguably simple
enough to open-code, let's factor it out for the sake of cleanliness.
Let's also take this opportunity to document the intent of these
helpers for clarity.
Cc: Joerg Roedel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Jiajun Cao <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Lu Baolu <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Robin Murphy <[email protected]>
---
Changes from Robin's version:
* Added iommu_iotlb_gather_add_range() stub !CONFIG_IOMMU_API
* Use iommu_iotlb_gather_add_range() in iommu_iotlb_gather_add_page()
---
drivers/iommu/mtk_iommu.c | 5 +----
include/linux/iommu.h | 43 ++++++++++++++++++++++++++++++++++-----
2 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index e06b8a0e2b56..0af4a91ac7da 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -523,10 +523,7 @@ static size_t mtk_iommu_unmap(struct iommu_domain *domain,
struct mtk_iommu_domain *dom = to_mtk_domain(domain);
unsigned long end = iova + size - 1;
- if (gather->start > iova)
- gather->start = iova;
- if (gather->end < end)
- gather->end = end;
+ iommu_iotlb_gather_add_range(gather, iova, size);
return dom->iop->unmap(dom->iop, iova, size, gather);
}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..f254c62f3720 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -497,6 +497,38 @@ static inline void iommu_iotlb_sync(struct iommu_domain *domain,
iommu_iotlb_gather_init(iotlb_gather);
}
+/**
+ * iommu_iotlb_gather_add_range - Gather for address-based TLB invalidation
+ * @gather: TLB gather data
+ * @iova: start of page to invalidate
+ * @size: size of page to invalidate
+ *
+ * Helper for IOMMU drivers to build arbitrarily-sized invalidation commands
+ * where only the address range matters, and simply minimising intermediate
+ * syncs is preferred.
+ */
+static inline void iommu_iotlb_gather_add_range(struct iommu_iotlb_gather *gather,
+ unsigned long iova, size_t size)
+{
+ unsigned long end = iova + size - 1;
+
+ if (gather->start > iova)
+ gather->start = iova;
+ if (gather->end < end)
+ gather->end = end;
+}
+
+/**
+ * iommu_iotlb_gather_add_page - Gather for page-based TLB invalidation
+ * @domain: IOMMU domain to be invalidated
+ * @gather: TLB gather data
+ * @iova: start of page to invalidate
+ * @size: size of page to invalidate
+ *
+ * Helper for IOMMU drivers to build invalidation commands based on individual
+ * pages, or with page size/table level hints which cannot be gathered if they
+ * differ.
+ */
static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
struct iommu_iotlb_gather *gather,
unsigned long iova, size_t size)
@@ -515,11 +547,7 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
gather->pgsize = size;
}
- if (gather->end < end)
- gather->end = end;
-
- if (gather->start > start)
- gather->start = start;
+ iommu_iotlb_gather_add_range(gather, iova, size);
}
/* PCI device grouping function */
@@ -702,6 +730,11 @@ static inline void iommu_iotlb_sync(struct iommu_domain *domain,
{
}
+static inline void iommu_iotlb_gather_add_range(struct iommu_iotlb_gather *gather,
+ unsigned long iova, size_t size)
+{
+}
+
static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
{
return 0;
--
2.25.1
On Mon, Jun 07, 2021 at 11:25:38AM -0700, Nadav Amit wrote:
> From: Robin Murphy <[email protected]>
>
> The Mediatek driver is not the only one which might want a basic
> address-based gathering behaviour, so although it's arguably simple
> enough to open-code, let's factor it out for the sake of cleanliness.
> Let's also take this opportunity to document the intent of these
> helpers for clarity.
>
> Cc: Joerg Roedel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Jiajun Cao <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Lu Baolu <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Robin Murphy <[email protected]>
>
> ---
>
> Changes from Robin's version:
> * Added iommu_iotlb_gather_add_range() stub !CONFIG_IOMMU_API
> * Use iommu_iotlb_gather_add_range() in iommu_iotlb_gather_add_page()
> ---
> drivers/iommu/mtk_iommu.c | 5 +----
> include/linux/iommu.h | 43 ++++++++++++++++++++++++++++++++++-----
> 2 files changed, 39 insertions(+), 9 deletions(-)
Acked-by: Will Deacon <[email protected]>
Will
On 2021-06-07 19:25, Nadav Amit wrote:
> From: Robin Murphy <[email protected]>
>
> The Mediatek driver is not the only one which might want a basic
> address-based gathering behaviour, so although it's arguably simple
> enough to open-code, let's factor it out for the sake of cleanliness.
> Let's also take this opportunity to document the intent of these
> helpers for clarity.
>
> Cc: Joerg Roedel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Jiajun Cao <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Lu Baolu <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Robin Murphy <[email protected]>
Nit: missing your signoff.
> ---
>
> Changes from Robin's version:
> * Added iommu_iotlb_gather_add_range() stub !CONFIG_IOMMU_API
Out of curiosity, is there any config in which a stub is actually
needed? Unlike iommu_iotlb_gather_init(), I would have thought that
these helpers should only ever be called by driver code which already
depends on IOMMU_API.
Thanks,
Robin.
> * Use iommu_iotlb_gather_add_range() in iommu_iotlb_gather_add_page()
> ---
> drivers/iommu/mtk_iommu.c | 5 +----
> include/linux/iommu.h | 43 ++++++++++++++++++++++++++++++++++-----
> 2 files changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index e06b8a0e2b56..0af4a91ac7da 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -523,10 +523,7 @@ static size_t mtk_iommu_unmap(struct iommu_domain *domain,
> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
> unsigned long end = iova + size - 1;
>
> - if (gather->start > iova)
> - gather->start = iova;
> - if (gather->end < end)
> - gather->end = end;
> + iommu_iotlb_gather_add_range(gather, iova, size);
> return dom->iop->unmap(dom->iop, iova, size, gather);
> }
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 32d448050bf7..f254c62f3720 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -497,6 +497,38 @@ static inline void iommu_iotlb_sync(struct iommu_domain *domain,
> iommu_iotlb_gather_init(iotlb_gather);
> }
>
> +/**
> + * iommu_iotlb_gather_add_range - Gather for address-based TLB invalidation
> + * @gather: TLB gather data
> + * @iova: start of page to invalidate
> + * @size: size of page to invalidate
> + *
> + * Helper for IOMMU drivers to build arbitrarily-sized invalidation commands
> + * where only the address range matters, and simply minimising intermediate
> + * syncs is preferred.
> + */
> +static inline void iommu_iotlb_gather_add_range(struct iommu_iotlb_gather *gather,
> + unsigned long iova, size_t size)
> +{
> + unsigned long end = iova + size - 1;
> +
> + if (gather->start > iova)
> + gather->start = iova;
> + if (gather->end < end)
> + gather->end = end;
> +}
> +
> +/**
> + * iommu_iotlb_gather_add_page - Gather for page-based TLB invalidation
> + * @domain: IOMMU domain to be invalidated
> + * @gather: TLB gather data
> + * @iova: start of page to invalidate
> + * @size: size of page to invalidate
> + *
> + * Helper for IOMMU drivers to build invalidation commands based on individual
> + * pages, or with page size/table level hints which cannot be gathered if they
> + * differ.
> + */
> static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
> struct iommu_iotlb_gather *gather,
> unsigned long iova, size_t size)
> @@ -515,11 +547,7 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
> gather->pgsize = size;
> }
>
> - if (gather->end < end)
> - gather->end = end;
> -
> - if (gather->start > start)
> - gather->start = start;
> + iommu_iotlb_gather_add_range(gather, iova, size);
> }
>
> /* PCI device grouping function */
> @@ -702,6 +730,11 @@ static inline void iommu_iotlb_sync(struct iommu_domain *domain,
> {
> }
>
> +static inline void iommu_iotlb_gather_add_range(struct iommu_iotlb_gather *gather,
> + unsigned long iova, size_t size)
> +{
> +}
> +
> static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
> {
> return 0;
>
On Mon, 2021-06-07 at 11:25 -0700, Nadav Amit wrote:
> From: Robin Murphy <[email protected]>
>
> The Mediatek driver is not the only one which might want a basic
> address-based gathering behaviour, so although it's arguably simple
> enough to open-code, let's factor it out for the sake of cleanliness.
> Let's also take this opportunity to document the intent of these
> helpers for clarity.
>
> Cc: Joerg Roedel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Jiajun Cao <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Lu Baolu <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Robin Murphy <[email protected]>
>
> ---
>
> Changes from Robin's version:
> * Added iommu_iotlb_gather_add_range() stub !CONFIG_IOMMU_API
> * Use iommu_iotlb_gather_add_range() in iommu_iotlb_gather_add_page()
> ---
> drivers/iommu/mtk_iommu.c | 5 +----
> include/linux/iommu.h | 43 ++++++++++++++++++++++++++++++++++-----
> 2 files changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index e06b8a0e2b56..0af4a91ac7da 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -523,10 +523,7 @@ static size_t mtk_iommu_unmap(struct iommu_domain *domain,
> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
> unsigned long end = iova + size - 1;
Please also help delete this "end".
>
> - if (gather->start > iova)
> - gather->start = iova;
> - if (gather->end < end)
> - gather->end = end;
> + iommu_iotlb_gather_add_range(gather, iova, size);
> return dom->iop->unmap(dom->iop, iova, size, gather);
> }
[snip]
On 2021-06-15 13:29, Yong Wu wrote:
> On Mon, 2021-06-07 at 11:25 -0700, Nadav Amit wrote:
>> From: Robin Murphy <[email protected]>
>>
>> The Mediatek driver is not the only one which might want a basic
>> address-based gathering behaviour, so although it's arguably simple
>> enough to open-code, let's factor it out for the sake of cleanliness.
>> Let's also take this opportunity to document the intent of these
>> helpers for clarity.
>>
>> Cc: Joerg Roedel <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Jiajun Cao <[email protected]>
>> Cc: Robin Murphy <[email protected]>
>> Cc: Lu Baolu <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Robin Murphy <[email protected]>
>>
>> ---
>>
>> Changes from Robin's version:
>> * Added iommu_iotlb_gather_add_range() stub !CONFIG_IOMMU_API
>> * Use iommu_iotlb_gather_add_range() in iommu_iotlb_gather_add_page()
>> ---
>> drivers/iommu/mtk_iommu.c | 5 +----
>> include/linux/iommu.h | 43 ++++++++++++++++++++++++++++++++++-----
>> 2 files changed, 39 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>> index e06b8a0e2b56..0af4a91ac7da 100644
>> --- a/drivers/iommu/mtk_iommu.c
>> +++ b/drivers/iommu/mtk_iommu.c
>> @@ -523,10 +523,7 @@ static size_t mtk_iommu_unmap(struct iommu_domain *domain,
>> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
>> unsigned long end = iova + size - 1;
>
> Please also help delete this "end".
Yes, that was part of my original patch - not sure what happened here :/
Robin.
>>
>> - if (gather->start > iova)
>> - gather->start = iova;
>> - if (gather->end < end)
>> - gather->end = end;
>> + iommu_iotlb_gather_add_range(gather, iova, size);
>> return dom->iop->unmap(dom->iop, iova, size, gather);
>> }
>
> [snip]
>
>
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
> On Jun 15, 2021, at 3:42 AM, Robin Murphy <[email protected]> wrote:
>
> On 2021-06-07 19:25, Nadav Amit wrote:
>> From: Robin Murphy <[email protected]>
>> The Mediatek driver is not the only one which might want a basic
>> address-based gathering behaviour, so although it's arguably simple
>> enough to open-code, let's factor it out for the sake of cleanliness.
>> Let's also take this opportunity to document the intent of these
>> helpers for clarity.
>> Cc: Joerg Roedel <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Jiajun Cao <[email protected]>
>> Cc: Robin Murphy <[email protected]>
>> Cc: Lu Baolu <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Robin Murphy <[email protected]>
>
> Nit: missing your signoff.
>
>> ---
>> Changes from Robin's version:
>> * Added iommu_iotlb_gather_add_range() stub !CONFIG_IOMMU_API
>
> Out of curiosity, is there any config in which a stub is actually needed? Unlike iommu_iotlb_gather_init(), I would have thought that these helpers should only ever be called by driver code which already depends on IOMMU_API.
Indeed, this was only done as a defensive step.
I will remove it. I see no reason for it. Sorry for ruining your patch.
> On Jun 15, 2021, at 12:05 PM, Nadav Amit <[email protected]> wrote:
>
>
>
>> On Jun 15, 2021, at 3:42 AM, Robin Murphy <[email protected]> wrote:
>>
>> On 2021-06-07 19:25, Nadav Amit wrote:
>>> From: Robin Murphy <[email protected]>
>>> The Mediatek driver is not the only one which might want a basic
>>> address-based gathering behaviour, so although it's arguably simple
>>> enough to open-code, let's factor it out for the sake of cleanliness.
>>> Let's also take this opportunity to document the intent of these
>>> helpers for clarity.
>>> Cc: Joerg Roedel <[email protected]>
>>> Cc: Will Deacon <[email protected]>
>>> Cc: Jiajun Cao <[email protected]>
>>> Cc: Robin Murphy <[email protected]>
>>> Cc: Lu Baolu <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Signed-off-by: Robin Murphy <[email protected]>
>>
>> Nit: missing your signoff.
>>
>>> ---
>>> Changes from Robin's version:
>>> * Added iommu_iotlb_gather_add_range() stub !CONFIG_IOMMU_API
>>
>> Out of curiosity, is there any config in which a stub is actually needed? Unlike iommu_iotlb_gather_init(), I would have thought that these helpers should only ever be called by driver code which already depends on IOMMU_API.
>
> Indeed, this was only done as a defensive step.
>
> I will remove it. I see no reason for it. Sorry for ruining your patch.
And remove the stub for iommu_iotlb_gather_is_disjoint() as well.