Re: [PATCH v6 01/16] iommu/dma: Prepare MSI physical address lists

[Robin Murphy <robin.murphy@arm.com>]

On 25/09/2026 4:16 pm, Andrew Jones wrote:
> Software MSI mappings may cover an ordered list of physical addresses
> which must be mapped into one contiguous IOVA range. Extend the internal
> DMA-IOMMU mapping helpers to accept an address array, address count, and
> required mapping granule.
> 
> Teach iommu_dma_get_msi_page() to map the list into one contiguous IOVA
> allocation. Keep the existing per-page cache entries and mark the first
> entry with the size of the allocation so an identical list can reuse the
> mapping without changing the existing cache representation.
> 
> This prepares for the forthcoming iommu_dma_prepare_msi_list() API.

Sorry, but this looks pretty bonkers, even before we get to patch #8. 
You already end up adding what is effectively a RISC-V-specific 
entrypoint, so you may as well just carry that all the way through to 
its own effectively RISC-V-specific implementation, without making an 
unmaintainable mess of the existing code.

The current design for both DMA_IOVA and DMA_MSI cookies is based on the 
(Arm-centric) notion that different devices may be associated with 
different MSI controllers that are independent of each other, so we only 
map what we know we need (and have a way to get the address of at all), 
and the list lookup is to save redundant mappings and IOVA space when 
devices do happen to share. Your requirement is almost the complete 
opposite, where *any* device needs *every* possible MSI controller 
mapped up-front, plus the MSI controller driver has to be in on this 
notion too, so it really doesn't fit the same logic well at all. In fact 
IIUC it should be far simpler - you shouldn't need a list, nor even 
really care about the addresses, it should merely be a case of whether 
a) this is the first call for the given cookie so everything needs 
mapping, or b) it's not the first call, so everything must already be 
mapped and we can just return the IOVA. If trying to cram these opposing 
notions down the same path results in a bunch of weird complexity for 
pretending to support distinct and overlapping values of "everything", 
which neither case needs, that seems like a pretty clear sign of it 
being a bad idea IMO.

Note that since the core cookie rework, untangling the DMA_MSI 
implementation from DMA_IOVA has been on the table, at which point 
adding more top-level types of MSI-only cookies would clearly be 
straightforward. However for DMA_IOVA that could end up getting a bit 
combinatorial, and keeping an internal sub-type (like for flush queues) 
would probably be simpler, so it may well make sense to take the latter 
approach for both, at least to start with. But getting rid of those odd 
special cases in iommu_dma_{alloc,free}_iova() and the clunkiness of 
cookie_msi_{granule,pages}() would still be nice either way...

At very worst, a separate hook to just pre-populate msi_page_list with a 
regular page for each IMSIC address, such that the existing reuse 
mechanism keeps working as-is, could probably suffice without any other 
major structural changes; it's only really the IOVA allocation and the 
fact that it all has to be done under a single lock acquisition that's 
special.

Thanks,
Robin.

