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

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

On 11/7/2023 4:48 AM, Jason Gunthorpe wrote:
> 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/

Ok. I have grabbed above patch (minus arm stuff) and added as separate patch in
my series.

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

Fixed.

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


Ok. I have renamed these function names.

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

Ok.

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

Fixed.

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

I do have check in next patch. Anyway I have re-arranged patches and its all
part of single patch now.

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

mmu_notifier is moved to domain alloc/free.

-Vasant