Re: [PATCH v3 05/12] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Oct 16, 2023 at 10:43:44AM +0000, Vasant Hegde wrote:
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index da94dca1eb92..20961353460d 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -8,7 +8,9 @@
>  #ifndef _ASM_X86_AMD_IOMMU_TYPES_H
>  #define _ASM_X86_AMD_IOMMU_TYPES_H
>  
> +#include <linux/iommu.h>
>  #include <linux/types.h>
> +#include <linux/mmu_notifier.h>
>  #include <linux/mutex.h>
>  #include <linux/msi.h>
>  #include <linux/list.h>
> @@ -541,6 +543,16 @@ enum protection_domain_mode {
>  	PD_MODE_V2,
>  };
>  
> +/* Track PASID list for the protection domain */
> +struct pdom_pasid_data {
> +	/* PASID attached to the protection domain */
> +	ioasid_t pasid;
> +	/* Points to attached device data */
> +	struct iommu_dev_data *dev_data;
> +	/* Link to protection domain */
> +	struct list_head pdom_link;
> +};
> +
>  /*
>   * This structure contains generic data for  IOMMU protection domains
>   * independent of their use.
> @@ -556,6 +568,9 @@ struct protection_domain {
>  	enum protection_domain_mode pd_mode; /* Track page table type */
>  	unsigned dev_cnt;	/* devices assigned to this domain */
>  	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
> +
> +	struct mmu_notifier mn;	/* mmu notifier for the SVA domain */
> +	struct list_head pasid_list; /* List of pdom_pasid_data */
>  };

Oh, see here is the data structure I mentioned earlier, it just needs
to be done as part of the invalidation cleanup and replace the other
things I mentioned. Has nothing to do with SVA.

> @@ -2432,6 +2428,8 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
>  	case IOMMU_DOMAIN_UNMANAGED:
>  		pgtable = AMD_IOMMU_V1;
>  		break;
> +	case IOMMU_DOMAIN_SVA:
> +		return amd_iommu_sva_domain_alloc(domain);

Same remark as I gave intel, you should take my patch (strip the ARM
parts) to give this it's own op which significantly cleans up this
flow.

https://lore.kernel.org/linux-iommu/20-v2-16665a652079+5947-smmuv3_newapi_p2_jgg@nvidia.com/

> +static void dev_pasid_remove(struct pdom_pasid_data *pasid_data)
> +{
> +	/* make it visible */
> +	smp_wmb();

What is "it"? Don't put barriers like this, the barrier should
immediately follow whatever store it is protecting.

> +static struct pdom_pasid_data *get_pdom_pasid_data(struct protection_domain *pdom,
> +					   struct device *dev, ioasid_t pasid)
> +{
> +	struct pdom_pasid_data *pasid_data;
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +

Add a lockdep assertion, but it seems strange a function like this
would even exist.. Better to call it "remove_pdom_pasid_data" and
include the list_del(). And then pull the locking into here too.

> +static void sva_arch_invalidate_secondary_tlbs(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;
> +	unsigned long flags;
> +
> +	sva_pdom = container_of(mn, struct protection_domain, mn);
> +
> +	spin_lock_irqsave(&sva_pdom->lock, flags);
> +
> +	list_for_each_entry(pasid_data, &sva_pdom->pasid_list, pdom_link) {
> +		dev_data = pasid_data->dev_data;
> +
> +		spin_lock(&dev_data->lock);
> +		amd_iommu_dev_flush_pasid_pages(dev_data, pasid_data->pasid,
> +						start, end - start);

Nope, can't unlock while still holding the pointer. Use a mutex or a
rcu scheme. Again this should all be general non-SVA code to fix the
entire invalidation logic.

> +static void sva_mn_release(struct mmu_notifier *mn, struct mm_struct *mm)
> +{
> +	struct pdom_pasid_data *pasid_data, *next;
> +	struct protection_domain *sva_pdom;
> +	unsigned long flags;
> +
> +	sva_pdom = container_of(mn, struct protection_domain, mn);
> +
> +	spin_lock_irqsave(&sva_pdom->lock, flags);
> +
> +	/* Assume pasid_list contains same PASID with different devices */
> +	list_for_each_entry_safe(pasid_data, next,
> +				 &sva_pdom->pasid_list, pdom_link) {
> +		dev_pasid_remove(pasid_data);
> +	}

Be mindful of any logging in other parts of the driver.. Ideally a
non-present PASID translation will generate a debugging log but a
released SVA will not.

> +static int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
> +				   struct device *dev, ioasid_t pasid)
> +{
> +	struct pdom_pasid_data *pasid_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;
> +
> +	/* 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);

GFP_KERNEL under a spinlock - did you test this with full debugging?

> +	if (pasid_data == NULL) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	pasid_data->pasid = pasid;
> +	pasid_data->dev_data = dev_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) {
> +			sva_pdom->mn.ops = NULL;
> +			goto out_free_pasid_data;
> +		}
> +	}
> +
> +	/* Setup GCR3 table */
> +	ret = amd_iommu_set_gcr3(dev_data, pasid,
> +				 iommu_virt_to_phys(domain->mm->pgd));
> +	if (ret)
> +		goto out_unreg_notifier;

Something looks missing, what if the RID domain hasn't installed a
GCR3? Eg it is a v1 domain or something?

> +	list_add(&pasid_data->pdom_link, &sva_pdom->pasid_list);
> +	spin_unlock_irqrestore(&sva_pdom->lock, flags);
> +	return ret;
> +
> +out_unreg_notifier:
> +	mmu_notifier_unregister(&sva_pdom->mn, domain->mm);
> +	sva_pdom->mn.ops = NULL;

I'm pretty sure you can't actually unregister while holding the
spinlock without creating a deadlock.

Take my SVA patch and move notifier registration to alloc/free and out
of attach to fix this.

Jason