Add iommu map_sg/unmap_sg API

Mapping and unmapping are more often than not in the critical path. map_sg and unmap_sg allows IOMMU driver implementations to optimize the process of mapping and unmapping buffers into the IOMMU page tables.

Instead of mapping a buffer one page at a time and requiring potentially expensive TLB operations for each page, this function allows the driver to map all pages in one go and defer TLB maintenance until after all pages have been mapped.

Additionally, the mapping operation would be faster in general since clients does not have to keep calling map API over and over again for each physically contiguous chunk of memory that needs to be mapped to a virtually contiguous region.

Signed-off-by Olav Haugan <[email protected]>

drivers/iommu/iommu.c +7 -0

+			break;
+		}
+		offset += page_len;
+	}
+
+	return ret;
+}

On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
.. snip..

drivers/iommu/iommu.c size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) · line 1108

Quoted 9 lines

drivers/iommu/iommu.c +9 -0

+	for_each_sg(sg, s, nents, i) {
+		phys_addr_t phys = page_to_phys(sg_page(s));
+		size_t page_len = s->offset + s->length;
+
+		ret = iommu_map(domain, iova + offset, phys, page_len,
+				prot);
+		if (ret) {
+			/* undo mappings already done */
+			iommu_unmap(domain, iova, offset);

Don't we want then to unmap all of the scatter list instead of just the last one?

include/linux/iommu.h +4 -0

@@ -93,6 +94,10 @@ enum iommu_attr {
  * @detach_dev: detach device from an iommu domain
  * @map: map a physically contiguous memory region to an iommu domain
  * @unmap: unmap a physically contiguous memory region from an iommu domain
+ * @map_sg: map a scatter-gather list of physically contiguous memory chunks
+ * to an iommu domain
+ * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
+ * chunks from an iommu domain
  * @iova_to_phys: translate iova to physical address
  * @domain_has_cap: domain capabilities query
  * @add_device: add device to iommu grouping

Hi Olav,

Quoted 12 lines · Olav Haugan

include/linux/iommu.h +4 -0

@@ -93,6 +94,10 @@ enum iommu_attr {
  * @detach_dev: detach device from an iommu domain
  * @map: map a physically contiguous memory region to an iommu domain
  * @unmap: unmap a physically contiguous memory region from an iommu domain
+ * @map_sg: map a scatter-gather list of physically contiguous memory chunks
+ * to an iommu domain
+ * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
+ * chunks from an iommu domain
  * @iova_to_phys: translate iova to physical address
  * @domain_has_cap: domain capabilities query
  * @add_device: add device to iommu grouping

include/linux/iommu.h struct iommu_ops { · line 122

Quoted 8 lines

include/linux/iommu.h +5 -0

@@ -110,6 +115,11 @@ struct iommu_ops {
                   phys_addr_t paddr, size_t size, int prot);
        size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
                     size_t size);
+       int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
+                       struct scatterlist *sg, unsigned int nents, int prot,
+                       unsigned long flags);
+       int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
+                       size_t size, unsigned long flags);

Do you have any exmaple/explanation for the above "flags"?

Is this going to be used for iommu global/standard attribute or SoC spcific one?

Quoted 27 lines · Hiroshi Doyu

Hi Olav,

Quoted 12 lines · Olav Haugan

include/linux/iommu.h +4 -0

@@ -93,6 +94,10 @@ enum iommu_attr {
  * @detach_dev: detach device from an iommu domain
  * @map: map a physically contiguous memory region to an iommu domain
  * @unmap: unmap a physically contiguous memory region from an iommu domain
+ * @map_sg: map a scatter-gather list of physically contiguous memory chunks
+ * to an iommu domain
+ * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
+ * chunks from an iommu domain
  * @iova_to_phys: translate iova to physical address
  * @domain_has_cap: domain capabilities query
  * @add_device: add device to iommu grouping

include/linux/iommu.h struct iommu_ops { · line 122

Quoted 8 lines

include/linux/iommu.h +5 -0

@@ -110,6 +115,11 @@ struct iommu_ops {
                   phys_addr_t paddr, size_t size, int prot);
        size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
                     size_t size);
+       int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
+                       struct scatterlist *sg, unsigned int nents, int prot,
+                       unsigned long flags);
+       int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
+                       size_t size, unsigned long flags);

Do you have any exmaple/explanation for the above "flags"?

Quoted 2 lines

Is this going to be used for iommu global/standard attribute or SoC spcific one?

iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for drivers which wanted to map/unmap N buffers with a single flush at the end. There might have been some other usages envisioned.

Quoted 3 lines · Rob Clark

iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for drivers which wanted to map/unmap N buffers with a single flush at the end. There might have been some other usages envisioned.

Yes, that was the original intent of the flags for now. I am sure we can find other uses for this in the future.

Quoted 5 lines · Olav Haugan

Quoted 3 lines · Rob Clark

iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for drivers which wanted to map/unmap N buffers with a single flush at the end. There might have been some other usages envisioned.

Quoted 2 lines

Yes, that was the original intent of the flags for now. I am sure we can find other uses for this in the future.

Do you have anything else in mind already besides the DONT_FLUSH_TLB flag?

How is the IOTLB supposed to be flushed when this flag is used?

Joerg

Quoted 6 lines · On Mon, Aug 18, 2014 at 10:07 AM, joro at 8bytes.org <[email protected]> wrote:

Quoted 5 lines · Olav Haugan

Quoted 3 lines · Rob Clark

iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for drivers which wanted to map/unmap N buffers with a single flush at the end. There might have been some other usages envisioned.

Quoted 5 lines

Quoted 2 lines

Yes, that was the original intent of the flags for now. I am sure we can find other uses for this in the future.

Do you have anything else in mind already besides the DONT_FLUSH_TLB flag?

Quoted 2 lines

How is the IOTLB supposed to be flushed when this flag is used?

well, I was thinking one of two ways:

1) add new flush() vfunc.. this, I think, would be most convenient for drivers using this feature
2) or driver simply doesn't set DONT_FLUSH_TLB flag on the last {map,unmap}.. that would be slightly more awkward to use, but would avoid adding a new vfunc

