Re: [PATCH v2 04/11] iommu/amd: Initial SVA support for AMD IOMMU

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,

On 9/12/2023 10:17 PM, Jason Gunthorpe wrote:
> On Mon, Sep 11, 2023 at 12:10:39PM +0000, Vasant Hegde wrote:
> 
>> +static inline struct protection_domain *amd_iommu_get_pdomain(struct device *dev)
>> +{
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> +
>> +	if (!dev_data || !dev_data->domain)
>> +		return NULL;
>> +
>> +	return dev_data->domain;
>> +}
> 
> Not used in this patch

Yeah. Forgot to move to different patch after reworking SVA code. Will fix it.

> 
>> @@ -2192,6 +2188,11 @@ static int protection_domain_init_v2(struct protection_domain *pdom)
>>  	return 0;
>>  }
>>  
>> +static const struct iommu_domain_ops amd_svm_domain_ops = {
>> +	.set_dev_pasid = amd_iommu_set_dev_pasid,
>> +	.free	       = amd_iommu_domain_free
>> +};
> 
> "amd_sva_domain_ops" please

Fixed.

> 
>> new file mode 100644
>> index 000000000000..d18dc3f676b9
>> --- /dev/null
>> +++ b/drivers/iommu/amd/sva.c
>> @@ -0,0 +1,158 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#define pr_fmt(fmt)     "AMD-Vi: " fmt
>> +#define dev_fmt(fmt)    pr_fmt(fmt)
>> +
>> +#include <linux/iommu.h>
>> +#include <linux/mm_types.h>
>> +#include <linux/mmu_notifier.h>
>> +
>> +#include "amd_iommu.h"
>> +
>> +static void sva_mn_invalidate_range(struct mmu_notifier *mn,
>> +				    struct mm_struct *mm,
>> +				    unsigned long start, unsigned long end)
>> +{
>> +	struct protection_domain *sva_pdom;
>> +	struct pdom_pasid_data *pasid_data;
>> +	struct iommu_dev_data *dev_data;
>> +
>> +	sva_pdom = container_of(mn, struct protection_domain, mn);
>> +
>> +	list_for_each_entry(pasid_data, &sva_pdom->pasid_list, pdom_link) {
> 
> Need to hold sva_pdom->lock if iterating over pasid_list
> 
>> +		dev_data = pasid_data->dev_data;
>> +
>> +		if ((start ^ (end - 1)) < PAGE_SIZE) {
>> +			amd_iommu_flush_page(dev_data->domain,
>> +					     pasid_data->pasid, start);
>> +		} else {
>> +			amd_iommu_flush_tlb(dev_data->domain,
>> +					    pasid_data->pasid);
>> +		}
> 
> It is baffling why dev_data->domain would be used here, and it isn't
> locked properly so this is a problem.
> 
> Looking into __flush_pasid it doesn't really make sense. The PASID
> domain definately can't assume that the RID domain has the the same
> set of iommus. Most likely the SVA domain has a wider set of iommus,
> at least I didn't notice any logic preventing this in set_dev_pasid.

Right. SVA protection domain will just add dev/PASID combination without
worrying device protection domain/iommu.

In invalidation path, I walk the `pasid_list` from SVA protection domain, get
dev_data/PASID info and use those for invaliation. Since invalidation needs
device protection domain I have to refer dev_data->domain.

> 
> Also, didn't you say you wanted to de-duplicate the IOTLB and ATC
> invalidations like ARM is doing?

I don't have the understanding of ARM. But our invalidation interfaces needs
improvement. Like current code does unncessary complete_wait() calls. Those will
be fixed along with other invalidation cleanup.

> 
>> +static const struct mmu_notifier_ops sva_mn = {
>> +	.invalidate_range = sva_mn_invalidate_range,
>> +	.release = sva_mn_release,
>> +};
> 
> Why an empty release function?
> 
> That can't be right, when release is called the driver has to stop
> using iommu_virt_to_phys(domain->mm->pgd)) because it will be freed
> memory. It should replace it with a global empty pgd and flush caches
> before release returns.

In remove_dev_pasid() its already removed PASID from GCR3 table, flushing IOMMU.
Also if its last device unbind, then it also does mmu_notifier_unregister().

I thought remove_dev_pasid() always gets called before mmu notifier releases.
Is there any chance of mmu notifier relase() getting called before
remove_dev_pasid().

> 
>> +int amd_iommu_set_dev_pasid(struct iommu_domain *domain,
>> +			    struct device *dev, ioasid_t pasid)
>> +{
>> +	struct protection_domain *sva_pdom = to_pdomain(domain);
> 
> Just call this pdom, there is nothing about this function that is sva
> specific anymore.

Ok.

> 
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> +	struct pdom_pasid_data *pasid_data;
>> +	int ret = -EINVAL;
>> +	unsigned long flags;
>> +
>> +	/* PASID zero is used for requests from the I/O device without PASID */
>> +	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
>> +		return ret;
>> +
>> +	/* Use SVA protection domain lock */
>> +	spin_lock_irqsave(&sva_pdom->lock, flags);
>> +
>> +	/* Add PASID to protection domain pasid list */
>> +	pasid_data = kzalloc(sizeof(*pasid_data), GFP_KERNEL);
>> +	if (pasid_data == NULL) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	pasid_data->pasid = pasid;
>> +	pasid_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)
>> +		goto out_free_pasid_data;
>> +
>> +	if (list_empty(&sva_pdom->pasid_list)) {
>> +		sva_pdom->mn.ops = &sva_mn;
>> +
>> +		ret = mmu_notifier_register(&sva_pdom->mn, domain->mm);
>> +		if (ret)
>> +			goto out_clear_gcr3;
>> +	}
> 
> Looks to me like mmu_notifier_register() should be done before
> amd_iommu_set_gcr3() so we don't have a risk of stale iotlb data.

Actually we need to update GCR3 table with PASID before registering mmu
notifier. So that hardware is ready to handle the invalidations. Otherwise we
will have a redudant invalidations.


> 
>> +
>> +	list_add(&pasid_data->pdom_link, &sva_pdom->pasid_list);
>> +	spin_unlock_irqrestore(&sva_pdom->lock, flags);
>> +	return ret;
>> +
>> +out_clear_gcr3:
>> +	amd_iommu_clear_gcr3(dev_data, pasid);
>> +
>> +out_free_pasid_data:
>> +	kfree(pasid_data);
>> +
>> +out:
>> +	spin_unlock_irqrestore(&sva_pdom->lock, flags);
>> +	return ret;
>> +}
> 
> But this looks fine, it is entirely generic.
> 
>> +static struct pdom_pasid_data *get_pdom_pasid_data(struct protection_domain *pdom,
>> +						   struct device *dev, ioasid_t pasid)
>> +{
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> +	struct pdom_pasid_data *pasid_data;
>> +
>> +	list_for_each_entry(pasid_data, &pdom->pasid_list, pdom_link)
>> {
> 
> Add a lockdep assert for pdom->lock here

Ok.

> 
>> +		if (pasid_data->pasid == pasid &&
>> +		    pasid_data->dev_data == dev_data)
>> +			return pasid_data;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>> +{
>> +	struct pdom_pasid_data *pasid_data;
>> +	struct protection_domain *sva_pdom;
>> +	struct iommu_domain *domain;
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> +	unsigned long flags;
>> +
>> +	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
>> +		return;
>> +
>> +	/* Get protection domain */
>> +	domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
>> +	if (!domain)
>> +		return;
>> +	sva_pdom = to_pdomain(domain);
>> +
>> +	/* Ensure that all queued faults have been processed */
>> +	iopf_queue_flush_dev(dev);
>> +
>> +	spin_lock_irqsave(&sva_pdom->lock, flags);
>> +
>> +	pasid_data = get_pdom_pasid_data(sva_pdom, dev, pasid);
>> +	if (!pasid_data) {
>> +		spin_unlock_irqrestore(&sva_pdom->lock, flags);
>> +		return;
>> +	}
>> +
>> +	list_del(&pasid_data->pdom_link);
>> +	kfree(pasid_data);
>> +
>> +	/* make it visible */
>> +	smp_wmb();
>> +
>> +	/* Update GCR3 table and flush IOTLB */
>> +	amd_iommu_clear_gcr3(dev_data, pasid);
>> +
>> +	spin_unlock_irqrestore(&sva_pdom->lock, flags);
>> +
>> +	if (list_empty(&sva_pdom->pasid_list))
>> +		mmu_notifier_unregister(&sva_pdom->mn, domain->mm);
> 
> Can't drop the spinlock before doing this, it is racy.

Yeah. Its racy. But we cannot call mmu_notifier_unregister() with spinlock held.
May be I will introduce some lock to handle notifier.

-Vasant