[PATCH v6 01/11] iommu: Add dma ownership management interfaces
Multiple devices may be placed in the same IOMMU group because they
cannot be isolated from each other. These devices must either be
entirely under kernel control or userspace control, never a mixture.
This adds dma ownership management in iommu core and exposes several
interfaces for the device drivers and the device userspace assignment
framework (i.e. VFIO), so that any conflict between user and kernel
controlled dma could be detected at the beginning.
The device driver oriented interfaces are,
int iommu_device_use_default_domain(struct device *dev);
void iommu_device_unuse_default_domain(struct device *dev);
By calling iommu_device_use_default_domain(), the device driver tells
the iommu layer that the device dma is handled through the kernel DMA
APIs. The iommu layer will manage the IOVA and use the default domain
for DMA address translation.
The device user-space assignment framework oriented interfaces are,
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);
The device userspace assignment must be disallowed if the DMA owner
claiming interface returns failure.
Signed-off-by: Jason Gunthorpe
Signed-off-by: Kevin Tian
Signed-off-by: Lu Baolu
---
include/linux/iommu.h | 31 +
drivers/iommu/iommu.c | 158 +-
2 files changed, 186 insertions(+), 3 deletions(-)
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.
+
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
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);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
On Wed, Feb 23, 2022 at 06:00:06PM +, 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
On 2022-02-23 18:02, Jason Gunthorpe via iommu wrote: On Wed, Feb 23, 2022 at 06:00:06PM +, 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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
On Wed, Feb 23, 2022 at 06:20:36PM +, Robin Murphy wrote: > On 2022-02-23 18:02, Jason Gunthorpe via iommu wrote: > > On Wed, Feb 23, 2022 at 06:00:06PM +, 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
Hi Robin and Jason, On 2/24/22 2:02 AM, Jason Gunthorpe wrote: On Wed, Feb 23, 2022 at 06:00:06PM +, 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
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 +, 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces
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 +, 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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