On 8/18/2014 11:32 AM, Rob Clark wrote:

No, I do not have other uses right now. But could imagine use cases like "force <insert minimum mapping size here> mapping" flag etc.

Quoted 4 lines · Olav Haugan

On 8/18/2014 11:32 AM, Rob Clark wrote:

No, I do not have other uses right now. But could imagine use cases like "force <insert minimum mapping size here> mapping" flag etc.

I think it is worth discussing to add a flush() function to the IOMMU-API. I sent a patch-set myself a few years ago introducing an iommu_commit() function with the same semantics, but this is out-of-scope for this patch-set.

Also adding a flag parameter to iommu_map_sg would introduce an asymentrie in the API to the iommu_map() function. So please leave it out for this patch-set, when we really need a flag parameter someday we can introduce it for iommu_map() and iommu_map_sg() together.

Mapping and unmapping are more often than not in the critical path. map_sg and unmap_sg allows IOMMU driver implementations to optimize the process of mapping and unmapping buffers into the IOMMU page tables.

Instead of mapping a buffer one page at a time and requiring potentially expensive TLB operations for each page, this function allows the driver to map all pages in one go and defer TLB maintenance until after all pages have been mapped.

Additionally, the mapping operation would be faster in general since clients does not have to keep calling map API over and over again for each physically contiguous chunk of memory that needs to be mapped to a virtually contiguous region.

Signed-off-by Olav Haugan <[email protected]>

---
 drivers/iommu/amd_iommu.c      |  2 ++
 drivers/iommu/arm-smmu.c       |  2 ++
 drivers/iommu/exynos-iommu.c   |  2 ++
 drivers/iommu/intel-iommu.c    |  2 ++
 drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c     |  2 ++
 drivers/iommu/msm_iommu.c      |  2 ++
 drivers/iommu/omap-iommu.c     |  2 ++
 drivers/iommu/shmobile-iommu.c |  2 ++
 drivers/iommu/tegra-smmu.c     |  2 ++
 include/linux/iommu.h          | 44 +++++++++++++++++++++++++++++++++++++++
 11 files changed, 95 insertions(+)

drivers/iommu/iommu.c +0 -0

index 1698360..24cf727 100644
@@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain,
 unsigned long iova, size_t size)

include/linux/iommu.h +0 -0

index 20f9a52..ee106ce 100644

include/linux/iommu.h +9 -0

+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,
+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return domain->ops->unmap_sg(domain, iova, size, flags);
+}
+

Hi Olav,

Thank you for the patch.

Quoted 28 lines · On Monday 11 August 2014 15:45:50 Olav Haugan wrote:

Mapping and unmapping are more often than not in the critical path. map_sg and unmap_sg allows IOMMU driver implementations to optimize the process of mapping and unmapping buffers into the IOMMU page tables.

Instead of mapping a buffer one page at a time and requiring potentially expensive TLB operations for each page, this function allows the driver to map all pages in one go and defer TLB maintenance until after all pages have been mapped.

