Re: [PATCH v13 3/6] iommu: Add verisilicon IOMMU driver

[Will Deacon <will@kernel.org>]

On Mon, Feb 16, 2026 at 10:51:35AM +0100, Benjamin Gaignard wrote:
> The Verisilicon IOMMU hardware block can be found in combination
> with Verisilicon hardware video codecs (encoders or decoders) on
> different SoCs.
> Enable it will allow us to use non contiguous memory allocators
> for Verisilicon video codecs.
> If both decoder and this iommu driver are compiled has modules
> there is undefined symboles issues so this iommu driver could
> only be compiled has built-in.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  MAINTAINERS               |   8 +
>  drivers/iommu/Kconfig     |  11 +
>  drivers/iommu/Makefile    |   1 +
>  drivers/iommu/vsi-iommu.c | 794 ++++++++++++++++++++++++++++++++++++++
>  include/linux/vsi-iommu.h |  21 +
>  5 files changed, 835 insertions(+)
>  create mode 100644 drivers/iommu/vsi-iommu.c
>  create mode 100644 include/linux/vsi-iommu.h

[...]

> +static size_t vsi_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
> +			      size_t size, size_t count, struct iommu_iotlb_gather *gather)
> +{
> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
> +	dma_addr_t pte_dma, iova = (dma_addr_t)_iova;
> +	unsigned long flags;
> +	phys_addr_t pt_phys;
> +	u32 dte;
> +	u32 *pte_addr;
> +	size_t unmap_size = 0;
> +
> +	spin_lock_irqsave(&vsi_domain->lock, flags);
> +
> +	dte = vsi_domain->dt[vsi_iova_dte_index(iova)];
> +	/* Just return 0 if iova is unmapped */
> +	if (!vsi_dte_is_pt_valid(dte))
> +		goto unlock;
> +
> +	pt_phys = vsi_dte_pt_address(dte);
> +	pte_addr = (u32 *)phys_to_virt(pt_phys) + vsi_iova_pte_index(iova);
> +	pte_dma = pt_phys + vsi_iova_pte_index(iova) * sizeof(u32);
> +	unmap_size = vsi_iommu_unmap_iova(vsi_domain, pte_addr, pte_dma, size);
> +
> +unlock:
> +	spin_unlock_irqrestore(&vsi_domain->lock, flags);
> +
> +	return unmap_size;
> +}

I still think you need TLB invalidation here.

I looked at the downstream code that you linked to and it litters the
invalidation in the callers via mpp_iommu_flush_tlb(), which tend to
invalidate _before_ starting an operation. That's very likely buggy and
certainly not something we want upstream.

The unmap routine should do the invalidation so that, when it returns,
the pages really are unmapped from the device (assuming strict mode).

I know you said that you tried to add invalidation here and it "didn't
work", but that's not something I can really help you with.

Will