| Message ID | 20220218005521.172832-2-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix BUG_ON in vfio_iommu_group_notifier() | expand |
The overall API and patch looks fine, but: > + * iommu_group_dma_owner_claimed() - Query group dma ownership status > + * @group: The group. > + * > + * This provides status query on a given group. It is racey and only for > + * non-binding status reporting. s/racey/racy/ > + */ > +bool iommu_group_dma_owner_claimed(struct iommu_group *group) > +{ > + unsigned int user; > + > + mutex_lock(&group->mutex); > + user = group->owner_cnt; > + mutex_unlock(&group->mutex); > + > + return user; > +} > +EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed); Still no no need for the lock here.
On 2/19/22 3:31 PM, Christoph Hellwig wrote: > The overall API and patch looks fine, but: > >> + * iommu_group_dma_owner_claimed() - Query group dma ownership status >> + * @group: The group. >> + * >> + * This provides status query on a given group. It is racey and only for >> + * non-binding status reporting. > > s/racey/racy/ Yes. > >> + */ >> +bool iommu_group_dma_owner_claimed(struct iommu_group *group) >> +{ >> + unsigned int user; >> + >> + mutex_lock(&group->mutex); >> + user = group->owner_cnt; >> + mutex_unlock(&group->mutex); >> + >> + return user; >> +} >> +EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed); > > Still no no need for the lock here. We've discussed this before. I tend to think that is right. We don't lose anything with this lock held and it also follows the rule that all accesses to the internal group structure must be done with the group->mutex held. Best regards, baolu
On 2022-02-18 00:55, Lu Baolu wrote: [...] > +/** > + * 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. > + */ > +int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner) > +{ > + int ret = 0; > + > + mutex_lock(&group->mutex); > + if (group->owner_cnt) { To clarify the comment buried in the other thread, I really think we should just unconditionally flag the error here... > + if (group->owner != owner) { > + ret = -EPERM; > + goto unlock_out; > + } > + } else { > + if (group->domain && group->domain != group->default_domain) { > + ret = -EBUSY; > + goto unlock_out; > + } > + > + group->owner = owner; > + if (group->domain) > + __iommu_detach_group(group->domain, group); > + } > + > + group->owner_cnt++; > +unlock_out: > + mutex_unlock(&group->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner); > + > +/** > + * 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); > + if (WARN_ON(!group->owner_cnt || !group->owner)) > + goto unlock_out; > + > + if (--group->owner_cnt > 0) > + goto unlock_out; ...and equivalently just set owner_cnt directly to 0 here. I don't see a realistic use-case for any driver to claim the same group more than once, and allowing it in the API just feels like opening up various potential corners for things to get out of sync. I think that's the only significant concern I have left with the series as a whole - you can consider my other grumbles non-blocking :) Thanks, Robin. > + > + /* > + * The UNMANAGED domain should be detached before all USER > + * owners have been released. > + */ > + if (!WARN_ON(group->domain) && group->default_domain) > + __iommu_attach_group(group->default_domain, group); > + group->owner = NULL; > + > +unlock_out: > + mutex_unlock(&group->mutex); > +} > +EXPORT_SYMBOL_GPL(iommu_group_release_dma_owner); > + > +/** > + * iommu_group_dma_owner_claimed() - Query group dma ownership status > + * @group: The group. > + * > + * This provides status query on a given group. It is racey and only for > + * non-binding status reporting. > + */ > +bool iommu_group_dma_owner_claimed(struct iommu_group *group) > +{ > + unsigned int user; > + > + mutex_lock(&group->mutex); > + user = group->owner_cnt; > + mutex_unlock(&group->mutex); > + > + return user; > +} > +EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
On Wed, Feb 23, 2022 at 06:00:06PM +0000, Robin Murphy wrote: > ...and equivalently just set owner_cnt directly to 0 here. I don't see a > realistic use-case for any driver to claim the same group more than once, > and allowing it in the API just feels like opening up various potential > corners for things to get out of sync. I am Ok if we toss it out to get this merged, as there is no in-kernel user right now. Something will have to come back for iommufd, but we can look at what is best suited then. Thanks, Jason
On 2022-02-23 18:02, Jason Gunthorpe via iommu wrote: > On Wed, Feb 23, 2022 at 06:00:06PM +0000, Robin Murphy wrote: > >> ...and equivalently just set owner_cnt directly to 0 here. I don't see a >> realistic use-case for any driver to claim the same group more than once, >> and allowing it in the API just feels like opening up various potential >> corners for things to get out of sync. > > I am Ok if we toss it out to get this merged, as there is no in-kernel > user right now. > > Something will have to come back for iommufd, but we can look at what > is best suited then. If iommufd plans to be too dumb to keep track of whether it already owns a given group or not, I can't see it dealing with attaching that group to a single domain no more than once, either ;) Robin.
On Wed, Feb 23, 2022 at 06:20:36PM +0000, Robin Murphy wrote: > On 2022-02-23 18:02, Jason Gunthorpe via iommu wrote: > > On Wed, Feb 23, 2022 at 06:00:06PM +0000, Robin Murphy wrote: > > > > > ...and equivalently just set owner_cnt directly to 0 here. I don't see a > > > realistic use-case for any driver to claim the same group more than once, > > > and allowing it in the API just feels like opening up various potential > > > corners for things to get out of sync. > > > > I am Ok if we toss it out to get this merged, as there is no in-kernel > > user right now. > > > > Something will have to come back for iommufd, but we can look at what > > is best suited then. > > If iommufd plans to be too dumb to keep track of whether it already owns a > given group or not, I can't see it dealing with attaching that group to a > single domain no more than once, either ;) Indeed, this is why I'd like to use the device API :) Jason
Hi Robin and Jason, On 2/24/22 2:02 AM, Jason Gunthorpe wrote: > On Wed, Feb 23, 2022 at 06:00:06PM +0000, Robin Murphy wrote: > >> ...and equivalently just set owner_cnt directly to 0 here. I don't see a >> realistic use-case for any driver to claim the same group more than once, >> and allowing it in the API just feels like opening up various potential >> corners for things to get out of sync. > I am Ok if we toss it out to get this merged, as there is no in-kernel > user right now. So we don't need the owner pointer in the API anymore, right? As we will only allow the claiming interface to be called only once, this token is unnecessary. Best regards, baolu
On 2/24/22 2:00 AM, Robin Murphy wrote: > On 2022-02-18 00:55, Lu Baolu wrote: > [...] >> +/** >> + * 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. >> + */ >> +int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner) >> +{ >> + int ret = 0; >> + >> + mutex_lock(&group->mutex); >> + if (group->owner_cnt) { > > To clarify the comment buried in the other thread, I really think we > should just unconditionally flag the error here... > >> + if (group->owner != owner) { >> + ret = -EPERM; >> + goto unlock_out; >> + } >> + } else { >> + if (group->domain && group->domain != group->default_domain) { >> + ret = -EBUSY; >> + goto unlock_out; >> + } >> + >> + group->owner = owner; >> + if (group->domain) >> + __iommu_detach_group(group->domain, group); >> + } >> + >> + group->owner_cnt++; >> +unlock_out: >> + mutex_unlock(&group->mutex); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner); >> + >> +/** >> + * 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); >> + if (WARN_ON(!group->owner_cnt || !group->owner)) >> + goto unlock_out; >> + >> + if (--group->owner_cnt > 0) >> + goto unlock_out; > > ...and equivalently just set owner_cnt directly to 0 here. I don't see a > realistic use-case for any driver to claim the same group more than > once, and allowing it in the API just feels like opening up various > potential corners for things to get out of sync. Yeah! Both make sense to me. I will also drop the owner token in the API as it's unnecessary anymore after the change. > I think that's the only significant concern I have left with the series > as a whole - you can consider my other grumbles non-blocking :) Thank you and very appreciated for your time! Best regards, baolu
On 2/24/22 1:16 PM, Lu Baolu wrote: > Hi Robin and Jason, > > On 2/24/22 2:02 AM, Jason Gunthorpe wrote: >> On Wed, Feb 23, 2022 at 06:00:06PM +0000, Robin Murphy wrote: >> >>> ...and equivalently just set owner_cnt directly to 0 here. I don't see a >>> realistic use-case for any driver to claim the same group more than >>> once, >>> and allowing it in the API just feels like opening up various potential >>> corners for things to get out of sync. >> I am Ok if we toss it out to get this merged, as there is no in-kernel >> user right now. > > So we don't need the owner pointer in the API anymore, right? Oh, NO. The owner token represents that the group has been claimed for user space access. And the default domain auto-attach policy will be changed accordingly. So we still need this. Sorry for the noise. Best regards, baolu
On 2022-02-24 05:29, Lu Baolu wrote: > On 2/24/22 1:16 PM, Lu Baolu wrote: >> Hi Robin and Jason, >> >> On 2/24/22 2:02 AM, Jason Gunthorpe wrote: >>> On Wed, Feb 23, 2022 at 06:00:06PM +0000, Robin Murphy wrote: >>> >>>> ...and equivalently just set owner_cnt directly to 0 here. I don't >>>> see a >>>> realistic use-case for any driver to claim the same group more than >>>> once, >>>> and allowing it in the API just feels like opening up various potential >>>> corners for things to get out of sync. >>> I am Ok if we toss it out to get this merged, as there is no in-kernel >>> user right now. >> >> So we don't need the owner pointer in the API anymore, right? > > Oh, NO. > > The owner token represents that the group has been claimed for user > space access. And the default domain auto-attach policy will be changed > accordingly. > > So we still need this. Sorry for the noise. Exactly. In fact we could almost go the other way, and rename owner_cnt to dma_api_users and make it mutually exclusive with owner being set, but that's really just cosmetic. It's understandable enough as-is that owner_cnt > 0 with owner == NULL represents implicit DMA API ownership. Cheers, Robin.
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 9208eca4b0d1..77972ef978b5 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -675,6 +675,13 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, void iommu_sva_unbind_device(struct iommu_sva *handle); u32 iommu_sva_get_pasid(struct iommu_sva *handle); +int iommu_device_use_default_domain(struct device *dev); +void iommu_device_unuse_default_domain(struct device *dev); + +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); + #else /* CONFIG_IOMMU_API */ struct iommu_ops {}; @@ -1031,6 +1038,30 @@ static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev) { return NULL; } + +static inline int iommu_device_use_default_domain(struct device *dev) +{ + return 0; +} + +static inline void iommu_device_unuse_default_domain(struct device *dev) +{ +} + +static inline int +iommu_group_claim_dma_owner(struct iommu_group *group, void *owner) +{ + return -ENODEV; +} + +static inline void iommu_group_release_dma_owner(struct iommu_group *group) +{ +} + +static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group) +{ + return false; +} #endif /* CONFIG_IOMMU_API */ /** diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index f2c45b85b9fc..4e2ad7124780 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -48,6 +48,8 @@ struct iommu_group { struct iommu_domain *default_domain; struct iommu_domain *domain; struct list_head entry; + unsigned int owner_cnt; + void *owner; }; struct group_device { @@ -294,7 +296,11 @@ int iommu_probe_device(struct device *dev) mutex_lock(&group->mutex); iommu_alloc_default_domain(group, dev); - if (group->default_domain) { + /* + * If device joined an existing group which has been claimed, don't + * attach the default domain. + */ + if (group->default_domain && !group->owner) { ret = __iommu_attach_device(group->default_domain, dev); if (ret) { mutex_unlock(&group->mutex); @@ -2109,7 +2115,7 @@ static int __iommu_attach_group(struct iommu_domain *domain, { int ret; - if (group->default_domain && group->domain != group->default_domain) + if (group->domain && group->domain != group->default_domain) return -EBUSY; ret = __iommu_group_for_each_dev(group, domain, @@ -2146,7 +2152,11 @@ static void __iommu_detach_group(struct iommu_domain *domain, { int ret; - if (!group->default_domain) { + /* + * If the group has been claimed already, do not re-attach the default + * domain. + */ + if (!group->default_domain || group->owner) { __iommu_group_for_each_dev(group, domain, iommu_group_do_detach_device); group->domain = NULL; @@ -3095,3 +3105,145 @@ static ssize_t iommu_group_store_type(struct iommu_group *group, return ret; } + +/** + * iommu_device_use_default_domain() - Device driver wants to handle device + * DMA through the kernel DMA API. + * @dev: The device. + * + * The device driver about to bind @dev wants to do DMA through the kernel + * DMA API. Return 0 if it is allowed, otherwise an error. + */ +int iommu_device_use_default_domain(struct device *dev) +{ + struct iommu_group *group = iommu_group_get(dev); + int ret = 0; + + if (!group) + return 0; + + mutex_lock(&group->mutex); + if (group->owner_cnt) { + if (group->domain != group->default_domain || + group->owner) { + ret = -EBUSY; + goto unlock_out; + } + } + + group->owner_cnt++; + +unlock_out: + mutex_unlock(&group->mutex); + iommu_group_put(group); + + return ret; +} + +/** + * iommu_device_unuse_default_domain() - Device driver stops handling device + * DMA through the kernel DMA API. + * @dev: The device. + * + * The device driver doesn't want to do DMA through kernel DMA API anymore. + * It must be called after iommu_device_use_default_domain(). + */ +void iommu_device_unuse_default_domain(struct device *dev) +{ + struct iommu_group *group = iommu_group_get(dev); + + if (!group) + return; + + mutex_lock(&group->mutex); + if (!WARN_ON(!group->owner_cnt)) + group->owner_cnt--; + + mutex_unlock(&group->mutex); + iommu_group_put(group); +} + +/** + * 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. + */ +int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner) +{ + int ret = 0; + + mutex_lock(&group->mutex); + if (group->owner_cnt) { + if (group->owner != owner) { + ret = -EPERM; + goto unlock_out; + } + } else { + if (group->domain && group->domain != group->default_domain) { + ret = -EBUSY; + goto unlock_out; + } + + group->owner = owner; + if (group->domain) + __iommu_detach_group(group->domain, group); + } + + group->owner_cnt++; +unlock_out: + mutex_unlock(&group->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner); + +/** + * 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); + if (WARN_ON(!group->owner_cnt || !group->owner)) + goto unlock_out; + + if (--group->owner_cnt > 0) + goto unlock_out; + + /* + * The UNMANAGED domain should be detached before all USER + * owners have been released. + */ + if (!WARN_ON(group->domain) && group->default_domain) + __iommu_attach_group(group->default_domain, group); + group->owner = NULL; + +unlock_out: + mutex_unlock(&group->mutex); +} +EXPORT_SYMBOL_GPL(iommu_group_release_dma_owner); + +/** + * iommu_group_dma_owner_claimed() - Query group dma ownership status + * @group: The group. + * + * This provides status query on a given group. It is racey and only for + * non-binding status reporting. + */ +bool iommu_group_dma_owner_claimed(struct iommu_group *group) +{ + unsigned int user; + + mutex_lock(&group->mutex); + user = group->owner_cnt; + mutex_unlock(&group->mutex); + + return user; +} +EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);