| Message ID | 1407797150-515-2-git-send-email-ohaugan@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 11, 2014 at 03:45:50PM -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. > > Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> Thank you for changing it this way. .. snip.. > + 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? > + break; > + } > + offset += page_len; > + } > + > + return ret; > +} -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Olav, Olav Haugan <ohaugan@codeaurora.org> writes: > @@ -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); 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? -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 11, 2014 at 9:51 PM, Hiroshi Doyu <hdoyu@nvidia.com> wrote: > Hi Olav, > > Olav Haugan <ohaugan@codeaurora.org> writes: > >> @@ -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); > > 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? 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. BR, -R -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/11/2014 6:35 PM, Konrad Rzeszutek Wilk wrote: > On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote: > .. snip.. >> + 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? > It reverts all the mapping that was already done up until the error occurred. "offset" contains the amount we have mapped so far. Thanks, Olav
Hi Olav, Thank you for the patch. 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 <ohaugan@codeaurora.org> > --- > 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] > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 1698360..24cf727 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain, > unsigned long iova, size_t size) [snip] > +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. > +{ > + 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] > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 20f9a52..ee106ce 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h [snip] > @@ -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); > +} > + > +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); > +} > +
On 8/12/2014 3:48 AM, Rob Clark wrote: > On Mon, Aug 11, 2014 at 9:51 PM, Hiroshi Doyu <hdoyu@nvidia.com> wrote: >> Hi Olav, >> >> Olav Haugan <ohaugan@codeaurora.org> writes: >> >>> @@ -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); >> >> 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? > > 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. Thanks, Olav
Hi Laurent, On 8/12/2014 9:55 AM, Laurent Pinchart wrote: > Hi Olav, > > Thank you for the patch. > > 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 <ohaugan@codeaurora.org> >> --- >> 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] > >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 1698360..24cf727 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain, >> unsigned long iova, size_t size) > > [snip] > >> +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. > >> +{ >> + 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 ? Good question. Yes, maybe some drivers does not need or want to override this but a use case is to provide a more optimized version of the map_sg/unmap_sg functions. For example a very simple way to optimize this would be to have an implementation that unmaps everything and then does a TLB invalidate instead of doing a TLB invalidate after every single unmap (which happens with the default implementation if your driver does TLB invalidate after unmapping). > [snip] > >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index 20f9a52..ee106ce 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h > > [snip] > >> @@ -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); This was my original proposal but after some discussion on the list we ended up with what we have now. > >> +} >> + >> +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); >> +} >> + > Thanks, Olav
On Tue, Aug 12, 2014 at 09:56:11AM -0700, Olav Haugan wrote: > On 8/12/2014 3:48 AM, Rob Clark wrote: > > 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 18, 2014 at 10:07 AM, joro@8bytes.org <joro@8bytes.org> wrote: > On Tue, Aug 12, 2014 at 09:56:11AM -0700, Olav Haugan wrote: >> On 8/12/2014 3:48 AM, Rob Clark wrote: >> > 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. > > 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? > 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 BR, -R > Joerg > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/18/2014 11:32 AM, Rob Clark wrote: > On Mon, Aug 18, 2014 at 10:07 AM, joro@8bytes.org <joro@8bytes.org> wrote: >> On Tue, Aug 12, 2014 at 09:56:11AM -0700, Olav Haugan wrote: >>> On 8/12/2014 3:48 AM, Rob Clark wrote: >>>> 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. >> >> Do you have anything else in mind already besides the DONT_FLUSH_TLB >> flag? No, I do not have other uses right now. But could imagine use cases like "force <insert minimum mapping size here> mapping" flag etc. >> 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 > I agree and would support adding a public flush() API. It is cleaner. However, it does allow clients to chose which method to use. Either an extra call to flush() or just do not pass DONT_FLUSH_TLB flag on the last invocation of map/unmap. Thanks, Olav
On Mon, Aug 18, 2014 at 01:48:46PM -0700, Olav Haugan wrote: > 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. Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/18/2014 2:26 PM, joro@8bytes.org wrote: > On Mon, Aug 18, 2014 at 01:48:46PM -0700, Olav Haugan wrote: >> 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. > Sure, I can remove it. I was trying to make iommu_map_sg/iommu_unmap_sg symmetric :-) Anything else before I push v6? Thanks, Olav
On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote: > +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)); > + 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. > +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(). > +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); > +} These function pointers need to be checked for != NULL before calling them. Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/18/2014 2:55 PM, Joerg Roedel wrote: > On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote: >> +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)); >> + 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. 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. >> +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(). 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). >> +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); >> +} > > These function pointers need to be checked for != NULL before calling > them. 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). Thanks, Olav
On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: > 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. > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: > On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: > > 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. > > > 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. 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. > > 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.
On 8/19/2014 4:59 AM, Joerg Roedel wrote: > On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: >> 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. Ok, but I don't think we want to force a specific alignment here (as I discussed earlier with Will D.). So I would just do something like iommu_map(domain, iova + offset, phys, s->length, prot); offset += s->length; >> 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. So are you looking for something like this then: int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, struct scatterlist *sg, unsigned int nents, int prot, unsigned long flags) int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, struct scatterlist *sg, unsigned int nents, unsigned long flags) This makes unmapping code more complicated (and slow) though. For example the way I would implement the fallback would be to iterate through the sglist first to calculate the total length to unmap and then call iommu_unmap(). I know we talked about removing the flag argument in the map call. However, the TLB_NO_INV usage is actually needed for both map and unmap. We have HW that requires TLB invalidate on MAP and TLB inv. is also required if you implement your mapping function to support replacing an existing mapping... Rob, did you have an issue with this API before or was it just an issue with not having the iova in the unmap call? >> 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > Olav
On 8/19/2014 9:11 AM, Laurent Pinchart wrote: > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: >>> 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. >> >>> 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. > > 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. > >>> 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. > Both Thierry R. and Konrad W. argued for modifying the drivers instead so I implemented what the majority wanted. :-) Olav
On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote: > On 8/19/2014 9:11 AM, Laurent Pinchart wrote: > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: > >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: > >>> 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. > >> > >>> 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. > > > > 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. > > > >>> 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. > > 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.
On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote: > On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote: > > On 8/19/2014 9:11 AM, Laurent Pinchart wrote: > > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: > > >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: > > >>> 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. > > >> > > >>> 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. > > > > > > 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. > > > > > >>> 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. > > > > 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. In my opinion it's much more direct that way. It means that if a driver doesn't implement it, it won't fall back to some default implementation instead. Providing an explicit helper like this makes it obvious that the driver is using a default implementation rather than making things work "magically". It's easier to see in the driver that there's the potential to optimize. It also has the side-effect of keeping the core code cleaner in my opinion, since the core iommu_map_sg() and iommu_unmap_sg() functions can now blindly call into drivers directly rather than performing the various checks to see if they implement the required functionality. Thierry
On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote: > On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote: > > On 8/19/2014 9:11 AM, Laurent Pinchart wrote: > > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: > > >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: > > >>> 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. > > >> > > >>> 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. > > > > > > 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. > > > > > >>> 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. > > > > 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. 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. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Konrad, On Wednesday 20 August 2014 09:02:50 Konrad Rzeszutek Wilk wrote: > On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote: > > On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote: > > > On 8/19/2014 9:11 AM, Laurent Pinchart wrote: > > > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: [snip] > > > >> 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. > > > > > > 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. > > 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. That's because the other functions are mandatory, while this particular one is just an optimization and can be implemented generically. > 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). I don't think that applies in this case, the generic implementation should work in all cases. > 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. That's an argument I can buy.
On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote: > +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). Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9/25/2014 10:01 AM, Joerg Roedel wrote: > On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote: >> +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). > Ok, sounds good. I'll post v6 soon. .Olav
On Mon, Oct 06, 2014 at 12:02:47PM -0700, Olav Haugan wrote: > On 9/25/2014 10:01 AM, Joerg Roedel wrote: > >On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote: > >>+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). > > > > Ok, sounds good. I'll post v6 soon. Perhaps make the return value ssize_t so that we can propagate errors? Thierry
On 10/15/2014 2:16 AM, Thierry Reding wrote: > On Mon, Oct 06, 2014 at 12:02:47PM -0700, Olav Haugan wrote: >> On 9/25/2014 10:01 AM, Joerg Roedel wrote: >>> On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote: >>>> +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). >>> >> >> Ok, sounds good. I'll post v6 soon. > > Perhaps make the return value ssize_t so that we can propagate errors? > I am fine with that. Joerg? Thanks, .Olav
On Thu, Oct 16, 2014 at 10:23:07AM -0700, Olav Haugan wrote: > On 10/15/2014 2:16 AM, Thierry Reding wrote: > >Perhaps make the return value ssize_t so that we can propagate errors? > > > > I am fine with that. Joerg? I am not so sure this is a good idea. On error the function should return the size that is already mapped, so that the caller can check that against the expected size and just call iommu_unmap with the returned size when return value does not match expected size. The benefit of ssize_t is that we can return more detailed error information when the function did not map anything yet. But this complicates the error handling for the caller and implementers might get it wrong and return an error-value even when some space was already mapped, so that the caller can not unmap the addresses. So taking this into mind, I prefer it to be size_t for now. We can always change it later if it turns out to be wrong. Regards, Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index 1840531..3604f3c 100644 --- a/drivers/iommu/amd_iommu.c +++ b/drivers/iommu/amd_iommu.c @@ -3402,6 +3402,8 @@ static const struct iommu_ops amd_iommu_ops = { .detach_dev = amd_iommu_detach_device, .map = amd_iommu_map, .unmap = amd_iommu_unmap, + .map_sg = default_iommu_map_sg, + .unmap_sg = default_iommu_unmap_sg, .iova_to_phys = amd_iommu_iova_to_phys, .domain_has_cap = amd_iommu_domain_has_cap, .pgsize_bitmap = AMD_IOMMU_PGSIZES, diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index ca18d6d..231e006 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1604,6 +1604,8 @@ static const struct iommu_ops arm_smmu_ops = { .detach_dev = arm_smmu_detach_dev, .map = arm_smmu_map, .unmap = arm_smmu_unmap, + .map_sg = default_iommu_map_sg, + .unmap_sg = default_iommu_unmap_sg, .iova_to_phys = arm_smmu_iova_to_phys, .domain_has_cap = arm_smmu_domain_has_cap, .add_device = arm_smmu_add_device, diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index d037e87..8876508 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -1177,6 +1177,8 @@ static const struct iommu_ops exynos_iommu_ops = { .detach_dev = exynos_iommu_detach_device, .map = exynos_iommu_map, .unmap = exynos_iommu_unmap, + .map_sg = default_iommu_map_sg, + .unmap_sg = default_iommu_unmap_sg, .iova_to_phys = exynos_iommu_iova_to_phys, .add_device = exynos_iommu_add_device, .remove_device = exynos_iommu_remove_device, diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index d1f5caa..4fc61ff 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -4462,6 +4462,8 @@ static const struct iommu_ops intel_iommu_ops = { .detach_dev = intel_iommu_detach_device, .map = intel_iommu_map, .unmap = intel_iommu_unmap, + .map_sg = default_iommu_map_sg, + .unmap_sg = default_iommu_unmap_sg, .iova_to_phys = intel_iommu_iova_to_phys, .domain_has_cap = intel_iommu_domain_has_cap, .add_device = intel_iommu_add_device, diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 1698360..24cf727 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) } EXPORT_SYMBOL_GPL(iommu_unmap); +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)); + 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); + break; + } + offset += page_len; + } + + return ret; +} +EXPORT_SYMBOL_GPL(default_iommu_map_sg); + +int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, + size_t size, unsigned long flags) +{ + return iommu_unmap(domain, iova, size); +} +EXPORT_SYMBOL_GPL(default_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/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c index 7dab5cb..1dc6f94 100644 --- a/drivers/iommu/ipmmu-vmsa.c +++ b/drivers/iommu/ipmmu-vmsa.c @@ -1127,6 +1127,8 @@ static const struct iommu_ops ipmmu_ops = { .detach_dev = ipmmu_detach_device, .map = ipmmu_map, .unmap = ipmmu_unmap, + .map_sg = default_iommu_map_sg, + .unmap_sg = default_iommu_unmap_sg, .iova_to_phys = ipmmu_iova_to_phys, .add_device = ipmmu_add_device, .remove_device = ipmmu_remove_device, diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c index 49f41d6..2b3c8d3 100644 --- a/drivers/iommu/msm_iommu.c +++ b/drivers/iommu/msm_iommu.c @@ -681,6 +681,8 @@ static const struct iommu_ops msm_iommu_ops = { .detach_dev = msm_iommu_detach_dev, .map = msm_iommu_map, .unmap = msm_iommu_unmap, + .map_sg = default_iommu_map_sg, + .unmap_sg = default_iommu_unmap_sg, .iova_to_phys = msm_iommu_iova_to_phys, .domain_has_cap = msm_iommu_domain_has_cap, .pgsize_bitmap = MSM_IOMMU_PGSIZES, diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index e202b0c..7d4c920 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -1285,6 +1285,8 @@ static const struct iommu_ops omap_iommu_ops = { .detach_dev = omap_iommu_detach_dev, .map = omap_iommu_map, .unmap = omap_iommu_unmap, + .map_sg = default_iommu_map_sg, + .unmap_sg = default_iommu_unmap_sg, .iova_to_phys = omap_iommu_iova_to_phys, .add_device = omap_iommu_add_device, .remove_device = omap_iommu_remove_device, diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c index 1333e6fb..3510f70 100644 --- a/drivers/iommu/shmobile-iommu.c +++ b/drivers/iommu/shmobile-iommu.c @@ -361,6 +361,8 @@ static const struct iommu_ops shmobile_iommu_ops = { .detach_dev = shmobile_iommu_detach_device, .map = shmobile_iommu_map, .unmap = shmobile_iommu_unmap, + .map_sg = default_iommu_map_sg, + .unmap_sg = default_iommu_unmap_sg, .iova_to_phys = shmobile_iommu_iova_to_phys, .add_device = shmobile_iommu_add_device, .pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K, diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 792da5e..ebd847b 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -954,6 +954,8 @@ static const struct iommu_ops smmu_iommu_ops = { .detach_dev = smmu_iommu_detach_dev, .map = smmu_iommu_map, .unmap = smmu_iommu_unmap, + .map_sg = default_iommu_map_sg, + .unmap_sg = default_iommu_unmap_sg, .iova_to_phys = smmu_iommu_iova_to_phys, .domain_has_cap = smmu_iommu_domain_has_cap, .pgsize_bitmap = SMMU_IOMMU_PGSIZES, diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 20f9a52..ee106ce 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,12 @@ 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 default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova, + struct scatterlist *sg,unsigned int nents, + int prot, unsigned long flags); +extern int default_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); @@ -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); +} + +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); +} + #else /* CONFIG_IOMMU_API */ struct iommu_ops {}; @@ -287,6 +317,20 @@ 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)
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/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(+)