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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Sep 11, 2023 at 12:10:46PM +0000, Vasant Hegde wrote:
> diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
> index 2891f67a6ca0..86c0cb836a71 100644
> --- a/drivers/iommu/amd/ppr.c
> +++ b/drivers/iommu/amd/ppr.c
> @@ -9,6 +9,7 @@
>  #include <linux/amd-iommu.h>
>  #include <linux/delay.h>
>  #include <linux/mmu_notifier.h>
> +#include <linux/pci-ats.h>
>  
>  #include "amd_iommu.h"
>  #include "amd_iommu_types.h"
> @@ -306,3 +307,58 @@ int amd_iommu_iopf_remove_device(struct amd_iommu *iommu, struct device *dev)
>  	raw_spin_unlock_irqrestore(&iommu->lock, flags);
>  	return ret;
>  }
> +
> +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..

And the iopf is a device wide thing, it makes no sense that some
random pdom->lock is protecting dev_dev->ppr..

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)

Jason