[Intel-gfx] [PATCH v5 08/10] iommufd: Add iommufd_ctx_has_group()

[Liu, Yi L]

> From: Alex Williamson <alex.williamson at redhat.com>
> Sent: Thursday, May 18, 2023 3:40 AM
> 
> On Sat, 13 May 2023 06:21:34 -0700
> Yi Liu <yi.l.liu at intel.com> wrote:
> 
> > to check if any device within the given iommu_group has been bound with
> 
> Nit, I find these commit logs where the subject line is intended to
> flow into the commit log to form a complete sentence difficult to read.
> I expect complete thoughts within the commit log itself and the subject
> should be a separate summary of the log.  Repeating the subject within
> the commit log is ok.

Sure. I'll go through the commit messages.

> 
> > the iommufd_ctx. This helpful for the checking on device ownership for
> 
> s/This/This is/
> 
> > the devices which have been bound but cannot be bound to any other iommufd
> 
> s/have been/have not been/?
> 
> > as the iommu_group has been bound.
> >
> > Signed-off-by: Yi Liu <yi.l.liu at intel.com>
> > ---
> >  drivers/iommu/iommufd/device.c | 29 +++++++++++++++++++++++++++++
> >  include/linux/iommufd.h        |  8 ++++++++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> > index 81466b97023f..5e5f7912807b 100644
> > --- a/drivers/iommu/iommufd/device.c
> > +++ b/drivers/iommu/iommufd/device.c
> > @@ -98,6 +98,35 @@ struct iommufd_device *iommufd_device_bind(struct
> iommufd_ctx *ictx,
> >  }
> >  EXPORT_SYMBOL_NS_GPL(iommufd_device_bind, IOMMUFD);
> >
> > +/**
> > + * iommufd_ctx_has_group - True if the struct device is bound to this ictx
> 
> What struct device?  Isn't this "True if any device within the group is
> bound to the ictx"?

Yes, yes. a poor copy from a prior version..

> 
> > + * @ictx: iommufd file descriptor
> > + * @group: Pointer to a physical iommu_group struct
> > + *
> > + * True if a iommufd_device_bind() is present for any device within the
> > + * group.
> 
> How can a function be present for a device?  Maybe "True if any device
> within the group has been bound to this ictx, ex. via
> iommufd_device_bind(), therefore implying ictx ownership of the group."  Thanks,

Yes, this is the meaning of it. will fix it.

Regards,
Yi Liu

> 
> > + */
> > +bool iommufd_ctx_has_group(struct iommufd_ctx *ictx, struct iommu_group *group)
> > +{
> > +	struct iommufd_object *obj;
> > +	unsigned long index;
> > +
> > +	if (!ictx || !group)
> > +		return false;
> > +
> > +	xa_lock(&ictx->objects);
> > +	xa_for_each(&ictx->objects, index, obj) {
> > +		if (obj->type == IOMMUFD_OBJ_DEVICE &&
> > +		    container_of(obj, struct iommufd_device, obj)->group == group) {
> > +			xa_unlock(&ictx->objects);
> > +			return true;
> > +		}
> > +	}
> > +	xa_unlock(&ictx->objects);
> > +	return false;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iommufd_ctx_has_group, IOMMUFD);
> > +
> >  /**
> >   * iommufd_device_unbind - Undo iommufd_device_bind()
> >   * @idev: Device returned by iommufd_device_bind()
> > diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
> > index 68cd65274e28..e49c16cd6831 100644
> > --- a/include/linux/iommufd.h
> > +++ b/include/linux/iommufd.h
> > @@ -16,6 +16,7 @@ struct page;
> >  struct iommufd_ctx;
> >  struct iommufd_access;
> >  struct file;
> > +struct iommu_group;
> >
> >  struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
> >  					   struct device *dev, u32 *id);
> > @@ -56,6 +57,7 @@ void iommufd_ctx_get(struct iommufd_ctx *ictx);
> >  #if IS_ENABLED(CONFIG_IOMMUFD)
> >  struct iommufd_ctx *iommufd_ctx_from_file(struct file *file);
> >  void iommufd_ctx_put(struct iommufd_ctx *ictx);
> > +bool iommufd_ctx_has_group(struct iommufd_ctx *ictx, struct iommu_group *group);
> >
> >  int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
> >  			     unsigned long length, struct page **out_pages,
> > @@ -77,6 +79,12 @@ static inline void iommufd_ctx_put(struct iommufd_ctx *ictx)
> >  {
> >  }
> >
> > +static inline bool iommufd_ctx_has_group(struct iommufd_ctx *ictx,
> > +					 struct iommu_group *group)
> > +{
> > +	return false;
> > +}
> > +
> >  static inline int iommufd_access_pin_pages(struct iommufd_access *access,
> >  					   unsigned long iova,
> >  					   unsigned long length,