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

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

Le 25/03/2026 à 17:36, Will Deacon a écrit :
> On Tue, Mar 24, 2026 at 05:28:44PM +0100, Benjamin Gaignard wrote:
>> 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.
> The hardware appears to have a register to invalidate the entire TLB.
> We can use that if there's nothing else.

VSI_MMU_BIT_FLUSH ? it discards everything.
Is there an api to call it when all buffers have been unmapped ?

>
>> 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.
> Then we can invalidate the entire TLB.
>
>> 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.
> The downstream code is a tangled mess; I don't think it suggests anything
> about what the IOMMU hardware is capable of.

If you have an other source to tell the hardware capabilities, I will be
more than happy to read it and fix the driver.

Benjamin

>
>> Maybe we should just admit that is how the hardware work.
> No.
>
> The upstream kernel isn't a dumping ground for vendor crap. The hardware
> has TLB invalidation functionality and so we should use it. If we don't,
> then we're not giving the IOMMU API what it expects and any callers
> outside of the video codecs will be landed with problems when unmap
> doesn't work as expected.
>
>> This v13 has fixed the documentation so I don't plan to spend more time on this driver.
> That's a shame, I'm really not asking for much.
>
> Will
>