iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Kevin Tian <kevin.tian@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Ashok Raj <ashok.raj@intel.com>,
	kvm@vger.kernel.org, Cornelia Huck <cohuck@redhat.com>,
	linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group()
Date: Thu, 10 Sep 2020 16:05:47 -0600	[thread overview]
Message-ID: <20200910160547.0a8b9891@w520.home> (raw)
In-Reply-To: <20200901033422.22249-3-baolu.lu@linux.intel.com>

On Tue,  1 Sep 2020 11:34:19 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> This adds two new APIs for the use cases like vfio/mdev where subdevices
> derived from physical devices are created and put in an iommu_group. The
> new IOMMU API interfaces mimic the vfio_mdev_at(de)tach_domain() directly,
> testing whether the resulting device supports IOMMU_DEV_FEAT_AUX and using
> an aux vs non-aux at(de)tach.
> 
> By doing this we could
> 
> - Set the iommu_group.domain. The iommu_group.domain is private to iommu
>   core (therefore vfio code cannot set it), but we need it set in order
>   for iommu_get_domain_for_dev() to work with a group attached to an aux
>   domain.
> 
> - Prefer to use the _attach_group() interfaces while the _attach_device()
>   interfaces are relegated to special cases.
> 
> Link: https://lore.kernel.org/linux-iommu/20200730134658.44c57a67@x1.home/
> Link: https://lore.kernel.org/linux-iommu/20200730151703.5daf8ad4@x1.home/
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 136 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/iommu.h |  20 +++++++
>  2 files changed, 156 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 38cdfeb887e1..fb21c2ff4861 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2757,6 +2757,142 @@ int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
>  
> +static int __iommu_aux_attach_device(struct iommu_domain *domain,
> +				     struct device *phys_dev,
> +				     struct device *sub_dev)
> +{
> +	int ret;
> +
> +	if (unlikely(!domain->ops->aux_attach_dev))
> +		return -ENODEV;
> +
> +	ret = domain->ops->aux_attach_dev(domain, phys_dev, sub_dev);
> +	if (!ret)
> +		trace_attach_device_to_domain(sub_dev);
> +
> +	return ret;
> +}
> +
> +static void __iommu_aux_detach_device(struct iommu_domain *domain,
> +				      struct device *phys_dev,
> +				      struct device *sub_dev)
> +{
> +	if (unlikely(!domain->ops->aux_detach_dev))
> +		return;
> +
> +	domain->ops->aux_detach_dev(domain, phys_dev, sub_dev);
> +	trace_detach_device_from_domain(sub_dev);
> +}
> +
> +static int __iommu_attach_subdev_group(struct iommu_domain *domain,
> +				       struct iommu_group *group,
> +				       iommu_device_lookup_t fn)
> +{
> +	struct group_device *device;
> +	struct device *phys_dev;
> +	int ret = -ENODEV;
> +
> +	list_for_each_entry(device, &group->devices, list) {
> +		phys_dev = fn(device->dev);
> +		if (!phys_dev) {
> +			ret = -ENODEV;
> +			break;
> +		}
> +
> +		if (iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
> +			ret = __iommu_aux_attach_device(domain, phys_dev,
> +							device->dev);
> +		else
> +			ret = __iommu_attach_device(domain, phys_dev);
> +
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
> +static void __iommu_detach_subdev_group(struct iommu_domain *domain,
> +					struct iommu_group *group,
> +					iommu_device_lookup_t fn)
> +{
> +	struct group_device *device;
> +	struct device *phys_dev;
> +
> +	list_for_each_entry(device, &group->devices, list) {
> +		phys_dev = fn(device->dev);
> +		if (!phys_dev)
> +			break;


Seems like this should be a continue rather than a break.  On the
unwind path maybe we're relying on holding the group mutex and
deterministic behavior from the fn() callback to unwind to the same
point, but we still have an entirely separate detach interface and I'm
not sure we couldn't end up with an inconsistent state if we don't
iterate each group device here.  Thanks,

Alex

> +
> +		if (iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
> +			__iommu_aux_detach_device(domain, phys_dev, device->dev);
> +		else
> +			__iommu_detach_device(domain, phys_dev);
> +	}
> +}
> +
> +/**
> + * iommu_attach_subdev_group - attach domain to an iommu_group which
> + *			       contains subdevices.
> + *
> + * @domain: domain
> + * @group:  iommu_group which contains subdevices
> + * @fn:     callback for each subdevice in the @iommu_group to retrieve the
> + *          physical device where the subdevice was created from.
> + *
> + * Returns 0 on success, or an error value.
> + */
> +int iommu_attach_subdev_group(struct iommu_domain *domain,
> +			      struct iommu_group *group,
> +			      iommu_device_lookup_t fn)
> +{
> +	int ret = -ENODEV;
> +
> +	mutex_lock(&group->mutex);
> +	if (group->domain) {
> +		ret = -EBUSY;
> +		goto unlock_out;
> +	}
> +
> +	ret = __iommu_attach_subdev_group(domain, group, fn);
> +	if (ret)
> +		__iommu_detach_subdev_group(domain, group, fn);
> +	else
> +		group->domain = domain;
> +
> +unlock_out:
> +	mutex_unlock(&group->mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_attach_subdev_group);
> +
> +/**
> + * iommu_detach_subdev_group - detach domain from an iommu_group which
> + *			       contains subdevices
> + *
> + * @domain: domain
> + * @group:  iommu_group which contains subdevices
> + * @fn:     callback for each subdevice in the @iommu_group to retrieve the
> + *          physical device where the subdevice was created from.
> + *
> + * The domain must have been attached to @group via iommu_attach_subdev_group().
> + */
> +void iommu_detach_subdev_group(struct iommu_domain *domain,
> +			       struct iommu_group *group,
> +			       iommu_device_lookup_t fn)
> +{
> +	mutex_lock(&group->mutex);
> +	if (!group->domain)
> +		goto unlock_out;
> +
> +	__iommu_detach_subdev_group(domain, group, fn);
> +	group->domain = NULL;
> +
> +unlock_out:
> +	mutex_unlock(&group->mutex);
> +}
> +EXPORT_SYMBOL_GPL(iommu_detach_subdev_group);
> +
>  /**
>   * iommu_sva_bind_device() - Bind a process address space to a device
>   * @dev: the device
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 871267104915..b9df8b510d4f 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -48,6 +48,7 @@ struct iommu_fault_event;
>  typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
>  			struct device *, unsigned long, int, void *);
>  typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault *, void *);
> +typedef struct device *(*iommu_device_lookup_t)(struct device *);
>  
>  struct iommu_domain_geometry {
>  	dma_addr_t aperture_start; /* First address that can be mapped    */
> @@ -631,6 +632,12 @@ bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
>  int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev);
>  void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev);
>  int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev);
> +int iommu_attach_subdev_group(struct iommu_domain *domain,
> +			      struct iommu_group *group,
> +			      iommu_device_lookup_t fn);
> +void iommu_detach_subdev_group(struct iommu_domain *domain,
> +			       struct iommu_group *group,
> +			       iommu_device_lookup_t fn);
>  
>  struct iommu_sva *iommu_sva_bind_device(struct device *dev,
>  					struct mm_struct *mm,
> @@ -1019,6 +1026,19 @@ iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
>  	return -ENODEV;
>  }
>  
> +static inline int
> +iommu_attach_subdev_group(struct iommu_domain *domain, struct iommu_group *group,
> +			  iommu_device_lookup_t fn)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline void
> +iommu_detach_subdev_group(struct iommu_domain *domain, struct iommu_group *group,
> +			  iommu_device_lookup_t fn)
> +{
> +}
> +
>  static inline struct iommu_sva *
>  iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
>  {

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply	other threads:[~2020-09-10 22:06 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-01  3:34 [PATCH v4 0/5] iommu aux-domain APIs extensions Lu Baolu
2020-09-01  3:34 ` [PATCH v4 1/5] iommu: Add optional subdev in aux_at(de)tach ops Lu Baolu
2020-09-10 22:05   ` Alex Williamson
2020-09-14  2:48     ` Lu Baolu
2020-09-01  3:34 ` [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group() Lu Baolu
2020-09-10 22:05   ` Alex Williamson [this message]
2020-09-14  3:02     ` Lu Baolu
2020-09-01  3:34 ` [PATCH v4 3/5] iommu: Add iommu_aux_get_domain_for_dev() Lu Baolu
2020-09-01  3:34 ` [PATCH v4 4/5] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
2020-09-01  3:34 ` [PATCH v4 5/5] iommu/vt-d: Add is_aux_domain support Lu Baolu
2020-09-10 22:05   ` Alex Williamson
2020-09-14  5:01     ` Lu Baolu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200910160547.0a8b9891@w520.home \
    --to=alex.williamson@redhat.com \
    --cc=ashok.raj@intel.com \
    --cc=baolu.lu@linux.intel.com \
    --cc=cohuck@redhat.com \
    --cc=dave.jiang@intel.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jean-philippe@linaro.org \
    --cc=kevin.tian@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robin.murphy@arm.com \
    --subject='Re: [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).