> Signed-off-by: Andrew Jones <andrew.jones@oss.qualcomm.com>
> Tested-by: Fangyu Yu <fangyu.yu@linux.alibaba.com>
> ---
>   drivers/iommu/dma-iommu.c | 116 +++++++++++++++++++++++++++++---------
>   1 file changed, 88 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 58c624513cd4..0b4eb47d1a95 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -42,6 +42,7 @@ struct iommu_dma_msi_page {
>   	struct list_head	list;
>   	dma_addr_t		iova;
>   	phys_addr_t		phys;
> +	size_t			range_size; /* IOVA range size, or 0 if not the first map */
>   };
>   
>   enum iommu_dma_queue_type {
> @@ -481,7 +482,7 @@ static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
>   	num_pages = iova_align(iovad, end - start) >> iova_shift(iovad);
>   
>   	for (i = 0; i < num_pages; i++) {
> -		msi_page = kmalloc_obj(*msi_page);
> +		msi_page = kzalloc_obj(*msi_page);
>   		if (!msi_page)
>   			return -ENOMEM;
>   
> @@ -2191,15 +2192,42 @@ static struct list_head *cookie_msi_pages(const struct iommu_domain *domain)
>   	}
>   }
>   
> +static bool iommu_dma_msi_range_matches(struct list_head *msi_page_list,
> +					const struct iommu_dma_msi_page *base_page,
> +					const phys_addr_t *phys_addrs,
> +					unsigned int nr_addrs, size_t granule)
> +{
> +	const struct iommu_dma_msi_page *msi_page;
> +	unsigned int nr_found = 0;
> +	dma_addr_t offset;
> +
> +	list_for_each_entry(msi_page, msi_page_list, list) {
> +		if (msi_page->iova < base_page->iova)
> +			continue;
> +		offset = msi_page->iova - base_page->iova;
> +		if (offset >= base_page->range_size)
> +			continue;
> +		if (!IS_ALIGNED((size_t)offset, granule) ||
> +		    msi_page->phys != phys_addrs[(size_t)offset / granule])
> +			return false;
> +		nr_found++;
> +	}
> +
> +	return nr_found == nr_addrs;
> +}
> +
>   static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
> -		phys_addr_t msi_addr, struct iommu_domain *domain)
> +		const phys_addr_t *phys_addrs, unsigned int nr_addrs, size_t granule,
> +		struct iommu_domain *domain)
>   {
>   	struct list_head *msi_page_list = cookie_msi_pages(domain);
> -	struct iommu_dma_msi_page *msi_page;
> -	dma_addr_t iova;
> +	struct iommu_dma_msi_page *msi_page, *first_page = NULL;
>   	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> -	size_t size = cookie_msi_granule(domain);
>   	static DEFINE_MUTEX(msi_prepare_lock);
> +	LIST_HEAD(new_msi_pages);
> +	dma_addr_t base_iova;
> +	unsigned int i;
> +	size_t size;
>   
>   	/*
>   	 * Normally a device's default domain is only ever attached to that
> @@ -2213,32 +2241,60 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
>   	 */
>   	guard(mutex)(&msi_prepare_lock);
>   
> -	msi_addr &= ~(phys_addr_t)(size - 1);
> -	list_for_each_entry(msi_page, msi_page_list, list)
> -		if (msi_page->phys == msi_addr)
> +	if (!nr_addrs || nr_addrs > SIZE_MAX / granule)
> +		return NULL;
> +	size = nr_addrs * granule;
> +
> +	list_for_each_entry(msi_page, msi_page_list, list) {
> +		if (msi_page->phys != phys_addrs[0])
> +			continue;
> +		if (nr_addrs == 1)
> +			return msi_page;
> +		if (msi_page->range_size == size &&
> +		    iommu_dma_msi_range_matches(msi_page_list, msi_page, phys_addrs,
> +						nr_addrs, granule))
>   			return msi_page;
> +	}
>   
> -	msi_page = kzalloc_obj(*msi_page);
> -	if (!msi_page)
> -		return NULL;
> +	for (i = 0; i < nr_addrs; i++) {
> +		msi_page = kzalloc_obj(*msi_page);
> +		if (!msi_page)
> +			goto out_free_pages;
> +		list_add_tail(&msi_page->list, &new_msi_pages);
> +	}
>   
> -	iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> -	if (!iova)
> -		goto out_free_page;
> +	base_iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> +	if (!base_iova)
> +		goto out_free_pages;
>   
> -	if (iommu_map(domain, iova, msi_addr, size, prot, GFP_KERNEL))
> -		goto out_free_iova;
> +	i = 0;
> +	list_for_each_entry(msi_page, &new_msi_pages, list) {
> +		msi_page->phys = phys_addrs[i];
> +		msi_page->iova = base_iova + i * granule;
> +		if (!i)
> +			first_page = msi_page;
> +		if (iommu_map(domain, msi_page->iova, msi_page->phys, granule, prot, GFP_KERNEL))
> +			goto out_unmap;
> +		i++;
> +	}
>   
> -	INIT_LIST_HEAD(&msi_page->list);
> -	msi_page->phys = msi_addr;
> -	msi_page->iova = iova;
> -	list_add(&msi_page->list, msi_page_list);
> -	return msi_page;
> +	first_page->range_size = size;
> +	list_splice(&new_msi_pages, msi_page_list);
> +	return first_page;
>   
> -out_free_iova:
> -	iommu_dma_free_iova(domain, iova, size, NULL);
> -out_free_page:
> -	kfree(msi_page);
> +out_unmap:
> +	if (i) {
> +		size_t unmap = iommu_unmap(domain, base_iova, i * granule);
> +
> +		WARN_ON_ONCE(unmap != i * granule);
> +	}
> +	iommu_dma_free_iova(domain, base_iova, size, NULL);
> +out_free_pages:
> +	while (!list_empty(&new_msi_pages)) {
> +		msi_page = list_first_entry(&new_msi_pages, typeof(*msi_page), list);
> +		list_del(&msi_page->list);
> +		kfree(msi_page);
> +	}
>   	return NULL;
>   }
>   
> @@ -2247,19 +2303,23 @@ int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
>   {
>   	struct device *dev = msi_desc_to_dev(desc);
>   	const struct iommu_dma_msi_page *msi_page;
> +	phys_addr_t phys_addr;
> +	size_t granule;
>   
>   	if (!has_msi_cookie(domain)) {
>   		msi_desc_set_iommu_msi_iova(desc, 0, 0);
>   		return 0;
>   	}
>   
> +	granule = cookie_msi_granule(domain);
> +	phys_addr = ALIGN_DOWN(msi_addr, granule);
> +
>   	iommu_group_mutex_assert(dev);
> -	msi_page = iommu_dma_get_msi_page(dev, msi_addr, domain);
> +	msi_page = iommu_dma_get_msi_page(dev, &phys_addr, 1, granule, domain);
>   	if (!msi_page)
>   		return -ENOMEM;
>   
> -	msi_desc_set_iommu_msi_iova(desc, msi_page->iova,
> -				    ilog2(cookie_msi_granule(domain)));
> +	msi_desc_set_iommu_msi_iova(desc, msi_page->iova, ilog2(granule));
>   	return 0;
>   }
>