| Message ID | 20211115020552.2378167-7-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix BUG_ON in vfio_iommu_group_notifier() | expand |
On Mon, Nov 15, 2021 at 10:05:47AM +0800, Lu Baolu wrote: > The vfio needs to set DMA_OWNER_USER for the entire group when attaching The vfio subsystem? driver? > it to a vfio container. So expose group variants of setting/releasing dma > ownership for this purpose. > > This also exposes the helper iommu_group_dma_owner_unclaimed() for vfio > report to userspace if the group is viable to user assignment, for .. for vfio to report .. ? > void iommu_device_release_dma_owner(struct device *dev, enum iommu_dma_owner owner); > +int iommu_group_set_dma_owner(struct iommu_group *group, enum iommu_dma_owner owner, > + struct file *user_file); > +void iommu_group_release_dma_owner(struct iommu_group *group, enum iommu_dma_owner owner); Pleae avoid all these overly long lines. > +static inline int iommu_group_set_dma_owner(struct iommu_group *group, > + enum iommu_dma_owner owner, > + struct file *user_file) > +{ > + return -EINVAL; > +} > + > +static inline void iommu_group_release_dma_owner(struct iommu_group *group, > + enum iommu_dma_owner owner) > +{ > +} > + > +static inline bool iommu_group_dma_owner_unclaimed(struct iommu_group *group) > +{ > + return false; > +} Why do we need these stubs? All potential callers should already require CONFIG_IOMMU_API? Same for the helpers added in patch 1, btw. > + mutex_lock(&group->mutex); > + ret = __iommu_group_set_dma_owner(group, owner, user_file); > + mutex_unlock(&group->mutex); > + mutex_lock(&group->mutex); > + __iommu_group_release_dma_owner(group, owner); > + mutex_unlock(&group->mutex); Unless I'm missing something (just skipping over the patches), the existing callers also take the lock just around these calls, so we don't really need the __-prefixed lowlevel helpers. > + mutex_lock(&group->mutex); > + owner = group->dma_owner; > + mutex_unlock(&group->mutex); No need for a lock to read a single scalar. > + > + return owner == DMA_OWNER_NONE; > +} > +EXPORT_SYMBOL_GPL(iommu_group_dma_owner_unclaimed);
Hi Christoph, On 2021/11/15 21:27, Christoph Hellwig wrote: > On Mon, Nov 15, 2021 at 10:05:47AM +0800, Lu Baolu wrote: >> The vfio needs to set DMA_OWNER_USER for the entire group when attaching > > The vfio subsystem? driver? "vfio subsystem" > >> it to a vfio container. So expose group variants of setting/releasing dma >> ownership for this purpose. >> >> This also exposes the helper iommu_group_dma_owner_unclaimed() for vfio >> report to userspace if the group is viable to user assignment, for > > .. for vfio to report .. ? Yes. > >> void iommu_device_release_dma_owner(struct device *dev, enum iommu_dma_owner owner); >> +int iommu_group_set_dma_owner(struct iommu_group *group, enum iommu_dma_owner owner, >> + struct file *user_file); >> +void iommu_group_release_dma_owner(struct iommu_group *group, enum iommu_dma_owner owner); > > Pleae avoid all these overly long lines. Sure. Thanks! > >> +static inline int iommu_group_set_dma_owner(struct iommu_group *group, >> + enum iommu_dma_owner owner, >> + struct file *user_file) >> +{ >> + return -EINVAL; >> +} >> + >> +static inline void iommu_group_release_dma_owner(struct iommu_group *group, >> + enum iommu_dma_owner owner) >> +{ >> +} >> + >> +static inline bool iommu_group_dma_owner_unclaimed(struct iommu_group *group) >> +{ >> + return false; >> +} > > Why do we need these stubs? All potential callers should already > require CONFIG_IOMMU_API? Same for the helpers added in patch 1, btw. You are right. This helper is only for vfio which requires IOMMU_API. I will remove this. The helpers in patch 1 seem not the same. The driver core (or bus dma_configure() callback as suggested) will also call them. > >> + mutex_lock(&group->mutex); >> + ret = __iommu_group_set_dma_owner(group, owner, user_file); >> + mutex_unlock(&group->mutex); > >> + mutex_lock(&group->mutex); >> + __iommu_group_release_dma_owner(group, owner); >> + mutex_unlock(&group->mutex); > > Unless I'm missing something (just skipping over the patches), > the existing callers also take the lock just around these calls, > so we don't really need the __-prefixed lowlevel helpers. > Move mutex_lock/unlock will make the helper implementation easier. :-) It seems to be common code style in iommu core. For example, __iommu_attach_group(), __iommu_group_for_each_dev(), etc. >> + mutex_lock(&group->mutex); >> + owner = group->dma_owner; >> + mutex_unlock(&group->mutex); > > No need for a lock to read a single scalar. Adding the lock will make kcasn happy. Jason G also told me that [citing from his review comment] " It is always incorrect to read concurrent data without an annotation of some kind. For instance it can cause mis-execution of logic where the compiler is unaware that a value it loads is allowed to change - ie no READ_ONCE/WRITE_ONCE semantic. " > >> + >> + return owner == DMA_OWNER_NONE; >> +} >> +EXPORT_SYMBOL_GPL(iommu_group_dma_owner_unclaimed); Best regards, baolu
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index f77eb9e7788a..3d2dfd220d3c 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -696,6 +696,10 @@ u32 iommu_sva_get_pasid(struct iommu_sva *handle); int iommu_device_set_dma_owner(struct device *dev, enum iommu_dma_owner owner, struct file *user_file); void iommu_device_release_dma_owner(struct device *dev, enum iommu_dma_owner owner); +int iommu_group_set_dma_owner(struct iommu_group *group, enum iommu_dma_owner owner, + struct file *user_file); +void iommu_group_release_dma_owner(struct iommu_group *group, enum iommu_dma_owner owner); +bool iommu_group_dma_owner_unclaimed(struct iommu_group *group); #else /* CONFIG_IOMMU_API */ @@ -1112,6 +1116,23 @@ static inline void iommu_device_release_dma_owner(struct device *dev, enum iommu_dma_owner owner) { } + +static inline int iommu_group_set_dma_owner(struct iommu_group *group, + enum iommu_dma_owner owner, + struct file *user_file) +{ + return -EINVAL; +} + +static inline void iommu_group_release_dma_owner(struct iommu_group *group, + enum iommu_dma_owner owner) +{ +} + +static inline bool iommu_group_dma_owner_unclaimed(struct iommu_group *group) +{ + return false; +} #endif /* CONFIG_IOMMU_API */ /** diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 916a4d448150..3dcd3fc4290a 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3431,6 +3431,63 @@ static void __iommu_group_release_dma_owner(struct iommu_group *group, } } +/** + * iommu_group_set_dma_owner() - Set DMA ownership of a group + * @group: The group. + * @owner: DMA_OWNER_KERNEL or DMA_OWNER_USER. + * @user_file: The device fd when set USER ownership. + * + * This is to support backward compatibility for legacy vfio which manages + * dma ownership in group level. New invocations on this interface should be + * prohibited. Instead, please turn to iommu_device_set_dma_owner(). + */ +int iommu_group_set_dma_owner(struct iommu_group *group, enum iommu_dma_owner owner, + struct file *user_file) +{ + int ret; + + mutex_lock(&group->mutex); + ret = __iommu_group_set_dma_owner(group, owner, user_file); + mutex_unlock(&group->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(iommu_group_set_dma_owner); + +/** + * iommu_group_release_dma_owner() - Release DMA ownership of a group + * @group: The group. + * @owner: DMA_OWNER_KERNEL or DMA_OWNER_USER. + * + * Release the DMA ownership claimed by iommu_group_set_dma_owner(). + */ +void iommu_group_release_dma_owner(struct iommu_group *group, enum iommu_dma_owner owner) +{ + mutex_lock(&group->mutex); + __iommu_group_release_dma_owner(group, owner); + mutex_unlock(&group->mutex); +} +EXPORT_SYMBOL_GPL(iommu_group_release_dma_owner); + +/** + * iommu_group_dma_owner_unclaimed() - Is group dma ownership claimed + * @group: The group. + * + * This provides status check on a given group. It is racey and only for + * non-binding status reporting. + */ +bool iommu_group_dma_owner_unclaimed(struct iommu_group *group) +{ + enum iommu_dma_owner owner; + + mutex_lock(&group->mutex); + owner = group->dma_owner; + mutex_unlock(&group->mutex); + + return owner == DMA_OWNER_NONE; +} +EXPORT_SYMBOL_GPL(iommu_group_dma_owner_unclaimed); + /** * iommu_device_set_dma_owner() - Set DMA ownership of a device * @dev: The device.
The vfio needs to set DMA_OWNER_USER for the entire group when attaching it to a vfio container. So expose group variants of setting/releasing dma ownership for this purpose. This also exposes the helper iommu_group_dma_owner_unclaimed() for vfio report to userspace if the group is viable to user assignment, for compatibility with VFIO_GROUP_FLAGS_VIABLE. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- include/linux/iommu.h | 21 ++++++++++++++++ drivers/iommu/iommu.c | 57 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+)