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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Feb 06, 2024 at 10:46:58PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 2/2/2024 8:55 PM, Jason Gunthorpe wrote:
> > 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
> 
> One time for IOMMU capability (amd_iommu_pasid_supported()) and one time for
> device capability.

Again I think this whole thing is out of sequence. The main focus
should be on the gcr3 table. You should dirctly know if it has been
installed or not via some direct means. Test all this stuff when you
go to install it the first time.

> is_pasid_enabled() is checked twice (once without lock, so that we can avoid
> lock in most cases and one inside lock to be sure no one else entered and
> enabled gcr3).

That never works, don't do that.

> 
> > 
> >> +		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
> 
> We need other checks to make sure both IOMMU and device is capable of PASID.
> Hence its a separate function.

That should be done at probe time and cached in the per-device max
pasid valid. It is 0 if there is no pasid support.

> > 2) Precompute the max pasids during device probe and store it in
> > iommu_dev_data. Just check if PASID >= max_pasid and fail
> 
> This is not required .. as set_dev_pasid() can directly check
> dev->iommu->max_pasids.

That is not the same thing, dev->iommu->max_pasids is the PCI/DT
capability only.

The driver still has to keep its own internal limit.

It is goofy and we should probably merge the driver limit and the core
limit at some point - until then it is better to follow the pattern
and keep a driver limit in the driver, doing all the work at probe
time not during attach.

> >> +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.
> 
> This is used in remove_dev_pasid and sva_mn_release path.
> 
> This just clears GCR3[pasid] entry. Its not updating DTE.

Same argument, it should not be iterating, there is only ever one.

The release path is not removing the PASID, it is just disabling the
GCR3 entry. The domain is still logically connected to that PASID as
far as everything else is concerned.

> > 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.
> 
> That's after reworking protection domain structure. Not in this series.

:( I wish you'd get this stuff cleaned up properly first instead of
building more mess on top of the wrong design.
 
> >> +	/* 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.
> 
> Ok. I can move that up and then have error handler path to remove it.

Because, like this shows, it is hard to get it all right and it is
even harder when everything is not consistent and there are two
versions of the same flows.

Jason