Re: [PATCH v2 04/11] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Sep 15, 2023 at 02:20:32PM +0530, Vasant Hegde wrote:

> >> +		dev_data = pasid_data->dev_data;
> >> +
> >> +		if ((start ^ (end - 1)) < PAGE_SIZE) {
> >> +			amd_iommu_flush_page(dev_data->domain,
> >> +					     pasid_data->pasid, start);
> >> +		} else {
> >> +			amd_iommu_flush_tlb(dev_data->domain,
> >> +					    pasid_data->pasid);
> >> +		}
> > 
> > It is baffling why dev_data->domain would be used here, and it isn't
> > locked properly so this is a problem.
> > 
> > Looking into __flush_pasid it doesn't really make sense. The PASID
> > domain definately can't assume that the RID domain has the the same
> > set of iommus. Most likely the SVA domain has a wider set of iommus,
> > at least I didn't notice any logic preventing this in set_dev_pasid.
> 
> Right. SVA protection domain will just add dev/PASID combination without
> worrying device protection domain/iommu.
>
> In invalidation path, I walk the `pasid_list` from SVA protection domain, get
> dev_data/PASID info and use those for invaliation. Since invalidation needs
> device protection domain I have to refer dev_data->domain.

See my other email, this area looks quite troubled.

> > Also, didn't you say you wanted to de-duplicate the IOTLB and ATC
> > invalidations like ARM is doing?
> 
> I don't have the understanding of ARM. But our invalidation
> interfaces needs improvement. Like current code does unncessary
> complete_wait() calls. Those will be fixed along with other
> invalidation cleanup.

Future fix is fine
 
> > 
> >> +static const struct mmu_notifier_ops sva_mn = {
> >> +	.invalidate_range = sva_mn_invalidate_range,
> >> +	.release = sva_mn_release,
> >> +};
> > 
> > Why an empty release function?
> > 
> > That can't be right, when release is called the driver has to stop
> > using iommu_virt_to_phys(domain->mm->pgd)) because it will be freed
> > memory. It should replace it with a global empty pgd and flush caches
> > before release returns.
> 
> In remove_dev_pasid() its already removed PASID from GCR3 table, flushing IOMMU.
> Also if its last device unbind, then it also does mmu_notifier_unregister().
>
> I thought remove_dev_pasid() always gets called before mmu notifier releases.
> Is there any chance of mmu notifier relase() getting called before
> remove_dev_pasid().

Yes it can be called in that order. An empty release function here is buggy.

> >> +	pasid_data->pasid = pasid;
> >> +	pasid_data->dev_data = dev_data;
> >> +
> >> +	/* Setup GCR3 table */
> >> +	ret = amd_iommu_set_gcr3(dev_data, pasid,
> >> +				 iommu_virt_to_phys(domain->mm->pgd));
> >> +	if (ret)
> >> +		goto out_free_pasid_data;
> >> +
> >> +	if (list_empty(&sva_pdom->pasid_list)) {
> >> +		sva_pdom->mn.ops = &sva_mn;
> >> +
> >> +		ret = mmu_notifier_register(&sva_pdom->mn, domain->mm);
> >> +		if (ret)
> >> +			goto out_clear_gcr3;
> >> +	}
> > 
> > Looks to me like mmu_notifier_register() should be done before
> > amd_iommu_set_gcr3() so we don't have a risk of stale iotlb data.
> 
> Actually we need to update GCR3 table with PASID before registering mmu
> notifier. So that hardware is ready to handle the invalidations. Otherwise we
> will have a redudant invalidations.

If you register the mmu and the paslid_list is empty (or locked) then the
invalidation handler does nothing. So this cannot be a concern.

Putting it out of order like this creates an edge case where the HW
could concivable cache a translation and miss a notification. It is
definately wrong.

> >> +void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> >> +{
> >> +	struct pdom_pasid_data *pasid_data;
> >> +	struct protection_domain *sva_pdom;
> >> +	struct iommu_domain *domain;
> >> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> >> +	unsigned long flags;
> >> +
> >> +	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
> >> +		return;
> >> +
> >> +	/* Get protection domain */
> >> +	domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
> >> +	if (!domain)
> >> +		return;
> >> +	sva_pdom = to_pdomain(domain);
> >> +
> >> +	/* Ensure that all queued faults have been processed */
> >> +	iopf_queue_flush_dev(dev);
> >> +
> >> +	spin_lock_irqsave(&sva_pdom->lock, flags);
> >> +
> >> +	pasid_data = get_pdom_pasid_data(sva_pdom, dev, pasid);
> >> +	if (!pasid_data) {
> >> +		spin_unlock_irqrestore(&sva_pdom->lock, flags);
> >> +		return;
> >> +	}
> >> +
> >> +	list_del(&pasid_data->pdom_link);
> >> +	kfree(pasid_data);
> >> +
> >> +	/* make it visible */
> >> +	smp_wmb();
> >> +
> >> +	/* Update GCR3 table and flush IOTLB */
> >> +	amd_iommu_clear_gcr3(dev_data, pasid);
> >> +
> >> +	spin_unlock_irqrestore(&sva_pdom->lock, flags);
> >> +
> >> +	if (list_empty(&sva_pdom->pasid_list))
> >> +		mmu_notifier_unregister(&sva_pdom->mn, domain->mm);
> > 
> > Can't drop the spinlock before doing this, it is racy.
> 
> Yeah. Its racy. But we cannot call mmu_notifier_unregister() with spinlock held.
> May be I will introduce some lock to handle notifier.

If you reoganize things so the notifier is always registered then you
don't need to have the spinlock protecting it anymore. this requires
putting the unregister in the domain deallocation routine.

Jason