Additionally, the mapping operation would be faster in general since clients does not have to keep calling map API over and over again for each physically contiguous chunk of memory that needs to be mapped to a virtually contiguous region.

Signed-off-by Olav Haugan <[email protected]>

---
 drivers/iommu/amd_iommu.c      |  2 ++
 drivers/iommu/arm-smmu.c       |  2 ++
 drivers/iommu/exynos-iommu.c   |  2 ++
 drivers/iommu/intel-iommu.c    |  2 ++
 drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c     |  2 ++
 drivers/iommu/msm_iommu.c      |  2 ++
 drivers/iommu/omap-iommu.c     |  2 ++
 drivers/iommu/shmobile-iommu.c |  2 ++
 drivers/iommu/tegra-smmu.c     |  2 ++
 include/linux/iommu.h          | 44 +++++++++++++++++++++++++++++++++++++++
 11 files changed, 95 insertions(+)

[snip]

Quoted 6 lines

drivers/iommu/iommu.c +0 -0

index 1698360..24cf727 100644
@@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain,
 unsigned long iova, size_t size)

[snip]

drivers/iommu/iommu.c size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) · line 1119

Quoted 2 lines

drivers/iommu/iommu.c +2 -0

+int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
+			       size_t size, unsigned long flags)

I would have called this iommu_default_unmap_sg (and same comment for default_iommu_map_sg) to keep the iommu_ prefix, but that's up to you.

drivers/iommu/iommu.c size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) · line 1123

Quoted 4 lines

drivers/iommu/iommu.c +4 -0

+{
+	return iommu_unmap(domain, iova, size);
+}
+EXPORT_SYMBOL_GPL(default_iommu_unmap_sg);

Do you expect drivers to need to override this ? What are the use cases for non-default implementation of unmap_sg different than this ?

[snip]

Quoted 4 lines

include/linux/iommu.h +0 -0

index 20f9a52..ee106ce 100644

[snip]

include/linux/iommu.h static inline int report_iommu_fault(struct · line 264

Quoted 10 lines

include/linux/iommu.h +5 -0

@@ -240,6 +256,20 @@ static inline int report_iommu_fault(struct
 iommu_domain *domain, return ret;
 }
 
+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long
 iova,
+			       struct scatterlist *sg, unsigned int nents,
+			       int prot, unsigned long flags)
+{
+	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);

Instead of having to modify all IOMMU drivers to set the map_sg operation to default_iommu_map_sg, how about calling it automatically as a fallback when map_sg is NULL ? Something like

	if (domain->ops->map_sg)
		return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);

	return default_iommu_map_sg(domain, iova, sg, nents, prot, flags);

include/linux/iommu.h static inline int report_iommu_fault(struct iommu_domain *domain, · line 272

Quoted 10 lines

include/linux/iommu.h +9 -0

+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,
+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return domain->ops->unmap_sg(domain, iova, size, flags);
+}
+

drivers/iommu/iommu.c +5 -0

+int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			 struct scatterlist *sg, unsigned int nents,
+			 int prot, unsigned long flags)
+{
+	int ret = 0;

Quoted 11 lines · Olav Haugan

drivers/iommu/iommu.c +11 -0

+int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			 struct scatterlist *sg, unsigned int nents,
+			 int prot, unsigned long flags)
+{
+	int ret = 0;
+	unsigned long offset = 0;
+	unsigned int i;
+	struct scatterlist *s;
+
+	for_each_sg(sg, s, nents, i) {
+		phys_addr_t phys = page_to_phys(sg_page(s));

drivers/iommu/iommu.c size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) · line 1105

Quoted 4 lines

drivers/iommu/iommu.c +4 -0

+		size_t page_len = s->offset + s->length;
+
+		ret = iommu_map(domain, iova + offset, phys, page_len,
+				prot);

This isn't going to work, iova + offset, phys and page_len need to be aligned to the minimum page-size the given IOMMU implementation supports. See the iommu_map implementation for details.

drivers/iommu/iommu.c size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) · line 1119

Quoted 2 lines

drivers/iommu/iommu.c +2 -0

+int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
+			       size_t size, unsigned long flags)

Another asymmentry here, why don't you just pass a scatterlist and nents like in the map_sg function? if you implement it like this it is just a duplication of iommu_unmap().

Quoted 8 lines

include/linux/iommu.h +8 -0

+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			       struct scatterlist *sg, unsigned int nents,
+			       int prot, unsigned long flags)
+{
+	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,

include/linux/iommu.h static inline int report_iommu_fault(struct iommu_domain *domain, · line 271

Quoted 5 lines

include/linux/iommu.h +5 -0

+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return domain->ops->unmap_sg(domain, iova, size, flags);
+}

These function pointers need to be checked for != NULL before calling them.

If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap.

Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions).

