From: Christoph Hellwig <hch@lst.de>
For the upcoming IOVA-based DMA API we want to batch the
ops->iotlb_sync_map() call after mapping multiple IOVAs from
dma-iommu without having a scatterlist. Improve the API.
Add a wrapper for the map_sync as iommu_sync_map() so that callers
don't need to poke into the methods directly.
Formalize __iommu_map() into iommu_map_nosync() which requires the
caller to call iommu_sync_map() after all maps are completed.
Refactor the existing sanity checks from all the different layers
into iommu_map_nosync().
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Will Deacon <will@kernel.org>
Tested-by: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/iommu/iommu.c | 65 +++++++++++++++++++------------------------
include/linux/iommu.h | 4 +++
2 files changed, 33 insertions(+), 36 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4f91a740c15f..02960585b8d4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2443,8 +2443,8 @@ static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
return pgsize;
}
-static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
- phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+int iommu_map_nosync(struct iommu_domain *domain, unsigned long iova,
+ phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
{
const struct iommu_domain_ops *ops = domain->ops;
unsigned long orig_iova = iova;
@@ -2453,12 +2453,19 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
phys_addr_t orig_paddr = paddr;
int ret = 0;
+ might_sleep_if(gfpflags_allow_blocking(gfp));
+
if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
return -EINVAL;
if (WARN_ON(!ops->map_pages || domain->pgsize_bitmap == 0UL))
return -ENODEV;
+ /* Discourage passing strange GFP flags */
+ if (WARN_ON_ONCE(gfp & (__GFP_COMP | __GFP_DMA | __GFP_DMA32 |
+ __GFP_HIGHMEM)))
+ return -EINVAL;
+
/* find out the minimum page size supported */
min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
@@ -2506,31 +2513,27 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
return ret;
}
-int iommu_map(struct iommu_domain *domain, unsigned long iova,
- phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+int iommu_sync_map(struct iommu_domain *domain, unsigned long iova, size_t size)
{
const struct iommu_domain_ops *ops = domain->ops;
- int ret;
-
- might_sleep_if(gfpflags_allow_blocking(gfp));
- /* Discourage passing strange GFP flags */
- if (WARN_ON_ONCE(gfp & (__GFP_COMP | __GFP_DMA | __GFP_DMA32 |
- __GFP_HIGHMEM)))
- return -EINVAL;
+ if (!ops->iotlb_sync_map)
+ return 0;
+ return ops->iotlb_sync_map(domain, iova, size);
+}
- ret = __iommu_map(domain, iova, paddr, size, prot, gfp);
- if (ret == 0 && ops->iotlb_sync_map) {
- ret = ops->iotlb_sync_map(domain, iova, size);
- if (ret)
- goto out_err;
- }
+int iommu_map(struct iommu_domain *domain, unsigned long iova,
+ phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+{
+ int ret;
- return ret;
+ ret = iommu_map_nosync(domain, iova, paddr, size, prot, gfp);
+ if (ret)
+ return ret;
-out_err:
- /* undo mappings already done */
- iommu_unmap(domain, iova, size);
+ ret = iommu_sync_map(domain, iova, size);
+ if (ret)
+ iommu_unmap(domain, iova, size);
return ret;
}
@@ -2630,26 +2633,17 @@ ssize_t iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
struct scatterlist *sg, unsigned int nents, int prot,
gfp_t gfp)
{
- const struct iommu_domain_ops *ops = domain->ops;
size_t len = 0, mapped = 0;
phys_addr_t start;
unsigned int i = 0;
int ret;
- might_sleep_if(gfpflags_allow_blocking(gfp));
-
- /* Discourage passing strange GFP flags */
- if (WARN_ON_ONCE(gfp & (__GFP_COMP | __GFP_DMA | __GFP_DMA32 |
- __GFP_HIGHMEM)))
- return -EINVAL;
-
while (i <= nents) {
phys_addr_t s_phys = sg_phys(sg);
if (len && s_phys != start + len) {
- ret = __iommu_map(domain, iova + mapped, start,
+ ret = iommu_map_nosync(domain, iova + mapped, start,
len, prot, gfp);
-
if (ret)
goto out_err;
@@ -2672,11 +2666,10 @@ ssize_t iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
sg = sg_next(sg);
}
- if (ops->iotlb_sync_map) {
- ret = ops->iotlb_sync_map(domain, iova, mapped);
- if (ret)
- goto out_err;
- }
+ ret = iommu_sync_map(domain, iova, mapped);
+ if (ret)
+ goto out_err;
+
return mapped;
out_err:
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ccce8a751e2a..ce472af8e9c3 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -872,6 +872,10 @@ extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
extern struct iommu_domain *iommu_get_dma_domain(struct device *dev);
extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
+int iommu_map_nosync(struct iommu_domain *domain, unsigned long iova,
+ phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
+int iommu_sync_map(struct iommu_domain *domain, unsigned long iova,
+ size_t size);
extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova,
size_t size);
extern size_t iommu_unmap_fast(struct iommu_domain *domain,
--
2.49.0On 4/28/25 17:22, Leon Romanovsky wrote:
> From: Christoph Hellwig<hch@lst.de>
>
> For the upcoming IOVA-based DMA API we want to batch the
> ops->iotlb_sync_map() call after mapping multiple IOVAs from
> dma-iommu without having a scatterlist. Improve the API.
>
> Add a wrapper for the map_sync as iommu_sync_map() so that callers
> don't need to poke into the methods directly.
>
> Formalize __iommu_map() into iommu_map_nosync() which requires the
> caller to call iommu_sync_map() after all maps are completed.
>
> Refactor the existing sanity checks from all the different layers
> into iommu_map_nosync().
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
> Acked-by: Will Deacon<will@kernel.org>
> Tested-by: Jens Axboe<axboe@kernel.dk>
> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
> Reviewed-by: Luis Chamberlain<mcgrof@kernel.org>
> Signed-off-by: Leon Romanovsky<leonro@nvidia.com>
> ---
> drivers/iommu/iommu.c | 65 +++++++++++++++++++------------------------
> include/linux/iommu.h | 4 +++
> 2 files changed, 33 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4f91a740c15f..02960585b8d4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2443,8 +2443,8 @@ static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
> return pgsize;
> }
>
> -static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
> - phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> +int iommu_map_nosync(struct iommu_domain *domain, unsigned long iova,
> + phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> {
> const struct iommu_domain_ops *ops = domain->ops;
> unsigned long orig_iova = iova;
> @@ -2453,12 +2453,19 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
> phys_addr_t orig_paddr = paddr;
> int ret = 0;
>
> + might_sleep_if(gfpflags_allow_blocking(gfp));
> +
> if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
> return -EINVAL;
>
> if (WARN_ON(!ops->map_pages || domain->pgsize_bitmap == 0UL))
> return -ENODEV;
>
> + /* Discourage passing strange GFP flags */
> + if (WARN_ON_ONCE(gfp & (__GFP_COMP | __GFP_DMA | __GFP_DMA32 |
> + __GFP_HIGHMEM)))
> + return -EINVAL;
> +
> /* find out the minimum page size supported */
> min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
>
> @@ -2506,31 +2513,27 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
> return ret;
> }
>
> -int iommu_map(struct iommu_domain *domain, unsigned long iova,
> - phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> +int iommu_sync_map(struct iommu_domain *domain, unsigned long iova, size_t size)
> {
> const struct iommu_domain_ops *ops = domain->ops;
> - int ret;
> -
> - might_sleep_if(gfpflags_allow_blocking(gfp));
>
> - /* Discourage passing strange GFP flags */
> - if (WARN_ON_ONCE(gfp & (__GFP_COMP | __GFP_DMA | __GFP_DMA32 |
> - __GFP_HIGHMEM)))
> - return -EINVAL;
> + if (!ops->iotlb_sync_map)
> + return 0;
> + return ops->iotlb_sync_map(domain, iova, size);
> +}
I am wondering whether iommu_sync_map() needs a return value. The
purpose of this callback is just to sync the TLB cache after new
mappings are created, which should effectively be a no-fail operation.
The definition of iotlb_sync_map in struct iommu_domain_ops seems
unnecessary:
struct iommu_domain_ops {
...
int (*iotlb_sync_map)(struct iommu_domain *domain, unsigned
long iova,
size_t size);
...
};
Furthermore, currently no iommu driver implements this callback in a way
that returns a failure. We could clean up the iommu definition in a
subsequent patch series, but for this driver-facing interface, it's
better to get it right from the beginning.
Thanks,
baolu
On Tue, Apr 29, 2025 at 10:19:46AM +0800, Baolu Lu wrote:
> > -int iommu_map(struct iommu_domain *domain, unsigned long iova,
> > - phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> > +int iommu_sync_map(struct iommu_domain *domain, unsigned long iova, size_t size)
> > {
> > const struct iommu_domain_ops *ops = domain->ops;
> > - int ret;
> > -
> > - might_sleep_if(gfpflags_allow_blocking(gfp));
> > - /* Discourage passing strange GFP flags */
> > - if (WARN_ON_ONCE(gfp & (__GFP_COMP | __GFP_DMA | __GFP_DMA32 |
> > - __GFP_HIGHMEM)))
> > - return -EINVAL;
> > + if (!ops->iotlb_sync_map)
> > + return 0;
> > + return ops->iotlb_sync_map(domain, iova, size);
> > +}
>
> I am wondering whether iommu_sync_map() needs a return value. The
> purpose of this callback is just to sync the TLB cache after new
> mappings are created, which should effectively be a no-fail operation.
Yeah, it is pretty much nonsense, the other flushes don't fail:
void (*flush_iotlb_all)(struct iommu_domain *domain);
int (*iotlb_sync_map)(struct iommu_domain *domain, unsigned long iova,
size_t size);
void (*iotlb_sync)(struct iommu_domain *domain,
struct iommu_iotlb_gather *iotlb_gather);
> Furthermore, currently no iommu driver implements this callback in a way
> that returns a failure.
Given s390 does weirdly fail sync_map but not sync this needs a bigger
touch than just that.
But what I really want to do is get rid of iotlb_sync_map and replace it
with iotlb_sync, and feed the gather through the iommu_map path.
It doesn't really make sense to have a special interface for this.
So I think this patch is fine as is..
Jason
On Tue, Apr 29, 2025 at 10:19:46AM +0800, Baolu Lu wrote:
> On 4/28/25 17:22, Leon Romanovsky wrote:
> > From: Christoph Hellwig<hch@lst.de>
> >
> > For the upcoming IOVA-based DMA API we want to batch the
> > ops->iotlb_sync_map() call after mapping multiple IOVAs from
> > dma-iommu without having a scatterlist. Improve the API.
> >
> > Add a wrapper for the map_sync as iommu_sync_map() so that callers
> > don't need to poke into the methods directly.
> >
> > Formalize __iommu_map() into iommu_map_nosync() which requires the
> > caller to call iommu_sync_map() after all maps are completed.
> >
> > Refactor the existing sanity checks from all the different layers
> > into iommu_map_nosync().
> >
> > Signed-off-by: Christoph Hellwig<hch@lst.de>
> > Acked-by: Will Deacon<will@kernel.org>
> > Tested-by: Jens Axboe<axboe@kernel.dk>
> > Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
> > Reviewed-by: Luis Chamberlain<mcgrof@kernel.org>
> > Signed-off-by: Leon Romanovsky<leonro@nvidia.com>
> > ---
> > drivers/iommu/iommu.c | 65 +++++++++++++++++++------------------------
> > include/linux/iommu.h | 4 +++
> > 2 files changed, 33 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 4f91a740c15f..02960585b8d4 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -2443,8 +2443,8 @@ static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
> > return pgsize;
> > }
> > -static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
> > - phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> > +int iommu_map_nosync(struct iommu_domain *domain, unsigned long iova,
> > + phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> > {
> > const struct iommu_domain_ops *ops = domain->ops;
> > unsigned long orig_iova = iova;
> > @@ -2453,12 +2453,19 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
> > phys_addr_t orig_paddr = paddr;
> > int ret = 0;
> > + might_sleep_if(gfpflags_allow_blocking(gfp));
> > +
> > if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
> > return -EINVAL;
> > if (WARN_ON(!ops->map_pages || domain->pgsize_bitmap == 0UL))
> > return -ENODEV;
> > + /* Discourage passing strange GFP flags */
> > + if (WARN_ON_ONCE(gfp & (__GFP_COMP | __GFP_DMA | __GFP_DMA32 |
> > + __GFP_HIGHMEM)))
> > + return -EINVAL;
> > +
> > /* find out the minimum page size supported */
> > min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> > @@ -2506,31 +2513,27 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
> > return ret;
> > }
> > -int iommu_map(struct iommu_domain *domain, unsigned long iova,
> > - phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> > +int iommu_sync_map(struct iommu_domain *domain, unsigned long iova, size_t size)
> > {
> > const struct iommu_domain_ops *ops = domain->ops;
> > - int ret;
> > -
> > - might_sleep_if(gfpflags_allow_blocking(gfp));
> > - /* Discourage passing strange GFP flags */
> > - if (WARN_ON_ONCE(gfp & (__GFP_COMP | __GFP_DMA | __GFP_DMA32 |
> > - __GFP_HIGHMEM)))
> > - return -EINVAL;
> > + if (!ops->iotlb_sync_map)
> > + return 0;
> > + return ops->iotlb_sync_map(domain, iova, size);
> > +}
>
> I am wondering whether iommu_sync_map() needs a return value. The
> purpose of this callback is just to sync the TLB cache after new
> mappings are created, which should effectively be a no-fail operation.
>
> The definition of iotlb_sync_map in struct iommu_domain_ops seems
> unnecessary:
>
> struct iommu_domain_ops {
> ...
> int (*iotlb_sync_map)(struct iommu_domain *domain, unsigned long
> iova,
> size_t size);
> ...
> };
>
> Furthermore, currently no iommu driver implements this callback in a way
> that returns a failure. We could clean up the iommu definition in a
> subsequent patch series, but for this driver-facing interface, it's
> better to get it right from the beginning.
I see that s390 is relying on return values:
569 static int s390_iommu_iotlb_sync_map(struct iommu_domain *domain,
570 unsigned long iova, size_t size)
571 {
<...>
581 ret = zpci_refresh_trans((u64)zdev->fh << 32,
582 iova, size);
583 /*
584 * let the hypervisor discover invalidated entries
585 * allowing it to free IOVAs and unpin pages
586 */
587 if (ret == -ENOMEM) {
588 ret = zpci_refresh_all(zdev);
589 if (ret)
590 break;
591 }
<...>
595 return ret;
596 }
>
> Thanks,
> baolu
© 2016 - 2025 Red Hat, Inc.