Re: [PATCH v4 01/10] iommu: Introduce a replace API for device pasid

[Yi Liu <yi.l.liu@intel.com>]

On 2024/9/13 10:44, Baolu Lu wrote:
> On 9/12/24 9:12 PM, Yi Liu wrote:
>> Provide a high-level API to allow replacements of one domain with
>> another for specific pasid of a device. This is similar to
>> iommu_group_replace_domain() and it is expected to be used only by
>> IOMMUFD.
>>
>> Co-developed-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommu-priv.h |  4 ++
>>   drivers/iommu/iommu.c      | 90 ++++++++++++++++++++++++++++++++++++--
>>   2 files changed, 90 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
>> index de5b54eaa8bf..90b367de267e 100644
>> --- a/drivers/iommu/iommu-priv.h
>> +++ b/drivers/iommu/iommu-priv.h
>> @@ -27,6 +27,10 @@ static inline const struct iommu_ops 
>> *iommu_fwspec_ops(struct iommu_fwspec *fwsp
>>   int iommu_group_replace_domain(struct iommu_group *group,
>>                      struct iommu_domain *new_domain);
>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>> +                   struct device *dev, ioasid_t pasid,
>> +                   struct iommu_attach_handle *handle);
>> +
>>   int iommu_device_register_bus(struct iommu_device *iommu,
>>                     const struct iommu_ops *ops,
>>                     const struct bus_type *bus,
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index b6b44b184004..066f659018a5 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3347,14 +3347,15 @@ static void iommu_remove_dev_pasid(struct device 
>> *dev, ioasid_t pasid,
>>   }
>>   static int __iommu_set_group_pasid(struct iommu_domain *domain,
>> -                   struct iommu_group *group, ioasid_t pasid)
>> +                   struct iommu_group *group, ioasid_t pasid,
>> +                   struct iommu_domain *old)
>>   {
>>       struct group_device *device, *last_gdev;
>>       int ret;
>>       for_each_group_device(group, device) {
>>           ret = domain->ops->set_dev_pasid(domain, device->dev,
>> -                         pasid, NULL);
>> +                         pasid, old);
>>           if (ret)
>>               goto err_revert;
>>       }
>> @@ -3366,7 +3367,20 @@ static int __iommu_set_group_pasid(struct 
>> iommu_domain *domain,
>>       for_each_group_device(group, device) {
>>           if (device == last_gdev)
>>               break;
>> -        iommu_remove_dev_pasid(device->dev, pasid, domain);
>> +        /* If no old domain, undo the succeeded devices/pasid */
>> +        if (!old) {
>> +            iommu_remove_dev_pasid(device->dev, pasid, domain);
>> +            continue;
>> +        }
>> +
>> +        /*
>> +         * Rollback the succeeded devices/pasid to the old domain.
>> +         * And it is a driver bug to fail attaching with a previously
>> +         * good domain.
>> +         */
>> +        if (WARN_ON(old->ops->set_dev_pasid(old, device->dev,
>> +                            pasid, domain)))
>> +            iommu_remove_dev_pasid(device->dev, pasid, domain);
> 
> You want to rollback to the 'old' domain, right? So, %s/domain/old/ ?

this will be invoked if the rollback failed. Since the set_dev_pasid op
would keep the 'old' configure, so at this point, the 'old' domain is 'domain'.

>>       }
>>       return ret;
>>   }
>> @@ -3425,7 +3439,7 @@ int iommu_attach_device_pasid(struct iommu_domain 
>> *domain,
>>       if (ret)
>>           goto out_unlock;
>> -    ret = __iommu_set_group_pasid(domain, group, pasid);
>> +    ret = __iommu_set_group_pasid(domain, group, pasid, NULL);
>>       if (ret)
>>           xa_erase(&group->pasid_array, pasid);
>>   out_unlock:
>> @@ -3434,6 +3448,74 @@ int iommu_attach_device_pasid(struct iommu_domain 
>> *domain,
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
>> +/**
>> + * iommu_replace_device_pasid - Replace the domain that a pasid is 
>> attached to
>> + * @domain: the new iommu domain
>> + * @dev: the attached device.
>> + * @pasid: the pasid of the device.
>> + * @handle: the attach handle.
>> + *
>> + * This API allows the pasid to switch domains. Return 0 on success, or an
>> + * error. The pasid will keep the old configuration if replacement failed.
>> + * This is supposed to be used by iommufd, and iommufd can guarantee that
>> + * both iommu_attach_device_pasid() and iommu_replace_device_pasid() would
>> + * pass in a valid @handle.
>> + */
>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>> +                   struct device *dev, ioasid_t pasid,
>> +                   struct iommu_attach_handle *handle)
> 
> How about passing the old domain as a parameter?

I suppose it was agreed in the below link.

https://lore.kernel.org/linux-iommu/20240816124707.GZ2032816@nvidia.com/

>> +{
>> +    /* Caller must be a probed driver on dev */
>> +    struct iommu_group *group = dev->iommu_group;
>> +    struct iommu_attach_handle *curr;
>> +    int ret;
>> +
>> +    if (!domain->ops->set_dev_pasid)
>> +        return -EOPNOTSUPP;
>> +
>> +    if (!group)
>> +        return -ENODEV;
>> +
>> +    if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
>> +        pasid == IOMMU_NO_PASID || !handle)
> 
> dev_has_iommu() check is duplicate with above if (!group) check.

I was just referring to the iommu_attach_device_pasid(). So both the two
path could drop the dev_has_iommu() check, is it?

> By the way, why do you require a non-NULL attach handle? In the current
> design, attach handles are only used for domains with iopf capability.

yeah, but it looks fine to always pass in an attach handle. The iopf
path would require hwpt->domain->iopf_handler.

>> +        return -EINVAL;
>> +
>> +    handle->domain = domain;
>> +
>> +    mutex_lock(&group->mutex);
>> +    /*
>> +     * The iommu_attach_handle of the pasid becomes inconsistent with the
>> +     * actual handle per the below operation. The concurrent PRI path will
>> +     * deliver the PRQs per the new handle, this does not have a function
>> +     * impact. The PRI path would eventually become consistent when the
>> +     * replacement is done.
>> +     */
>> +    curr = (struct iommu_attach_handle *)xa_store(&group->pasid_array,
>> +                              pasid, handle,
>> +                              GFP_KERNEL);
>> +    if (!curr) {
>> +        xa_erase(&group->pasid_array, pasid);
>> +        ret = -EINVAL;
>> +        goto out_unlock;
>> +    }
> 
> This seems to be broken as explained above. The attach handle is
> currently only for iopf-capable domains.

if attach handle is always passed, then this is not broken. is it?

> If I understand it correctly, you just want the previous attached domain
> here, right? If so, why not just passing it to this helper from callers?

yeah, I'm open about it. :) @Jason, your opinion?

>> +
>> +    ret = xa_err(curr);
>> +    if (ret)
>> +        goto out_unlock;
>> +
>> +    if (curr->domain == domain)
>> +        goto out_unlock;
>> +
>> +    ret = __iommu_set_group_pasid(domain, group, pasid, curr->domain);
>> +    if (ret)
>> +        WARN_ON(handle != xa_store(&group->pasid_array, pasid,
>> +                       curr, GFP_KERNEL));
>> +out_unlock:
>> +    mutex_unlock(&group->mutex);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid, IOMMUFD_INTERNAL);
>> +
>>   /*
>>    * iommu_detach_device_pasid() - Detach the domain from pasid of device
>>    * @domain: the iommu domain.

-- 
Regards,
Yi Liu