Re: [PATCH v5 3/5] iommu: Add verisilicon IOMMU driver

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

Le 09/07/2025 à 18:45, Jason Gunthorpe a écrit :
> On Wed, Jul 09, 2025 at 10:53:28AM +0200, Benjamin Gaignard wrote:
>> +static int vsi_iommu_attach_device(struct iommu_domain *domain,
>> +				   struct device *dev)
>> +{
>> +	struct vsi_iommu *iommu = dev_iommu_priv_get(dev);
>> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	ret = pm_runtime_resume_and_get(iommu->dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	spin_lock_irqsave(&iommu->lock, flags);
>> +	/* iommu already attached */
>> +	if (iommu->domain == domain)
>> +		goto unlock;
>> +
>> +	vsi_iommu_enable(iommu, domain);
>> +	list_add_tail(&iommu->node, &vsi_domain->iommus);
>> +	iommu->domain = domain;
>> +
>> +unlock:
>> +	spin_unlock_irqrestore(&iommu->lock, flags);
>> +	pm_runtime_put_autosuspend(iommu->dev);
>> +	return ret;
> I thought this was mentioned before, but this doesn't handle
> attach_device being called twice without an identity attach in
> between.
>
> And now the new locking doesn't protect concurrent invalidation races,
> the lock is in the wrong place.
>
> hold the domain lock across the whole sequence to hold off any
> invalidation until the linked list is consistent with the HW
> programming:
>
>   spin_lock_irqsave(&vsi_domain->lock, flags2); // Prevent invalidation
>
>   vsi_iommu_enable(iommu, domain);
>   list_del(&iommu->node);
>   list_add_tail(&iommu->node, &vsi_domain->iommus);
>
>   spin_unlock_irqrestore(&vsi_domain->lock, flags);
>
> Then remove this:
>
>   +	/* iommu already attached */
>   +	if (iommu->domain == domain)
>   +		goto unlock;
>
> Since the fix above makes it safe regardless.
>
> And, also feels like again, but vsi_iommu_enable() needs to fully
> flush the cache since the translation is being changed, shouldn't
> there also be writes to VSI_MMU_FLUSH_BASE ?
>
> Otherwise the locking change looks OK and I don't have other comments.

I have fix that in v6.

Thanks a lot for your review.

Benjamin

>
> Jason