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