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

[Jean-Philippe Brucker <jean-philippe.brucker@arm.com>]

On 26/09/2018 23:35, Jacob Pan wrote:
> On Thu, 20 Sep 2018 18:00:39 +0100
> Jean-Philippe Brucker <jean-philippe.brucker@arm.com> 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?

Thanks,
Jean