Re: [PATCH v11 3/7] iommu: Add verisilicon IOMMU driver

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

Le 14/01/2026 à 13:59, Will Deacon a écrit :
> On Tue, Jan 13, 2026 at 05:25:38PM +0100, Benjamin Gaignard wrote:
>> Le 13/01/2026 à 17:10, Will Deacon a écrit :
>>> Hi Benjamin,
>>>
>>> Thanks for posting a v11.
>>>
>>> On Wed, Jan 07, 2026 at 11:09:53AM +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>
>>>> ---
>>>> changes in version 11:
>>>> - Fix dependency issue when decoder driver is build as module.
>>>>
>>>>    drivers/iommu/Kconfig     |  11 +
>>>>    drivers/iommu/Makefile    |   1 +
>>>>    drivers/iommu/vsi-iommu.c | 808 ++++++++++++++++++++++++++++++++++++++
>>>>    include/linux/vsi-iommu.h |  21 +
>>>>    4 files changed, 841 insertions(+)
>>>>    create mode 100644 drivers/iommu/vsi-iommu.c
>>>>    create mode 100644 include/linux/vsi-iommu.h
>>> Based on your reply to v9:
>>>
>>> https://lore.kernel.org/all/0eff8b1a-c45f-47b1-a871-59f4a0101f0f@collabora.com/
>>>
>>> I took another look at this to see whether it had changed significantly
>>> from v6 when compared to the rockchip driver. Sadly, they still look
>>> very similar to me and I continue to suspect that the hardware is a
>>> derivative. I really don't understand why having a shared implementation
>>> of the default domain ops is difficult or controversial. Have you tried
>>> to write it?
>>>
>>> However, given that nobody from the Rockchip side has contributed to the
>>> discussion and you claim that this is a distinct piece of IP, I don't
>>> want to block the merging of the driver by leaving the conversation
>>> hanging.
>>>
>>> There is still one thing I don't understand (which, amusingly, the
>>> rockchip driver doesn't seem to suffer from):
>>>
>>>> +static void vsi_iommu_flush_tlb_all(struct iommu_domain *domain)
>>>> +{
>>>> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
>>>> +	struct list_head *pos;
>>>> +	unsigned long flags;
>>>> +
>>>> +	spin_lock_irqsave(&vsi_domain->lock, flags);
>>>> +
>>>> +	list_for_each(pos, &vsi_domain->iommus) {
>>>> +		struct vsi_iommu *iommu;
>>>> +		int ret;
>>>> +
>>>> +		iommu = list_entry(pos, struct vsi_iommu, node);
>>>> +		ret = pm_runtime_resume_and_get(iommu->dev);
>>>> +		if (ret < 0)
>>>> +			continue;
>>>> +
>>>> +		spin_lock(&iommu->lock);
>>>> +
>>>> +		writel(VSI_MMU_BIT_FLUSH, iommu->regs + VSI_MMU_FLUSH_BASE);
>>>> +		writel(0, iommu->regs + VSI_MMU_FLUSH_BASE);
>>>> +
>>>> +		spin_unlock(&iommu->lock);
>>>> +		pm_runtime_put_autosuspend(iommu->dev);
>>>> +	}
>>>> +
>>>> +	spin_unlock_irqrestore(&vsi_domain->lock, flags);
>>>> +}
>>> [...]
>>>
>>>> +static const struct iommu_ops vsi_iommu_ops = {
>>>> +	.identity_domain = &vsi_identity_domain,
>>>> +	.release_domain = &vsi_identity_domain,
>>>> +	.domain_alloc_paging = vsi_iommu_domain_alloc_paging,
>>>> +	.of_xlate = vsi_iommu_of_xlate,
>>>> +	.probe_device = vsi_iommu_probe_device,
>>>> +	.release_device = vsi_iommu_release_device,
>>>> +	.device_group = generic_single_device_group,
>>>> +	.owner = THIS_MODULE,
>>>> +	.default_domain_ops = &(const struct iommu_domain_ops) {
>>>> +		.attach_dev		= vsi_iommu_attach_device,
>>>> +		.map_pages		= vsi_iommu_map,
>>>> +		.unmap_pages		= vsi_iommu_unmap,
>>>> +		.flush_iotlb_all	= vsi_iommu_flush_tlb_all,
>>> This has no callers and so your unmap routine appears to be broken.
>> It is a leftover of previous attempt to allow video decoder to clean/flush
>> the iommu by using a function from the API.
>> Now it is using vsi_iommu_restore_ctx().
>> I while remove it in version 12.
> Don't you still need some invalidation on the unmap path?

In vsi_iommu_unmap_iova() page is invalided by calling vsi_mk_pte_invalid().
That clear BIT(0) so the hardware knows the page is invalid.
Do I have miss something here ?

Benjamin

>
> Will
>