Re: [PATCH v4 1/2] iommu: Add iommu_for_each_pci_mmio_window() iterator

[Jason Gunthorpe]

On Sun, Oct 25, 2026 at 09:29:59PM +0800, Guanghui Feng wrote:
> Add a callback-based iterator that walks a PCI device's host bridge
> MMIO windows and invokes a caller-supplied function with each window's
> bus address range.  This consolidates the bridge window enumeration
> logic into a single shared algorithm in the IOMMU core.
> 
> Refactor iova_reserve_pci_windows() to use the new iterator, replacing
> its open-coded resource_list_for_each_entry loop.  The callback approach
> avoids intermediate allocations: the DMA path's callback directly calls
> reserve_iova() with the computed PFN range, maintaining the same
> zero-allocation overhead as the original open-coded loop.
> 
> Using a callback rather than returning a linked list of iommu_resv_region
> entries eliminates size_t truncation risk on 32-bit PAE systems (the
> callback receives phys_addr_t start/end directly) and removes the need
> for allocation failure cleanup paths.
> 
> Note: commit cd2c9fcf5c66 ("iommu/dma: Move PCI window region reservation back into dma specific path.")
> intentionally keeps PCI window reservation out of the IOMMU group-level
> reserved region API.  This iterator respects that boundary: it does not
> inject windows into iommu_get_group_resv_regions().
> 
> Signed-off-by: Guanghui Feng <guanghuifeng@xxxxxxxxxxxxxxxxx>
> ---
>  drivers/iommu/dma-iommu.c  | 29 ++++++++++++++++------
>  drivers/iommu/iommu-priv.h | 25 +++++++++++++++++++
>  drivers/iommu/iommu.c      | 51 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 97 insertions(+), 8 deletions(-)

Seems OK enough

> @@ -504,6 +505,20 @@ static int iommu_dma_ranges_sort(void *priv, const struct list_head *a,
>  	return res_a->res->start > res_b->res->start;
>  }
>  
> +struct iova_pci_resv_data {
> +	struct iova_domain *iovad;
> +};

We don't need a struct to pass a single pointer

> +struct pci_dev;
> +
> +/**
> + * iommu_pci_mmio_window_fn_t - callback for host bridge MMIO window iteration
> + * @start: First PCI bus address of the window (inclusive)
> + * @end: Last PCI bus address of the window (inclusive)
> + * @data: Opaque pointer from iommu_for_each_pci_mmio_window() caller
> + *
> + * Return: 0 to continue iteration, negative errno to stop and propagate.
> + */
> +typedef int (*iommu_pci_mmio_window_fn_t)(phys_addr_t start, phys_addr_t end,
> +					  void *data);

Don't really need a comment here

> +#ifdef CONFIG_PCI
> +/**
> + * iommu_for_each_pci_mmio_window - iterate over PCI host bridge MMIO windows
> + * @dev: PCI device whose host bridge to scan
> + * @fn: callback invoked with each window's bus address range
> + * @data: opaque pointer passed to @fn
> + *
> + * Walks the MEM windows of @dev's PCI host bridge and invokes
> + * @fn(start, end, data) for each one, where start and end are PCI bus
> + * addresses (inclusive).  Iteration stops early if @fn returns non-zero,
> + * and that value is propagated to the caller.
> + *
> + * This provides a zero-allocation way for both the DMA IOVA layer and
> + * iommufd to enumerate the same host bridge window information using a
> + * shared algorithm.
> + *
> + * Note: commit cd2c9fcf5c66 ("iommu/dma: Move PCI window region reservation back into dma specific path.")
> + * intentionally keeps PCI window reservation out of the IOMMU group-level
> + * reserved region API.  This iterator respects that boundary: it does not
> + * inject windows into iommu_get_group_resv_regions().

The last two pargraphs are not very useful

> + *
> + * Return: 0 on success, negative errno on failure (including from @fn).
> + */
> +int iommu_for_each_pci_mmio_window(struct pci_dev *dev,
> +				   iommu_pci_mmio_window_fn_t fn, void *data)
> +{
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
> +	struct resource_entry *window;
> +
> +	if (!bridge)
> +		return -ENODEV;

Can it fail? The existing code doesn't check it so I guess not. Stick
a WARN_ON there if you want to keep it.

> +
> +	resource_list_for_each_entry(window, &bridge->windows) {
> +		phys_addr_t start, end;
> +		int ret;
> +
> +		if (resource_type(window->res) != IORESOURCE_MEM)
> +			continue;
> +
> +		start = window->res->start - window->offset;
> +		end = window->res->end - window->offset;
> +
> +		ret = fn(start, end, data);

Don't think we need the start/end variable either

Jason