Quoted 6 lines · Olav Haugan

If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap.

But sg->offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s->offset + s->length' you use it the same way).
So when you pass iova + offset the result will no longer be page-aligned. You should force sg->offset == 0 and sg->length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API.

Quoted 7 lines

Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too.

Quoted 4 lines

I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions).

Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here.

Quoted 25 lines · On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:

Quoted 6 lines · Olav Haugan

If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap.

But sg->offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s->offset + s->length' you use it the same way).
So when you pass iova + offset the result will no longer be page-aligned. You should force sg->offset == 0 and sg->length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API.

Quoted 7 lines

Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

Quoted 3 lines

Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too.

Do we have a use case where the unmap_sg() implementation would be different than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() completely.

Quoted 7 lines

Quoted 4 lines

I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions).

Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here.

I suggested doing a

if (ops is not NULL)

	return ops();
else
	return default_ops();

to avoid modifying all drivers. I'm not sure why that wasn't received with much enthusiasm.

Quoted 51 lines · Laurent Pinchart

Quoted 26 lines · On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:

Quoted 6 lines · Olav Haugan

If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap.

But sg->offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s->offset + s->length' you use it the same way).
So when you pass iova + offset the result will no longer be page-aligned. You should force sg->offset == 0 and sg->length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API.

Quoted 8 lines

Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

Quoted 3 lines

Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too.

Do we have a use case where the unmap_sg() implementation would be different than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() completely.

Quoted 7 lines

Quoted 4 lines

I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions).

Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here.

I suggested doing a

if (ops is not NULL)

	return ops();

else

Quoted 4 lines

	return default_ops();

to avoid modifying all drivers. I'm not sure why that wasn't received with much enthusiasm.

Both Thierry R. and Konrad W. argued for modifying the drivers instead so I implemented what the majority wanted. :-)

Quoted 54 lines · On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:

Quoted 53 lines · Laurent Pinchart

Quoted 26 lines · On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:

Quoted 6 lines · Olav Haugan

If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap.

But sg->offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s->offset + s->length' you use it the same way).
So when you pass iova + offset the result will no longer be page-aligned. You should force sg->offset == 0 and sg->length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API.

Quoted 8 lines

Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

Quoted 3 lines

Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too.

Do we have a use case where the unmap_sg() implementation would be different than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() completely.

Quoted 7 lines

Quoted 4 lines

I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions).

Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here.

I suggested doing a

if (ops is not NULL)

	return ops();

else

	return default_ops();

Quoted 5 lines

Quoted 2 lines

to avoid modifying all drivers. I'm not sure why that wasn't received with much enthusiasm.

Both Thierry R. and Konrad W. argued for modifying the drivers instead so I implemented what the majority wanted. :-)

I'm not blaming you :-) I was just wondering what their rationale was.

Quoted 54 lines · On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:

Quoted 53 lines · Laurent Pinchart

Quoted 26 lines · On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:

Quoted 6 lines · Olav Haugan

If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap.

But sg->offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s->offset + s->length' you use it the same way).
So when you pass iova + offset the result will no longer be page-aligned. You should force sg->offset == 0 and sg->length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API.

Quoted 8 lines

Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

Quoted 3 lines

Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too.

Do we have a use case where the unmap_sg() implementation would be different than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() completely.

Quoted 7 lines

Quoted 4 lines

I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions).

Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here.

I suggested doing a

if (ops is not NULL)

	return ops();

else

	return default_ops();

Quoted 5 lines

Quoted 2 lines

to avoid modifying all drivers. I'm not sure why that wasn't received with much enthusiasm.

Both Thierry R. and Konrad W. argued for modifying the drivers instead so I implemented what the majority wanted. :-)

I'm not blaming you :-) I was just wondering what their rationale was.

Quoted 3 lines · Laurent Pinchart

Quoted 2 lines · On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:

Quoted 1 line · Laurent Pinchart

On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:

Quoted 16 lines

Quoted 15 lines

Quoted 5 lines

Quoted 2 lines

Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here.

I suggested doing a

Quoted 7 lines

if (ops is not NULL)

	return ops();
else
	return default_ops();

to avoid modifying all drivers. I'm not sure why that wasn't received with much enthusiasm.

Both Thierry R. and Konrad W. argued for modifying the drivers instead so I implemented what the majority wanted. :-)

Quoted 1 line

I'm not blaming you :-) I was just wondering what their rationale was.

a). To be inline with the usage of this API.

For this particular case the other functions (but maybe I missed some) follow the idea that they implement the ops->func for every operation and they don't have an fallback.

