Re: [PATCH v5 12/14] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Jan 18, 2024 at 07:33:37AM +0000, Vasant Hegde wrote:

> +static int iommu_pasid_enable(struct iommu_dev_data *dev_data)
> +{
> +	struct device *dev = dev_data->dev;
> +	int ret = 0;
> +
> +	spin_lock(&dev_data->lock);
> +
> +	if (is_pasid_enabled(dev_data))
> +		goto out;
> +
> +	if (!amd_iommu_pasid_supported()) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	/* attach_device path enables device PASID feature */
> +	if (!dev_data->pasid_enabled) {

How many times are we testing for this? Just check if the gcr3 table
is installed once

> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = amd_iommu_gcr3_init(dev_data, dev->iommu->max_pasids);
> +
> +out:
> +	spin_unlock(&dev_data->lock);
> +	return ret;
> +}

This seems like too much, and it doesn't need to be in a function..

1) Check directly if the gcr3 table is installed otherwise try to
install it. That might be a usefull function

2) Precompute the max pasids during device probe and store it in
iommu_dev_data. Just check if PASID >= max_pasid and fail

> +static void iommu_pasid_disable(struct iommu_dev_data *dev_data)
> +{
> +	spin_lock(&dev_data->lock);
> +
> +	if (!is_gcr3_table_empty(dev_data))
> +		goto out;

Caller already checked this

> +
> +	if (dev_data->gcr3_info.gcr3_tbl == NULL)
> +		goto out;

Can't happen, right?

> +
> +	amd_iommu_gcr3_uninit(dev_data);

Just write it out clearly in remove dev pasid

if (is_gcr3_table_empty(dev_data))
   amd_iommu_gcr3_uninit(dev_data);

> +static int iommu_setup_pasid_pri(struct iommu_dev_data *dev_data)
> +{
> +	struct pci_dev *pdev;
> +	int ret;
> +
> +	if (is_pasid_enabled(dev_data))
> +		return 0;
> +
> +	ret = iommu_pasid_enable(dev_data);
> +	if (ret)
> +		return ret;
> +
> +	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
> +	if (!pdev || !amd_iommu_pdev_pri_supported(pdev))
> +		return 0;
> +
> +	if (!dev_data->pri_enabled)
> +		return -EINVAL;
> +
> +	ret = amd_iommu_iopf_enable_device(dev_data->dev);
> +
> +	return ret;
> +}
> +
> +static void remove_dev_pasid(struct pdom_dev_data *pdom_dev_data)
> +{
> +	/* Update GCR3 table and flush IOTLB */
> +	amd_iommu_clear_gcr3(pdom_dev_data->dev_data, pdom_dev_data->pasid);

This is in the wrong place, there is only one DTE/GCR3 remove_dev is
touching. The list iteration below is just to clean up the tracking
list it should not touch HW. Move this up into
amd_iommu_remove_dev_pasid.

And these functions related to the tracking list should be more
general and called in more places. Just the tracking list itself
should have a few patches to create it and situate it generically in
the driver. Even the RID should be using the same mechanism.

> +int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
> +			    struct device *dev, ioasid_t pasid)
> +{
> +	struct pdom_dev_data *pdom_dev_data;
> +	struct protection_domain *sva_pdom = to_pdomain(domain);
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +	unsigned long flags;
> +	int ret = -EINVAL;
> +
> +	/* PASID zero is used for requests from the I/O device without PASID */
> +	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
> +		return ret;
> +
> +	/* Make sure PASID/PRI is enabled */
> +	ret = iommu_setup_pasid_pri(dev_data);
> +	if (ret)
> +		return ret;
> +
> +	/* Add PASID to protection domain pasid list */
> +	pdom_dev_data = kzalloc(sizeof(*pdom_dev_data), GFP_KERNEL);
> +	if (pdom_dev_data == NULL)
> +		return ret;
> +
> +	pdom_dev_data->pasid = pasid;
> +	pdom_dev_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) {
> +		kfree(pdom_dev_data);
> +		return ret;
> +	}
> +
> +	spin_lock_irqsave(&sva_pdom->lock, flags);
> +	list_add(&pdom_dev_data->list, &sva_pdom->dev_data_list);
> +	spin_unlock_irqrestore(&sva_pdom->lock, flags);

This doesn't seem right? The tracking list should be loaded before any
change is made visible to the HW, or at least under the same lock.

Jason