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

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 24/03/2026 à 16:46, Will Deacon a écrit :
> 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.

I know you expect the hardware to work like that but that isn't not the case.
I spend quite long to try to found hidden bit(s) or an other way to do like
you want but I can't find any solution.
As you mention the downstream code suggest that the iommu can't invalidate
TLB in unmap routine so I don't see how to progress.
Maybe we should just admit that is how the hardware work.
This v13 has fixed the documentation so I don't plan to spend more time on this driver.

Benjamin

>
> Will