* [PATCH v4 0/1] Add iommu map_sg/unmap_sg API
@ 2014-08-01 0:54 ` Olav Haugan
0 siblings, 0 replies; 24+ messages in thread
From: Olav Haugan @ 2014-08-01 0:54 UTC (permalink / raw)
To: joro
Cc: robdclark, will.deacon, thierry.reding, iommu, linux-arm-kernel,
linux-arm-msm, mitchelh, Olav Haugan
Patch for adding map_sg/unmap_sg to the generic IOMMU API.
v3 -> v4:
* Removed BUG_ON in both map_sg and unmap_sg
* Removed PAGE_ALIGN of length of mapping
v2 -> v3:
* Updated commit text
* Simplifed fallback code and fixed variable types in map_sg function.
* Renamed and changed arguments to map and unmap functions
v1 -> v2:
* Added support for "option" argument to unmap call. This can be used by
IOMMU driver implentations to allow clients to signal to the driver not
to do TLB invalidate for example.
* Added fallback in case iommu_{map,unmap}_range is called for an IOMMU driver
that does not have these implemented.
* Rebased on top of Joerg's tree.
* Split out patch from the rest of the patches.
Olav Haugan (1):
iommu-api: Add map_sg/unmap_sg functions
drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h | 28 ++++++++++++++++++++++++++++
2 files changed, 72 insertions(+)
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v4 0/1] Add iommu map_sg/unmap_sg API @ 2014-08-01 0:54 ` Olav Haugan 0 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-01 0:54 UTC (permalink / raw) To: linux-arm-kernel Patch for adding map_sg/unmap_sg to the generic IOMMU API. v3 -> v4: * Removed BUG_ON in both map_sg and unmap_sg * Removed PAGE_ALIGN of length of mapping v2 -> v3: * Updated commit text * Simplifed fallback code and fixed variable types in map_sg function. * Renamed and changed arguments to map and unmap functions v1 -> v2: * Added support for "option" argument to unmap call. This can be used by IOMMU driver implentations to allow clients to signal to the driver not to do TLB invalidate for example. * Added fallback in case iommu_{map,unmap}_range is called for an IOMMU driver that does not have these implemented. * Rebased on top of Joerg's tree. * Split out patch from the rest of the patches. Olav Haugan (1): iommu-api: Add map_sg/unmap_sg functions drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-01 0:54 ` Olav Haugan @ 2014-08-01 0:54 ` Olav Haugan -1 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-01 0:54 UTC (permalink / raw) To: joro Cc: robdclark, will.deacon, thierry.reding, iommu, linux-arm-kernel, linux-arm-msm, mitchelh, Olav Haugan 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 <ohaugan@codeaurora.org> --- drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 1698360..1d5dc2e 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) } EXPORT_SYMBOL_GPL(iommu_unmap); +int 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; + + if (unlikely(domain->ops->map_sg == NULL)) { + unsigned int i; + struct scatterlist *s; + + 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) + goto fail; + + offset += page_len; + } + } else { + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); + } + goto out; + +fail: + /* undo mappings already done in case of error */ + iommu_unmap(domain, iova, offset); +out: + return ret; +} +EXPORT_SYMBOL_GPL(iommu_map_sg); + +int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, + size_t size, unsigned long flags) +{ + if (unlikely(domain->ops->unmap_sg == NULL)) + return iommu_unmap(domain, iova, size); + else + return domain->ops->unmap_sg(domain, iova, size, flags); +} +EXPORT_SYMBOL_GPL(iommu_unmap_sg); int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr, phys_addr_t paddr, u64 size, int prot) diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 20f9a52..66ad543 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -22,6 +22,7 @@ #include <linux/errno.h> #include <linux/err.h> #include <linux/types.h> +#include <linux/scatterlist.h> #include <trace/events/iommu.h> #define IOMMU_READ (1 << 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 @@ -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); phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova); int (*domain_has_cap)(struct iommu_domain *domain, unsigned long cap); @@ -153,6 +163,11 @@ extern int iommu_map(struct iommu_domain *domain, unsigned long iova, phys_addr_t paddr, size_t size, int prot); extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size); +extern int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, + struct scatterlist *sg, unsigned int nents, int prot, + unsigned long flags); +extern int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, + size_t size, unsigned long flags); extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova); extern int iommu_domain_has_cap(struct iommu_domain *domain, unsigned long cap); @@ -287,6 +302,19 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova, return -ENODEV; } +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 -ENODEV; +} + +static inline int iommu_unmap_sg(struct iommu_domain *domain, + unsigned long iova, size_t size, unsigned long flags) +{ + return -ENODEV; +} + static inline int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr, phys_addr_t paddr, u64 size, int prot) -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-01 0:54 ` Olav Haugan 0 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-01 0:54 UTC (permalink / raw) To: linux-arm-kernel 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 <ohaugan@codeaurora.org> --- drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 1698360..1d5dc2e 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) } EXPORT_SYMBOL_GPL(iommu_unmap); +int 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; + + if (unlikely(domain->ops->map_sg == NULL)) { + unsigned int i; + struct scatterlist *s; + + 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) + goto fail; + + offset += page_len; + } + } else { + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); + } + goto out; + +fail: + /* undo mappings already done in case of error */ + iommu_unmap(domain, iova, offset); +out: + return ret; +} +EXPORT_SYMBOL_GPL(iommu_map_sg); + +int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, + size_t size, unsigned long flags) +{ + if (unlikely(domain->ops->unmap_sg == NULL)) + return iommu_unmap(domain, iova, size); + else + return domain->ops->unmap_sg(domain, iova, size, flags); +} +EXPORT_SYMBOL_GPL(iommu_unmap_sg); int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr, phys_addr_t paddr, u64 size, int prot) diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 20f9a52..66ad543 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -22,6 +22,7 @@ #include <linux/errno.h> #include <linux/err.h> #include <linux/types.h> +#include <linux/scatterlist.h> #include <trace/events/iommu.h> #define IOMMU_READ (1 << 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 @@ -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); phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova); int (*domain_has_cap)(struct iommu_domain *domain, unsigned long cap); @@ -153,6 +163,11 @@ extern int iommu_map(struct iommu_domain *domain, unsigned long iova, phys_addr_t paddr, size_t size, int prot); extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size); +extern int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, + struct scatterlist *sg, unsigned int nents, int prot, + unsigned long flags); +extern int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, + size_t size, unsigned long flags); extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova); extern int iommu_domain_has_cap(struct iommu_domain *domain, unsigned long cap); @@ -287,6 +302,19 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova, return -ENODEV; } +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 -ENODEV; +} + +static inline int iommu_unmap_sg(struct iommu_domain *domain, + unsigned long iova, size_t size, unsigned long flags) +{ + return -ENODEV; +} + static inline int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr, phys_addr_t paddr, u64 size, int prot) -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 24+ messages in thread
[parent not found: <1406854484-3848-2-git-send-email-ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-01 0:54 ` Olav Haugan @ 2014-08-01 8:22 ` Will Deacon -1 siblings, 0 replies; 24+ messages in thread From: Will Deacon @ 2014-08-01 8:22 UTC (permalink / raw) To: Olav Haugan Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Hi Olav, On Fri, Aug 01, 2014 at 01:54:44AM +0100, 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. Just a couple of minor comments, but I think this is almost there now. > Signed-off-by: Olav Haugan <ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> > --- > drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 1698360..1d5dc2e 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) > } > EXPORT_SYMBOL_GPL(iommu_unmap); > > +int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, > + struct scatterlist *sg, unsigned int nents, > + int prot, unsigned long flags) > +{ What do you anticipate passing in the flags parameter? I assume it's something specific to the scatterlist, since we can't provide this to iommu_map as it stands? > + int ret = 0; > + unsigned long offset = 0; > + > + if (unlikely(domain->ops->map_sg == NULL)) { > + unsigned int i; > + struct scatterlist *s; > + > + 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) > + goto fail; > + > + offset += page_len; > + } > + } else { > + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); > + } > + goto out; > + > +fail: > + /* undo mappings already done in case of error */ > + iommu_unmap(domain, iova, offset); I think this would be cleaner if you stuck it in the loop above and removed all these labels: if (ret) { iommu_unmap(...); break; } Will ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-01 8:22 ` Will Deacon 0 siblings, 0 replies; 24+ messages in thread From: Will Deacon @ 2014-08-01 8:22 UTC (permalink / raw) To: linux-arm-kernel Hi Olav, On Fri, Aug 01, 2014 at 01:54:44AM +0100, 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. Just a couple of minor comments, but I think this is almost there now. > Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> > --- > drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 1698360..1d5dc2e 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) > } > EXPORT_SYMBOL_GPL(iommu_unmap); > > +int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, > + struct scatterlist *sg, unsigned int nents, > + int prot, unsigned long flags) > +{ What do you anticipate passing in the flags parameter? I assume it's something specific to the scatterlist, since we can't provide this to iommu_map as it stands? > + int ret = 0; > + unsigned long offset = 0; > + > + if (unlikely(domain->ops->map_sg == NULL)) { > + unsigned int i; > + struct scatterlist *s; > + > + 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) > + goto fail; > + > + offset += page_len; > + } > + } else { > + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); > + } > + goto out; > + > +fail: > + /* undo mappings already done in case of error */ > + iommu_unmap(domain, iova, offset); I think this would be cleaner if you stuck it in the loop above and removed all these labels: if (ret) { iommu_unmap(...); break; } Will ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20140801082228.GC15733-5wv7dgnIgG8@public.gmane.org>]
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-01 8:22 ` Will Deacon @ 2014-08-01 16:44 ` Olav Haugan -1 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-01 16:44 UTC (permalink / raw) To: Will Deacon Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Hi Will, On 8/1/2014 1:22 AM, Will Deacon wrote: > Hi Olav, > > On Fri, Aug 01, 2014 at 01:54:44AM +0100, 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. > > Just a couple of minor comments, but I think this is almost there now. > >> Signed-off-by: Olav Haugan <ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> >> --- >> drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 72 insertions(+) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 1698360..1d5dc2e 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) >> } >> EXPORT_SYMBOL_GPL(iommu_unmap); >> >> +int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, >> + struct scatterlist *sg, unsigned int nents, >> + int prot, unsigned long flags) >> +{ > > What do you anticipate passing in the flags parameter? I assume it's > something specific to the scatterlist, since we can't provide this to > iommu_map as it stands? Initially the flags argument is planned to be used by clients to indicate to the driver that no TLB operation is necessary. This allows clients to for example map/unmap multiple scatter-gather lists without doing expensive TLB invalidate operations for each call but just do this at the last mapping/unmapping call instead. I believe Rob Clark was looking for this feature and I can see the benefit for our use cases also. >> + int ret = 0; >> + unsigned long offset = 0; >> + >> + if (unlikely(domain->ops->map_sg == NULL)) { >> + unsigned int i; >> + struct scatterlist *s; >> + >> + 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) >> + goto fail; >> + >> + offset += page_len; >> + } >> + } else { >> + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); >> + } >> + goto out; >> + >> +fail: >> + /* undo mappings already done in case of error */ >> + iommu_unmap(domain, iova, offset); > > I think this would be cleaner if you stuck it in the loop above and removed > all these labels: > > if (ret) { > iommu_unmap(...); > break; > } Sure, I can do that. Thanks, Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-01 16:44 ` Olav Haugan 0 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-01 16:44 UTC (permalink / raw) To: linux-arm-kernel Hi Will, On 8/1/2014 1:22 AM, Will Deacon wrote: > Hi Olav, > > On Fri, Aug 01, 2014 at 01:54:44AM +0100, 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. > > Just a couple of minor comments, but I think this is almost there now. > >> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> >> --- >> drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 72 insertions(+) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 1698360..1d5dc2e 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) >> } >> EXPORT_SYMBOL_GPL(iommu_unmap); >> >> +int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, >> + struct scatterlist *sg, unsigned int nents, >> + int prot, unsigned long flags) >> +{ > > What do you anticipate passing in the flags parameter? I assume it's > something specific to the scatterlist, since we can't provide this to > iommu_map as it stands? Initially the flags argument is planned to be used by clients to indicate to the driver that no TLB operation is necessary. This allows clients to for example map/unmap multiple scatter-gather lists without doing expensive TLB invalidate operations for each call but just do this at the last mapping/unmapping call instead. I believe Rob Clark was looking for this feature and I can see the benefit for our use cases also. >> + int ret = 0; >> + unsigned long offset = 0; >> + >> + if (unlikely(domain->ops->map_sg == NULL)) { >> + unsigned int i; >> + struct scatterlist *s; >> + >> + 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) >> + goto fail; >> + >> + offset += page_len; >> + } >> + } else { >> + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); >> + } >> + goto out; >> + >> +fail: >> + /* undo mappings already done in case of error */ >> + iommu_unmap(domain, iova, offset); > > I think this would be cleaner if you stuck it in the loop above and removed > all these labels: > > if (ret) { > iommu_unmap(...); > break; > } Sure, I can do that. Thanks, Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <53DBC3F1.40705-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-01 16:44 ` Olav Haugan @ 2014-08-04 18:03 ` Olav Haugan -1 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-04 18:03 UTC (permalink / raw) To: Will Deacon Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Any more comments on this from anyone before I submit v5? On 8/1/2014 9:44 AM, Olav Haugan wrote: > Hi Will, > > On 8/1/2014 1:22 AM, Will Deacon wrote: >> Hi Olav, >> >> On Fri, Aug 01, 2014 at 01:54:44AM +0100, 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. >> >> Just a couple of minor comments, but I think this is almost there now. >> >>> Signed-off-by: Olav Haugan <ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> >>> --- >>> drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ >>> 2 files changed, 72 insertions(+) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index 1698360..1d5dc2e 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) >>> } >>> EXPORT_SYMBOL_GPL(iommu_unmap); >>> >>> +int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, >>> + struct scatterlist *sg, unsigned int nents, >>> + int prot, unsigned long flags) >>> +{ >> >> What do you anticipate passing in the flags parameter? I assume it's >> something specific to the scatterlist, since we can't provide this to >> iommu_map as it stands? > > Initially the flags argument is planned to be used by clients to > indicate to the driver that no TLB operation is necessary. This allows > clients to for example map/unmap multiple scatter-gather lists without > doing expensive TLB invalidate operations for each call but just do this > at the last mapping/unmapping call instead. I believe Rob Clark was > looking for this feature and I can see the benefit for our use cases also. > >>> + int ret = 0; >>> + unsigned long offset = 0; >>> + >>> + if (unlikely(domain->ops->map_sg == NULL)) { >>> + unsigned int i; >>> + struct scatterlist *s; >>> + >>> + 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) >>> + goto fail; >>> + >>> + offset += page_len; >>> + } >>> + } else { >>> + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); >>> + } >>> + goto out; >>> + >>> +fail: >>> + /* undo mappings already done in case of error */ >>> + iommu_unmap(domain, iova, offset); >> >> I think this would be cleaner if you stuck it in the loop above and removed >> all these labels: >> >> if (ret) { >> iommu_unmap(...); >> break; >> } > > Sure, I can do that. > > Thanks, > > Olav > Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-04 18:03 ` Olav Haugan 0 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-04 18:03 UTC (permalink / raw) To: linux-arm-kernel Any more comments on this from anyone before I submit v5? On 8/1/2014 9:44 AM, Olav Haugan wrote: > Hi Will, > > On 8/1/2014 1:22 AM, Will Deacon wrote: >> Hi Olav, >> >> On Fri, Aug 01, 2014 at 01:54:44AM +0100, 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. >> >> Just a couple of minor comments, but I think this is almost there now. >> >>> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> >>> --- >>> drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ >>> 2 files changed, 72 insertions(+) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index 1698360..1d5dc2e 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) >>> } >>> EXPORT_SYMBOL_GPL(iommu_unmap); >>> >>> +int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, >>> + struct scatterlist *sg, unsigned int nents, >>> + int prot, unsigned long flags) >>> +{ >> >> What do you anticipate passing in the flags parameter? I assume it's >> something specific to the scatterlist, since we can't provide this to >> iommu_map as it stands? > > Initially the flags argument is planned to be used by clients to > indicate to the driver that no TLB operation is necessary. This allows > clients to for example map/unmap multiple scatter-gather lists without > doing expensive TLB invalidate operations for each call but just do this > at the last mapping/unmapping call instead. I believe Rob Clark was > looking for this feature and I can see the benefit for our use cases also. > >>> + int ret = 0; >>> + unsigned long offset = 0; >>> + >>> + if (unlikely(domain->ops->map_sg == NULL)) { >>> + unsigned int i; >>> + struct scatterlist *s; >>> + >>> + 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) >>> + goto fail; >>> + >>> + offset += page_len; >>> + } >>> + } else { >>> + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); >>> + } >>> + goto out; >>> + >>> +fail: >>> + /* undo mappings already done in case of error */ >>> + iommu_unmap(domain, iova, offset); >> >> I think this would be cleaner if you stuck it in the loop above and removed >> all these labels: >> >> if (ret) { >> iommu_unmap(...); >> break; >> } > > Sure, I can do that. > > Thanks, > > Olav > Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-01 0:54 ` Olav Haugan @ 2014-08-05 15:13 ` Konrad Rzeszutek Wilk -1 siblings, 0 replies; 24+ messages in thread From: Konrad Rzeszutek Wilk @ 2014-08-05 15:13 UTC (permalink / raw) To: Olav Haugan Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, will.deacon-5wv7dgnIgG8, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On Thu, Jul 31, 2014 at 05:54:44PM -0700, 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. That is assuming that physical == bus topology. > > Signed-off-by: Olav Haugan <ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> > --- > drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 1698360..1d5dc2e 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) > } > EXPORT_SYMBOL_GPL(iommu_unmap); > > +int 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; > + > + if (unlikely(domain->ops->map_sg == NULL)) { > + unsigned int i; > + struct scatterlist *s; > + > + 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) > + goto fail; > + > + offset += page_len; > + } I think it would be better if you had an 'default_iommu_map_sg' with the implementation above. And then the default ops->map_sg would point to that and each IOMMU would over-write with its own version. That way you don't need any of this 'if' and can have the 'iommu_map_sg' be in the header file (either as static inline or an macro). > + } else { > + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); > + } > + goto out; > + > +fail: > + /* undo mappings already done in case of error */ > + iommu_unmap(domain, iova, offset); > +out: > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_map_sg); > + > +int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, > + size_t size, unsigned long flags) > +{ > + if (unlikely(domain->ops->unmap_sg == NULL)) > + return iommu_unmap(domain, iova, size); > + else > + return domain->ops->unmap_sg(domain, iova, size, flags); > +} > +EXPORT_SYMBOL_GPL(iommu_unmap_sg); > > int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr, > phys_addr_t paddr, u64 size, int prot) > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 20f9a52..66ad543 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -22,6 +22,7 @@ > #include <linux/errno.h> > #include <linux/err.h> > #include <linux/types.h> > +#include <linux/scatterlist.h> > #include <trace/events/iommu.h> > > #define IOMMU_READ (1 << 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 > @@ -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); > phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova); > int (*domain_has_cap)(struct iommu_domain *domain, > unsigned long cap); > @@ -153,6 +163,11 @@ extern int iommu_map(struct iommu_domain *domain, unsigned long iova, > phys_addr_t paddr, size_t size, int prot); > extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, > size_t size); > +extern int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, > + struct scatterlist *sg, unsigned int nents, int prot, > + unsigned long flags); > +extern int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, > + size_t size, unsigned long flags); > extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova); > extern int iommu_domain_has_cap(struct iommu_domain *domain, > unsigned long cap); > @@ -287,6 +302,19 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova, > return -ENODEV; > } > > +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 -ENODEV; > +} > + > +static inline int iommu_unmap_sg(struct iommu_domain *domain, > + unsigned long iova, size_t size, unsigned long flags) > +{ > + return -ENODEV; > +} > + > static inline int iommu_domain_window_enable(struct iommu_domain *domain, > u32 wnd_nr, phys_addr_t paddr, > u64 size, int prot) > -- > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation > > _______________________________________________ > iommu mailing list > iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-05 15:13 ` Konrad Rzeszutek Wilk 0 siblings, 0 replies; 24+ messages in thread From: Konrad Rzeszutek Wilk @ 2014-08-05 15:13 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jul 31, 2014 at 05:54:44PM -0700, 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. That is assuming that physical == bus topology. > > Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> > --- > drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 1698360..1d5dc2e 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) > } > EXPORT_SYMBOL_GPL(iommu_unmap); > > +int 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; > + > + if (unlikely(domain->ops->map_sg == NULL)) { > + unsigned int i; > + struct scatterlist *s; > + > + 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) > + goto fail; > + > + offset += page_len; > + } I think it would be better if you had an 'default_iommu_map_sg' with the implementation above. And then the default ops->map_sg would point to that and each IOMMU would over-write with its own version. That way you don't need any of this 'if' and can have the 'iommu_map_sg' be in the header file (either as static inline or an macro). > + } else { > + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); > + } > + goto out; > + > +fail: > + /* undo mappings already done in case of error */ > + iommu_unmap(domain, iova, offset); > +out: > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_map_sg); > + > +int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, > + size_t size, unsigned long flags) > +{ > + if (unlikely(domain->ops->unmap_sg == NULL)) > + return iommu_unmap(domain, iova, size); > + else > + return domain->ops->unmap_sg(domain, iova, size, flags); > +} > +EXPORT_SYMBOL_GPL(iommu_unmap_sg); > > int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr, > phys_addr_t paddr, u64 size, int prot) > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 20f9a52..66ad543 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -22,6 +22,7 @@ > #include <linux/errno.h> > #include <linux/err.h> > #include <linux/types.h> > +#include <linux/scatterlist.h> > #include <trace/events/iommu.h> > > #define IOMMU_READ (1 << 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 > @@ -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); > phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova); > int (*domain_has_cap)(struct iommu_domain *domain, > unsigned long cap); > @@ -153,6 +163,11 @@ extern int iommu_map(struct iommu_domain *domain, unsigned long iova, > phys_addr_t paddr, size_t size, int prot); > extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, > size_t size); > +extern int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, > + struct scatterlist *sg, unsigned int nents, int prot, > + unsigned long flags); > +extern int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, > + size_t size, unsigned long flags); > extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova); > extern int iommu_domain_has_cap(struct iommu_domain *domain, > unsigned long cap); > @@ -287,6 +302,19 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova, > return -ENODEV; > } > > +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 -ENODEV; > +} > + > +static inline int iommu_unmap_sg(struct iommu_domain *domain, > + unsigned long iova, size_t size, unsigned long flags) > +{ > + return -ENODEV; > +} > + > static inline int iommu_domain_window_enable(struct iommu_domain *domain, > u32 wnd_nr, phys_addr_t paddr, > u64 size, int prot) > -- > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation > > _______________________________________________ > iommu mailing list > iommu at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20140805151323.GB19709-0iZWjJA6G8GSPmnEAIUT9EEOCMrvLtNR@public.gmane.org>]
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-05 15:13 ` Konrad Rzeszutek Wilk @ 2014-08-06 17:08 ` Olav Haugan -1 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-06 17:08 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, will.deacon-5wv7dgnIgG8, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On 8/5/2014 8:13 AM, Konrad Rzeszutek Wilk wrote: > On Thu, Jul 31, 2014 at 05:54:44PM -0700, 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. > > That is assuming that physical == bus topology. > >> >> Signed-off-by: Olav Haugan <ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> >> --- >> drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 72 insertions(+) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 1698360..1d5dc2e 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) >> } >> EXPORT_SYMBOL_GPL(iommu_unmap); >> >> +int 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; >> + >> + if (unlikely(domain->ops->map_sg == NULL)) { >> + unsigned int i; >> + struct scatterlist *s; >> + >> + 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) >> + goto fail; >> + >> + offset += page_len; >> + } > > I think it would be better if you had an 'default_iommu_map_sg' with > the implementation above. And then the default ops->map_sg would point to > that and each IOMMU would over-write with its own version. > > That way you don't need any of this 'if' and can have the 'iommu_map_sg' > be in the header file (either as static inline or an macro). so you are suggesting that I check in "bus_set_iommu()" whether the driver has set the map_sg/unmap_sg function pointers or not and if not set it to the default? Is bus_set_iommu() the only way drivers can set up the callbacks? > >> + } else { >> + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); >> + } >> + goto out; >> + >> +fail: >> + /* undo mappings already done in case of error */ >> + iommu_unmap(domain, iova, offset); >> +out: >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(iommu_map_sg); >> + >> +int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, >> + size_t size, unsigned long flags) >> +{ >> + if (unlikely(domain->ops->unmap_sg == NULL)) >> + return iommu_unmap(domain, iova, size); >> + else >> + return domain->ops->unmap_sg(domain, iova, size, flags); >> +} >> +EXPORT_SYMBOL_GPL(iommu_unmap_sg); >> >> int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr, >> phys_addr_t paddr, u64 size, int prot) >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index 20f9a52..66ad543 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -22,6 +22,7 @@ >> #include <linux/errno.h> >> #include <linux/err.h> >> #include <linux/types.h> >> +#include <linux/scatterlist.h> >> #include <trace/events/iommu.h> >> >> #define IOMMU_READ (1 << 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 >> @@ -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); >> phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova); >> int (*domain_has_cap)(struct iommu_domain *domain, >> unsigned long cap); >> @@ -153,6 +163,11 @@ extern int iommu_map(struct iommu_domain *domain, unsigned long iova, >> phys_addr_t paddr, size_t size, int prot); >> extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, >> size_t size); >> +extern int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, >> + struct scatterlist *sg, unsigned int nents, int prot, >> + unsigned long flags); >> +extern int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, >> + size_t size, unsigned long flags); >> extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova); >> extern int iommu_domain_has_cap(struct iommu_domain *domain, >> unsigned long cap); >> @@ -287,6 +302,19 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova, >> return -ENODEV; >> } >> >> +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 -ENODEV; >> +} >> + >> +static inline int iommu_unmap_sg(struct iommu_domain *domain, >> + unsigned long iova, size_t size, unsigned long flags) >> +{ >> + return -ENODEV; >> +} >> + >> static inline int iommu_domain_window_enable(struct iommu_domain *domain, >> u32 wnd_nr, phys_addr_t paddr, >> u64 size, int prot) >> -- >> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >> hosted by The Linux Foundation >> >> _______________________________________________ >> iommu mailing list >> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org >> https://lists.linuxfoundation.org/mailman/listinfo/iommu > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-06 17:08 ` Olav Haugan 0 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-06 17:08 UTC (permalink / raw) To: linux-arm-kernel On 8/5/2014 8:13 AM, Konrad Rzeszutek Wilk wrote: > On Thu, Jul 31, 2014 at 05:54:44PM -0700, 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. > > That is assuming that physical == bus topology. > >> >> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> >> --- >> drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/iommu.h | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 72 insertions(+) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 1698360..1d5dc2e 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -1088,6 +1088,50 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) >> } >> EXPORT_SYMBOL_GPL(iommu_unmap); >> >> +int 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; >> + >> + if (unlikely(domain->ops->map_sg == NULL)) { >> + unsigned int i; >> + struct scatterlist *s; >> + >> + 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) >> + goto fail; >> + >> + offset += page_len; >> + } > > I think it would be better if you had an 'default_iommu_map_sg' with > the implementation above. And then the default ops->map_sg would point to > that and each IOMMU would over-write with its own version. > > That way you don't need any of this 'if' and can have the 'iommu_map_sg' > be in the header file (either as static inline or an macro). so you are suggesting that I check in "bus_set_iommu()" whether the driver has set the map_sg/unmap_sg function pointers or not and if not set it to the default? Is bus_set_iommu() the only way drivers can set up the callbacks? > >> + } else { >> + ret = domain->ops->map_sg(domain, iova, sg, nents, prot, flags); >> + } >> + goto out; >> + >> +fail: >> + /* undo mappings already done in case of error */ >> + iommu_unmap(domain, iova, offset); >> +out: >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(iommu_map_sg); >> + >> +int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, >> + size_t size, unsigned long flags) >> +{ >> + if (unlikely(domain->ops->unmap_sg == NULL)) >> + return iommu_unmap(domain, iova, size); >> + else >> + return domain->ops->unmap_sg(domain, iova, size, flags); >> +} >> +EXPORT_SYMBOL_GPL(iommu_unmap_sg); >> >> int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr, >> phys_addr_t paddr, u64 size, int prot) >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index 20f9a52..66ad543 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -22,6 +22,7 @@ >> #include <linux/errno.h> >> #include <linux/err.h> >> #include <linux/types.h> >> +#include <linux/scatterlist.h> >> #include <trace/events/iommu.h> >> >> #define IOMMU_READ (1 << 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 >> @@ -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); >> phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova); >> int (*domain_has_cap)(struct iommu_domain *domain, >> unsigned long cap); >> @@ -153,6 +163,11 @@ extern int iommu_map(struct iommu_domain *domain, unsigned long iova, >> phys_addr_t paddr, size_t size, int prot); >> extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, >> size_t size); >> +extern int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, >> + struct scatterlist *sg, unsigned int nents, int prot, >> + unsigned long flags); >> +extern int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, >> + size_t size, unsigned long flags); >> extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova); >> extern int iommu_domain_has_cap(struct iommu_domain *domain, >> unsigned long cap); >> @@ -287,6 +302,19 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova, >> return -ENODEV; >> } >> >> +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 -ENODEV; >> +} >> + >> +static inline int iommu_unmap_sg(struct iommu_domain *domain, >> + unsigned long iova, size_t size, unsigned long flags) >> +{ >> + return -ENODEV; >> +} >> + >> static inline int iommu_domain_window_enable(struct iommu_domain *domain, >> u32 wnd_nr, phys_addr_t paddr, >> u64 size, int prot) >> -- >> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >> hosted by The Linux Foundation >> >> _______________________________________________ >> iommu mailing list >> iommu at lists.linux-foundation.org >> https://lists.linuxfoundation.org/mailman/listinfo/iommu > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <53E26127.1040805-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-06 17:08 ` Olav Haugan @ 2014-08-06 20:17 ` Joerg Roedel -1 siblings, 0 replies; 24+ messages in thread From: Joerg Roedel @ 2014-08-06 20:17 UTC (permalink / raw) To: Olav Haugan Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, will.deacon-5wv7dgnIgG8, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: > so you are suggesting that I check in "bus_set_iommu()" whether the > driver has set the map_sg/unmap_sg function pointers or not and if not > set it to the default? Is bus_set_iommu() the only way drivers can set > up the callbacks? This doesn't work as the iommu_ops are now const. You have to either update the iommu drivers individually to point to the default function, or you do the check in the API function itself and fall back to the default it no call-back is provided. Joerg ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-06 20:17 ` Joerg Roedel 0 siblings, 0 replies; 24+ messages in thread From: Joerg Roedel @ 2014-08-06 20:17 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: > so you are suggesting that I check in "bus_set_iommu()" whether the > driver has set the map_sg/unmap_sg function pointers or not and if not > set it to the default? Is bus_set_iommu() the only way drivers can set > up the callbacks? This doesn't work as the iommu_ops are now const. You have to either update the iommu drivers individually to point to the default function, or you do the check in the API function itself and fall back to the default it no call-back is provided. Joerg ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20140806201740.GW9809-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>]
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-06 20:17 ` Joerg Roedel @ 2014-08-06 23:28 ` Olav Haugan -1 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-06 23:28 UTC (permalink / raw) To: Joerg Roedel Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, will.deacon-5wv7dgnIgG8, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On 8/6/2014 1:17 PM, Joerg Roedel wrote: > On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: >> so you are suggesting that I check in "bus_set_iommu()" whether the >> driver has set the map_sg/unmap_sg function pointers or not and if not >> set it to the default? Is bus_set_iommu() the only way drivers can set >> up the callbacks? > > This doesn't work as the iommu_ops are now const. You have to either > update the iommu drivers individually to point to the default function, > or you do the check in the API function itself and fall back to the > default it no call-back is provided. > Ok, then I think it is better to just leave the fallback where it is now in the function itself. Thanks, Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-06 23:28 ` Olav Haugan 0 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-06 23:28 UTC (permalink / raw) To: linux-arm-kernel On 8/6/2014 1:17 PM, Joerg Roedel wrote: > On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: >> so you are suggesting that I check in "bus_set_iommu()" whether the >> driver has set the map_sg/unmap_sg function pointers or not and if not >> set it to the default? Is bus_set_iommu() the only way drivers can set >> up the callbacks? > > This doesn't work as the iommu_ops are now const. You have to either > update the iommu drivers individually to point to the default function, > or you do the check in the API function itself and fall back to the > default it no call-back is provided. > Ok, then I think it is better to just leave the fallback where it is now in the function itself. Thanks, Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-06 23:28 ` Olav Haugan @ 2014-08-07 6:24 ` Thierry Reding -1 siblings, 0 replies; 24+ messages in thread From: Thierry Reding @ 2014-08-07 6:24 UTC (permalink / raw) To: Olav Haugan Cc: Joerg Roedel, Konrad Rzeszutek Wilk, linux-arm-msm, will.deacon, iommu, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 1137 bytes --] On Wed, Aug 06, 2014 at 04:28:45PM -0700, Olav Haugan wrote: > On 8/6/2014 1:17 PM, Joerg Roedel wrote: > > On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: > >> so you are suggesting that I check in "bus_set_iommu()" whether the > >> driver has set the map_sg/unmap_sg function pointers or not and if not > >> set it to the default? Is bus_set_iommu() the only way drivers can set > >> up the callbacks? > > > > This doesn't work as the iommu_ops are now const. You have to either > > update the iommu drivers individually to point to the default function, > > or you do the check in the API function itself and fall back to the > > default it no call-back is provided. > > > > Ok, then I think it is better to just leave the fallback where it is now > in the function itself. What Konrad was suggesting is what I also proposed. The idea is to implement a fallback as standalone function, then make all drivers use that by default in the struct iommu_ops that they register. When drivers implement an optimized version they can simply replace the fallback implementation with their own. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-07 6:24 ` Thierry Reding 0 siblings, 0 replies; 24+ messages in thread From: Thierry Reding @ 2014-08-07 6:24 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 06, 2014 at 04:28:45PM -0700, Olav Haugan wrote: > On 8/6/2014 1:17 PM, Joerg Roedel wrote: > > On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: > >> so you are suggesting that I check in "bus_set_iommu()" whether the > >> driver has set the map_sg/unmap_sg function pointers or not and if not > >> set it to the default? Is bus_set_iommu() the only way drivers can set > >> up the callbacks? > > > > This doesn't work as the iommu_ops are now const. You have to either > > update the iommu drivers individually to point to the default function, > > or you do the check in the API function itself and fall back to the > > default it no call-back is provided. > > > > Ok, then I think it is better to just leave the fallback where it is now > in the function itself. What Konrad was suggesting is what I also proposed. The idea is to implement a fallback as standalone function, then make all drivers use that by default in the struct iommu_ops that they register. When drivers implement an optimized version they can simply replace the fallback implementation with their own. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140807/b1e51e16/attachment.sig> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-07 6:24 ` Thierry Reding @ 2014-08-07 21:52 ` Olav Haugan -1 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-07 21:52 UTC (permalink / raw) To: Thierry Reding Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, will.deacon-5wv7dgnIgG8, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On 8/6/2014 11:24 PM, Thierry Reding wrote: > On Wed, Aug 06, 2014 at 04:28:45PM -0700, Olav Haugan wrote: >> On 8/6/2014 1:17 PM, Joerg Roedel wrote: >>> On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: >>>> so you are suggesting that I check in "bus_set_iommu()" whether the >>>> driver has set the map_sg/unmap_sg function pointers or not and if not >>>> set it to the default? Is bus_set_iommu() the only way drivers can set >>>> up the callbacks? >>> >>> This doesn't work as the iommu_ops are now const. You have to either >>> update the iommu drivers individually to point to the default function, >>> or you do the check in the API function itself and fall back to the >>> default it no call-back is provided. >>> >> >> Ok, then I think it is better to just leave the fallback where it is now >> in the function itself. > > What Konrad was suggesting is what I also proposed. The idea is to > implement a fallback as standalone function, then make all drivers use > that by default in the struct iommu_ops that they register. When drivers > implement an optimized version they can simply replace the fallback > implementation with their own. > Ok, I can do that. I misunderstood the point of the fallback. I thought the point of the fallback was to catch drivers that forget/neglect to implement this callback. If that is not a concern I will update my patch to create a separate function that I will point all existing drivers to. Thanks, Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-07 21:52 ` Olav Haugan 0 siblings, 0 replies; 24+ messages in thread From: Olav Haugan @ 2014-08-07 21:52 UTC (permalink / raw) To: linux-arm-kernel On 8/6/2014 11:24 PM, Thierry Reding wrote: > On Wed, Aug 06, 2014 at 04:28:45PM -0700, Olav Haugan wrote: >> On 8/6/2014 1:17 PM, Joerg Roedel wrote: >>> On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: >>>> so you are suggesting that I check in "bus_set_iommu()" whether the >>>> driver has set the map_sg/unmap_sg function pointers or not and if not >>>> set it to the default? Is bus_set_iommu() the only way drivers can set >>>> up the callbacks? >>> >>> This doesn't work as the iommu_ops are now const. You have to either >>> update the iommu drivers individually to point to the default function, >>> or you do the check in the API function itself and fall back to the >>> default it no call-back is provided. >>> >> >> Ok, then I think it is better to just leave the fallback where it is now >> in the function itself. > > What Konrad was suggesting is what I also proposed. The idea is to > implement a fallback as standalone function, then make all drivers use > that by default in the struct iommu_ops that they register. When drivers > implement an optimized version they can simply replace the fallback > implementation with their own. > Ok, I can do that. I misunderstood the point of the fallback. I thought the point of the fallback was to catch drivers that forget/neglect to implement this callback. If that is not a concern I will update my patch to create a separate function that I will point all existing drivers to. Thanks, Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions 2014-08-07 21:52 ` Olav Haugan @ 2014-08-08 17:14 ` Konrad Rzeszutek Wilk -1 siblings, 0 replies; 24+ messages in thread From: Konrad Rzeszutek Wilk @ 2014-08-08 17:14 UTC (permalink / raw) To: Olav Haugan Cc: Thierry Reding, Joerg Roedel, linux-arm-msm, will.deacon, iommu, linux-arm-kernel On Thu, Aug 07, 2014 at 02:52:56PM -0700, Olav Haugan wrote: > On 8/6/2014 11:24 PM, Thierry Reding wrote: > > On Wed, Aug 06, 2014 at 04:28:45PM -0700, Olav Haugan wrote: > >> On 8/6/2014 1:17 PM, Joerg Roedel wrote: > >>> On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: > >>>> so you are suggesting that I check in "bus_set_iommu()" whether the > >>>> driver has set the map_sg/unmap_sg function pointers or not and if not > >>>> set it to the default? Is bus_set_iommu() the only way drivers can set > >>>> up the callbacks? > >>> > >>> This doesn't work as the iommu_ops are now const. You have to either > >>> update the iommu drivers individually to point to the default function, > >>> or you do the check in the API function itself and fall back to the > >>> default it no call-back is provided. > >>> > >> > >> Ok, then I think it is better to just leave the fallback where it is now > >> in the function itself. > > > > What Konrad was suggesting is what I also proposed. The idea is to > > implement a fallback as standalone function, then make all drivers use > > that by default in the struct iommu_ops that they register. When drivers > > implement an optimized version they can simply replace the fallback > > implementation with their own. > > > > Ok, I can do that. I misunderstood the point of the fallback. I thought > the point of the fallback was to catch drivers that forget/neglect to > implement this callback. If that is not a concern I will update my patch Nah. We want those drivers to crash and burn so we can see that and fix it. And by fix I meant it would just point do: .map_sg = generic_map_sg, .unmap_sg = generic_unmap_sg, In other words, none of the function ops will have an NULL functions. > to create a separate function that I will point all existing drivers to. Excellent! > > Thanks, > > Olav > > -- > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions @ 2014-08-08 17:14 ` Konrad Rzeszutek Wilk 0 siblings, 0 replies; 24+ messages in thread From: Konrad Rzeszutek Wilk @ 2014-08-08 17:14 UTC (permalink / raw) To: linux-arm-kernel On Thu, Aug 07, 2014 at 02:52:56PM -0700, Olav Haugan wrote: > On 8/6/2014 11:24 PM, Thierry Reding wrote: > > On Wed, Aug 06, 2014 at 04:28:45PM -0700, Olav Haugan wrote: > >> On 8/6/2014 1:17 PM, Joerg Roedel wrote: > >>> On Wed, Aug 06, 2014 at 10:08:55AM -0700, Olav Haugan wrote: > >>>> so you are suggesting that I check in "bus_set_iommu()" whether the > >>>> driver has set the map_sg/unmap_sg function pointers or not and if not > >>>> set it to the default? Is bus_set_iommu() the only way drivers can set > >>>> up the callbacks? > >>> > >>> This doesn't work as the iommu_ops are now const. You have to either > >>> update the iommu drivers individually to point to the default function, > >>> or you do the check in the API function itself and fall back to the > >>> default it no call-back is provided. > >>> > >> > >> Ok, then I think it is better to just leave the fallback where it is now > >> in the function itself. > > > > What Konrad was suggesting is what I also proposed. The idea is to > > implement a fallback as standalone function, then make all drivers use > > that by default in the struct iommu_ops that they register. When drivers > > implement an optimized version they can simply replace the fallback > > implementation with their own. > > > > Ok, I can do that. I misunderstood the point of the fallback. I thought > the point of the fallback was to catch drivers that forget/neglect to > implement this callback. If that is not a concern I will update my patch Nah. We want those drivers to crash and burn so we can see that and fix it. And by fix I meant it would just point do: .map_sg = generic_map_sg, .unmap_sg = generic_unmap_sg, In other words, none of the function ops will have an NULL functions. > to create a separate function that I will point all existing drivers to. Excellent! > > Thanks, > > Olav > > -- > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2014-08-08 17:14 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-01 0:54 [PATCH v4 0/1] Add iommu map_sg/unmap_sg API Olav Haugan
2014-08-01 0:54 ` Olav Haugan
2014-08-01 0:54 ` [PATCH v4 1/1] iommu-api: Add map_sg/unmap_sg functions Olav Haugan
2014-08-01 0:54 ` Olav Haugan
[not found] ` <1406854484-3848-2-git-send-email-ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2014-08-01 8:22 ` Will Deacon
2014-08-01 8:22 ` Will Deacon
[not found] ` <20140801082228.GC15733-5wv7dgnIgG8@public.gmane.org>
2014-08-01 16:44 ` Olav Haugan
2014-08-01 16:44 ` Olav Haugan
[not found] ` <53DBC3F1.40705-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2014-08-04 18:03 ` Olav Haugan
2014-08-04 18:03 ` Olav Haugan
2014-08-05 15:13 ` Konrad Rzeszutek Wilk
2014-08-05 15:13 ` Konrad Rzeszutek Wilk
[not found] ` <20140805151323.GB19709-0iZWjJA6G8GSPmnEAIUT9EEOCMrvLtNR@public.gmane.org>
2014-08-06 17:08 ` Olav Haugan
2014-08-06 17:08 ` Olav Haugan
[not found] ` <53E26127.1040805-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2014-08-06 20:17 ` Joerg Roedel
2014-08-06 20:17 ` Joerg Roedel
[not found] ` <20140806201740.GW9809-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
2014-08-06 23:28 ` Olav Haugan
2014-08-06 23:28 ` Olav Haugan
2014-08-07 6:24 ` Thierry Reding
2014-08-07 6:24 ` Thierry Reding
2014-08-07 21:52 ` Olav Haugan
2014-08-07 21:52 ` Olav Haugan
2014-08-08 17:14 ` Konrad Rzeszutek Wilk
2014-08-08 17:14 ` Konrad Rzeszutek Wilk
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.