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

[Jason Gunthorpe <jgg@ziepe.ca>]

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

> @@ -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

> 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.

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

> +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.

> +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.

> +	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.

> +
> +	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

> +		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.

It would be best if the mmu notifier was registered at domain
allocation time and freed at domain destruction

Jason