b) Follow what x86 maintainers prefer (based on other API calls, such as x86_init or any other platform overrides).

c). When there are new IOMMUs it has to take in-to account all of the
function ops. If they fail to implement all of them the kernel crashes instead of working (but maybe working incorrectly).

d). Lastly, we also want the bloat of kernel. If the compiler decides
to roll in the fallback implementation for 'iommu_map_sg' in - it will needlessly expand the kernel wherever we use 'iommu_map_sg' call with a fallback implementation (which might not be called 99% of time).

Having the little inline static just do 'func_ops->func' will stop the compiler from optimizing too much and convert this just to a simple function.

Quoted 6 lines · Olav Haugan

If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap.

But sg->offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s->offset + s->length' you use it the same way).
So when you pass iova + offset the result will no longer be page-aligned. You should force sg->offset == 0 and sg->length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API.

Quoted 7 lines

Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too.

Quoted 4 lines

I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions).

Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here.

Joerg

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

include/linux/iommu.h static inline int report_iommu_fault(struct iommu_domain *domain, · line 271

Quoted 13 lines · Olav Haugan

include/linux/iommu.h +13 -0

+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			       struct scatterlist *sg, unsigned int nents,
+			       int prot, unsigned long flags)
+{
+	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,
+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return domain->ops->unmap_sg(domain, iova, size, flags);
+}

I have thought a little bit more about this interface and think that it would be better to just return a size_t from iommu_map_sg(). The function returns the amount of address space mapped by it, 0 in the worst case.

This makes it easy to unmap the region just with iommu_unmap(domain, iova, size) in the end and removing the need for a new iommu_unmap_sg() function. Also the error-path of the map_sg call-backs becomes easier as the function then just returns the amount of address-space already mapped before the error happened.

So the prototype would be:

size_t iommu_map_sg(struct iommu_domain *domain,
unsigned long iova, struct scatterlist *sg, unsigned int nents, int prot);

(as I said before, the flags parameter should not be added by this
patch-set).

Quoted 34 lines · Joerg Roedel

include/linux/iommu.h static inline int report_iommu_fault(struct iommu_domain *domain, · line 271

Quoted 13 lines · Olav Haugan

include/linux/iommu.h +13 -0

+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			       struct scatterlist *sg, unsigned int nents,
+			       int prot, unsigned long flags)
+{
+	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,
+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return domain->ops->unmap_sg(domain, iova, size, flags);
+}

I have thought a little bit more about this interface and think that it would be better to just return a size_t from iommu_map_sg(). The function returns the amount of address space mapped by it, 0 in the worst case.

This makes it easy to unmap the region just with iommu_unmap(domain, iova, size) in the end and removing the need for a new iommu_unmap_sg() function. Also the error-path of the map_sg call-backs becomes easier as the function then just returns the amount of address-space already mapped before the error happened.

So the prototype would be:

size_t iommu_map_sg(struct iommu_domain *domain,
unsigned long iova, struct scatterlist *sg, unsigned int nents, int prot);

Quoted 3 lines

(as I said before, the flags parameter should not be added by this
patch-set).

Ok, sounds good. I'll post v6 soon.

Quoted 35 lines · Olav Haugan

Quoted 34 lines · Joerg Roedel

include/linux/iommu.h static inline int report_iommu_fault(struct iommu_domain *domain, · line 271

Quoted 13 lines · Olav Haugan

include/linux/iommu.h +13 -0

+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			       struct scatterlist *sg, unsigned int nents,
+			       int prot, unsigned long flags)
+{
+	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,
+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return domain->ops->unmap_sg(domain, iova, size, flags);
+}

I have thought a little bit more about this interface and think that it would be better to just return a size_t from iommu_map_sg(). The function returns the amount of address space mapped by it, 0 in the worst case.

This makes it easy to unmap the region just with iommu_unmap(domain, iova, size) in the end and removing the need for a new iommu_unmap_sg() function. Also the error-path of the map_sg call-backs becomes easier as the function then just returns the amount of address-space already mapped before the error happened.

So the prototype would be:

size_t iommu_map_sg(struct iommu_domain *domain,
unsigned long iova, struct scatterlist *sg, unsigned int nents, int prot);

Quoted 5 lines

Quoted 3 lines

(as I said before, the flags parameter should not be added by this
patch-set).

Ok, sounds good. I'll post v6 soon.

Perhaps make the return value ssize_t so that we can propagate errors?

Quoted 2 lines · Thierry Reding

Perhaps make the return value ssize_t so that we can propagate errors?

I am fine with that. Joerg?