Re: [PATCH v3 03/10] iommu/sva: Manage process address spaces

[Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Wed, 3 Oct 2018 18:52:16 +0100
Jean-Philippe Brucker <jean-philippe.brucker-5wv7dgnIgG8@public.gmane.org> wrote:

> On 26/09/2018 23:35, Jacob Pan wrote:
> > On Thu, 20 Sep 2018 18:00:39 +0100
> > Jean-Philippe Brucker <jean-philippe.brucker-5wv7dgnIgG8@public.gmane.org> wrote:
> >   
> >> +
> >> +static int io_mm_attach(struct iommu_domain *domain, struct device
> >> *dev,
> >> +			struct io_mm *io_mm, void *drvdata)
> >> +{
> >> +	int ret;
> >> +	bool attach_domain = true;
> >> +	int pasid = io_mm->pasid;
> >> +	struct iommu_bond *bond, *tmp;
> >> +	struct iommu_sva_param *param =
> >> dev->iommu_param->sva_param; +
> >> +	if (!domain->ops->mm_attach || !domain->ops->mm_detach)
> >> +		return -ENODEV;
> >> +
> >> +	if (pasid > param->max_pasid || pasid < param->min_pasid)
> >> +		return -ERANGE;
> >> +
> >> +	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
> >> +	if (!bond)
> >> +		return -ENOMEM;
> >> +
> >> +	bond->domain		= domain;
> >> +	bond->io_mm		= io_mm;
> >> +	bond->dev		= dev;
> >> +	bond->drvdata		= drvdata;
> >> +
> >> +	spin_lock(&iommu_sva_lock);
> >> +	/*
> >> +	 * Check if this io_mm is already bound to the domain. In
> >> which case the
> >> +	 * IOMMU driver doesn't have to install the PASID table
> >> entry.
> >> +	 */
> >> +	list_for_each_entry(tmp, &domain->mm_list, domain_head) {
> >> +		if (tmp->io_mm == io_mm) {
> >> +			attach_domain = false;
> >> +			break;
> >> +		}
> >> +	}
> >> +
> >> +	ret = domain->ops->mm_attach(domain, dev, io_mm,
> >> attach_domain);
> >> +	if (ret) {
> >> +		kfree(bond);
> >> +		goto out_unlock;
> >> +	}
> >> +
> >> +	list_add(&bond->mm_head, &io_mm->devices);
> >> +	list_add(&bond->domain_head, &domain->mm_list);
> >> +	list_add(&bond->dev_head, &param->mm_list);
> >> +  
> > 
> > I am trying to understand if mm_list is needed for both per device
> > and per domain. Do you always unbind and detach domain? Seems
> > device could use the domain->mm_list to track all mm's, true?  
> 
> We need to track bonds per devices, since the bind/unbind() user
> interface in on devices. Tracking per domain is just a helper, so
> IOMMU drivers that have a single PASID table per domain know when
> they need to install a new entry (the attach_domain parameter above)
> and remove it. I think my code is wrong here: if binding two devices
> that are in the same domain to the same process we shouldn't add the
> io_mm to domain->mm_list twice.
> 
> I'm still not sure if I should remove domains handling here though,
> could you confirm if you're planning to support
> iommu_get_domain_for_dev for vt-d?
> 
yes. i am working on getting vt-d onto the same behavior in terms of
default domain. I have a patch being tested, we need to respect RMRR (
reserved region) that is setup before iommu_get_domain_for_dev().
> Thanks,
> Jean

[Jacob Pan]