Re: [PATCH v2 11/11] iommu/amd: Introduce logic to enable/disable IOPF

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Sep 15, 2023 at 01:56:54PM +0530, Vasant Hegde wrote:

> >> +static int amd_iommu_iopf_update(struct device *dev, bool enable)
> >> +{
> >> +	unsigned long flags;
> >> +	int ret;
> >> +	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
> >> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> >> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> >> +	struct protection_domain *pdom = amd_iommu_get_pdomain(dev);
> >> +
> >> +	if (!pdev || !iommu || !dev_data)
> >> +		return -EINVAL;
> >> +
> >> +	spin_lock_irqsave(&pdom->lock, flags);
> >> +
> >> +	if (enable) {
> >> +		ret = amd_iommu_iopf_add_device(iommu, dev);
> >> +		if (ret)
> >> +			goto out;
> >> +
> >> +		dev_data->ppr = true;
> >> +	} else {
> >> +		ret = amd_iommu_iopf_remove_device(iommu, dev);
> >> +		dev_data->ppr = false;
> >> +	}
> >> +
> >> +	amd_iommu_domain_update(pdom);
> >> +
> >> +out:
> >> +	spin_unlock_irqrestore(&pdom->lock, flags);
> >> +	return ret;
> >> +}
> > 
> > Same remarks as for the SVA, this is in the wrong place.
> > 
> > iopf_queue_add_device() should be done when a PRI enabled domain is
> > attached to a device, not in feature things. I also want to remove
> > these too once ARM is fixed..
> 
> You mean we should do this during first device/pasid bind time?

Yes, when a PRI capable domain is first attached the PRI stuff should
be setup. SVA is a PRI capable domain type, but we will have more..

> > amd_iommu_get_pdomain() has improper locking, it needs to hold some
> > kind of dev_data lock to access dev_data->domain. (and really this
> > would all be clearer if it was just written dev_data->domain instead
> > of the redundant function)
> 
> It helps to get iommu from device structure. Its useful and we want to use it
> other places as well.

As I said, it seems obfuscating as to how the locking needs to work as
you must hold a lock while using dev_data->domain. In some cases that
can be the core code's group mutex, but that doesn't always work..

Jason