Messages in this thread | | | Date | Tue, 16 Nov 2021 17:42:04 +0800 | From | Lu Baolu <> | Subject | Re: [PATCH 06/11] iommu: Expose group variants of dma ownership interfaces |
| |
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
| |