Re: [PATCH v4 02/17] iommu: Add device-centric DMA ownership interfaces

Eric Auger
2022-11-14T13:34:51Z

Hi,
On 11/8/22 01:48, Jason Gunthorpe wrote:
> From: Lu Baolu <baolu.lu@linux.intel.com>
>
> These complement the group interfaces used by VFIO and are for use by
> iommufd. The main difference is that multiple devices in the same group
> can all share the ownership by passing the same ownership pointer.
>
> Move the common code into shared functions.
>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/iommu/iommu.c | 124 +++++++++++++++++++++++++++++++++---------
> include/linux/iommu.h | 12 ++++
> 2 files changed, 110 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 6ca377f4fbf9e9..4cb14e44e40f83 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3108,41 +3108,52 @@ static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
> return 0;
> }
>
> +static int __iommu_take_dma_ownership(struct iommu_group *group, void *owner)
> +{
> + int ret;
> +
> + if (WARN_ON(!owner))
> + return -EINVAL;
> +
> + if ((group->domain && group->domain != group->default_domain) ||
> + !xa_empty(&group->pasid_array))
> + return -EBUSY;
> +
> + ret = __iommu_group_alloc_blocking_domain(group);
> + if (ret)
> + return ret;
> + ret = __iommu_group_set_domain(group, group->blocking_domain);
> + if (ret)
> + return ret;
> +
> + group->owner = owner;
> + group->owner_cnt++;
> + return 0;
> +}
> +
> /**
> * iommu_group_claim_dma_owner() - Set DMA ownership of a group
> * @group: The group.
> * @owner: Caller specified pointer. Used for exclusive ownership.
> *
> - * This is to support backward compatibility for vfio which manages
> - * the dma ownership in iommu_group level. New invocations on this
> - * interface should be prohibited.
> + * This is to support backward compatibility for vfio which manages the dma
> + * ownership in iommu_group level. New invocations on this interface should be
> + * prohibited. Only a single owner may exist for a group.
> */
> int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner)
> {
> int ret = 0;
>
> + if (WARN_ON(!owner))
> + return -EINVAL;
> +
> mutex_lock(&group->mutex);
> if (group->owner_cnt) {
> ret = -EPERM;
> goto unlock_out;
> - } else {
> - if ((group->domain && group->domain != group->default_domain) ||
> - !xa_empty(&group->pasid_array)) {
> - ret = -EBUSY;
> - goto unlock_out;
> - }
> -
> - ret = __iommu_group_alloc_blocking_domain(group);
> - if (ret)
> - goto unlock_out;
> -
> - ret = __iommu_group_set_domain(group, group->blocking_domain);
> - if (ret)
> - goto unlock_out;
> - group->owner = owner;
> }
>
> - group->owner_cnt++;
> + ret = __iommu_take_dma_ownership(group, owner);
> unlock_out:
> mutex_unlock(&group->mutex);
>
> @@ -3151,30 +3162,91 @@ int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner)
> EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner);
>
> /**
> - * iommu_group_release_dma_owner() - Release DMA ownership of a group
> - * @group: The group.
> + * iommu_device_claim_dma_owner() - Set DMA ownership of a device
> + * @dev: The device.
> + * @owner: Caller specified pointer. Used for exclusive ownership.
> *
> - * Release the DMA ownership claimed by iommu_group_claim_dma_owner().
> + * Claim the DMA ownership of a device. Multiple devices in the same group may
> + * concurrently claim ownership if they present the same owner value. Returns 0
> + * on success and error code on failure
> */
> -void iommu_group_release_dma_owner(struct iommu_group *group)
> +int iommu_device_claim_dma_owner(struct device *dev, void *owner)
> {
> - int ret;
> + struct iommu_group *group = iommu_group_get(dev);
> + int ret = 0;
> +
> + if (!group)
> + return -ENODEV;
>
> mutex_lock(&group->mutex);
> + if (group->owner_cnt) {
> + if (group->owner != owner) {
> + ret = -EPERM;
> + goto unlock_out;
> + }
> + group->owner_cnt++;
> + goto unlock_out;
> + }
> +
> + ret = __iommu_take_dma_ownership(group, owner);
> +unlock_out:
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner);
> +
> +static void __iommu_release_dma_ownership(struct iommu_group *group)
> +{
> + int ret;
> +
> if (WARN_ON(!group->owner_cnt || !group->owner ||
> !xa_empty(&group->pasid_array)))
> - goto unlock_out;
> + return;
>
> group->owner_cnt = 0;
> group->owner = NULL;
> ret = __iommu_group_set_domain(group, group->default_domain);
> WARN(ret, "iommu driver failed to attach the default domain");
> +}
>
> -unlock_out:
> +/**
> + * iommu_group_release_dma_owner() - Release DMA ownership of a group
> + * @group: The group.
> + *
> + * Release the DMA ownership claimed by iommu_group_claim_dma_owner().
> + */
> +void iommu_group_release_dma_owner(struct iommu_group *group)
> +{
> + mutex_lock(&group->mutex);
> + __iommu_release_dma_ownership(group);
> mutex_unlock(&group->mutex);
> }
> EXPORT_SYMBOL_GPL(iommu_group_release_dma_owner);
>
> +/**
> + * iommu_device_release_dma_owner() - Release DMA ownership of a device
> + * @group: The device.
@dev: the device
> + *
> + * Release the DMA ownership claimed by iommu_device_claim_dma_owner().
> + */
> +void iommu_device_release_dma_owner(struct device *dev)
> +{
> + struct iommu_group *group = iommu_group_get(dev);
> +
> + mutex_lock(&group->mutex);
> + if (group->owner_cnt > 1) {
> + group->owner_cnt--;
> + goto unlock_out;
> + }
> + __iommu_release_dma_ownership(group);
> +unlock_out:
> + mutex_unlock(&group->mutex);

if (group->owner_cnt > 1)

group->owner_cnt--;
else
__iommu_release_dma_ownership(group);

mutex_unlock(&group->mutex);

iommu_group_put(group);

> + iommu_group_put(group);
> +}
> +EXPORT_SYMBOL_GPL(iommu_device_release_dma_owner);
> +
> /**
> * iommu_group_dma_owner_claimed() - Query group dma ownership status
> * @group: The group.
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a09fd32d8cc273..1690c334e51631 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -707,6 +707,9 @@ int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner);
> void iommu_group_release_dma_owner(struct iommu_group *group);
> bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>
> +int iommu_device_claim_dma_owner(struct device *dev, void *owner);
> +void iommu_device_release_dma_owner(struct device *dev);
> +
> struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
> struct mm_struct *mm);
> int iommu_attach_device_pasid(struct iommu_domain *domain,
> @@ -1064,6 +1067,15 @@ static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
> return false;
> }
>
> +static inline void iommu_device_release_dma_owner(struct device *dev)
> +{
> +}
> +
> +static inline int iommu_device_claim_dma_owner(struct device *dev, void *owner)
> +{
> + return -ENODEV;
> +}
> +
> static inline struct iommu_domain *
> iommu_sva_domain_alloc(struct device *dev, struct mm_struct *mm)
> {
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric