linux-kernel - Re: [PATCH v10 03/24] iommu: generalize the batched sync after map interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f8d86cde-d485-4e5a-a693-e9323679474f@linux.intel.com>
Date: Tue, 29 Apr 2025 10:19:46 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Leon Romanovsky <leon@...nel.org>,
 Marek Szyprowski <m.szyprowski@...sung.com>, Jens Axboe <axboe@...nel.dk>,
 Christoph Hellwig <hch@....de>, Keith Busch <kbusch@...nel.org>
Cc: Jake Edge <jake@....net>, Jonathan Corbet <corbet@....net>,
 Jason Gunthorpe <jgg@...pe.ca>, Zhu Yanjun <zyjzyj2000@...il.com>,
 Robin Murphy <robin.murphy@....com>, Joerg Roedel <joro@...tes.org>,
 Will Deacon <will@...nel.org>, Sagi Grimberg <sagi@...mberg.me>,
 Bjorn Helgaas <bhelgaas@...gle.com>, Logan Gunthorpe <logang@...tatee.com>,
 Yishai Hadas <yishaih@...dia.com>,
 Shameer Kolothum <shameerali.kolothum.thodi@...wei.com>,
 Kevin Tian <kevin.tian@...el.com>,
 Alex Williamson <alex.williamson@...hat.com>,
 Jérôme Glisse <jglisse@...hat.com>,
 Andrew Morton <akpm@...ux-foundation.org>, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
 linux-rdma@...r.kernel.org, iommu@...ts.linux.dev,
 linux-nvme@...ts.infradead.org, linux-pci@...r.kernel.org,
 kvm@...r.kernel.org, linux-mm@...ck.org,
 Niklas Schnelle <schnelle@...ux.ibm.com>,
 Chuck Lever <chuck.lever@...cle.com>, Luis Chamberlain <mcgrof@...nel.org>,
 Matthew Wilcox <willy@...radead.org>, Dan Williams
 <dan.j.williams@...el.com>, Kanchan Joshi <joshi.k@...sung.com>,
 Chaitanya Kulkarni <kch@...dia.com>, Jason Gunthorpe <jgg@...dia.com>,
 Leon Romanovsky <leonro@...dia.com>
Subject: Re: [PATCH v10 03/24] iommu: generalize the batched sync after map
 interface

On 4/28/25 17:22, Leon Romanovsky wrote:
> From: Christoph Hellwig<hch@....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@....de>
> Acked-by: Will Deacon<will@...nel.org>
> Tested-by: Jens Axboe<axboe@...nel.dk>
> Reviewed-by: Jason Gunthorpe<jgg@...dia.com>
> Reviewed-by: Luis Chamberlain<mcgrof@...nel.org>
> Signed-off-by: Leon Romanovsky<leonro@...dia.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