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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Feb 07, 2024 at 03:01:22PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 2/6/2024 11:04 PM, Jason Gunthorpe wrote:
> > 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.
> 
> We are doing 1 SVA domain : 1 PASID : N devices model. That means we need to
> check all these things while attaching *first* PASID to device. (That's what we
> had discussed sometime back when I had all these things in feature_enable(SVA)
> path).

I thought I said you 1 PASID is not technically correct, but you could
get away it it for a short term. You should be planning to support N
PASIDs because that is what the API defines.

> Ex: If device is in domain with V1 page table tries to attach PASID it should fail
>    If device is in domain of PT mode, then we should setup gcr3.

Yes
 
> So I don't see how we can infer these things unless we have something like
> enable_feature(PASID).. which would have taken care of setting up things.

You just trivially check these conditions at the top of set_dev_pasid, look at
what I did for ARM:

	if (smmu_domain->smmu != master->smmu || pasid == IOMMU_NO_PASID)
		return -EINVAL;

	if (!master->cd_table.in_ste &&
	    sid_domain->type != IOMMU_DOMAIN_IDENTITY &&
	    sid_domain->type != IOMMU_DOMAIN_BLOCKED)
		return -EINVAL;

For AMD "cd_table in_ste" means that the DTE points to a GCR3 table
already. This is trivially tracked in the DTE programming in set_dte
because you know if the dte is being configured with a GCR3 or not.

The other cases represent the situations where we know how to upgrade a DTE
for those domains to one with a GCR3. Everything else is blocked.

> >> 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.
> 
> It does work as second one is lock protected. And the intention was to avoid
> locking for attaching second PASID onwards. Only for first time attaching PASID
> to device we will have check for two times.

You can get racy false negatives, that are hard to reason about.

 CPU0                               CPU 1
                                    lock()
                                    change state to !a
if (!a)
  forget it
                                    a = false
                                    unlock()
lock()
if (!a)
   forget it
change state to a
a = true
unlock()

There are ways with atomics you can build those kinds of lockless
patterns, but they are tricky and not justified here.

Jason