Re: [PATCH v3 01/18] iommu: Add device dma ownership set/release interfaces
From: Lu Baolu
Date: Mon Dec 06 2021 - 21:07:35 EST
On 12/6/21 10:42 PM, Christoph Hellwig wrote:
On Mon, Dec 06, 2021 at 09:58:46AM +0800, Lu Baolu wrote:
>From the perspective of who is initiating the device to do DMA, device
DMA could be divided into the following types:
DMA_OWNER_DMA_API: Device DMAs are initiated by a kernel driver
through the kernel DMA API.
DMA_OWNER_PRIVATE_DOMAIN: Device DMAs are initiated by a kernel
driver with its own PRIVATE domain.
DMA_OWNER_PRIVATE_DOMAIN_USER: Device DMAs are initiated by
userspace.
Different DMA ownerships are exclusive for all devices in the same iommu
group as an iommu group is the smallest granularity of device isolation
and protection that the IOMMU subsystem can guarantee. This extends the
iommu core to enforce this exclusion.
Basically two new interfaces are provided:
int iommu_device_set_dma_owner(struct device *dev,
enum iommu_dma_owner type, void *owner_cookie);
void iommu_device_release_dma_owner(struct device *dev,
enum iommu_dma_owner type);
Although above interfaces are per-device, DMA owner is tracked per group
under the hood. An iommu group cannot have different dma ownership set
at the same time. Violation of this assumption fails
iommu_device_set_dma_owner().
Kernel driver which does DMA have DMA_OWNER_DMA_API automatically set/
released in the driver binding/unbinding process (see next patch).
Kernel driver which doesn't do DMA could avoid setting the owner type.
Device bound to such driver is considered same as a driver-less device
which is compatible to all owner types.
Userspace driver framework (e.g. vfio) should set
DMA_OWNER_PRIVATE_DOMAIN_USER for a device before the userspace is allowed
to access it, plus a owner cookie pointer to mark the user identity so a
single group cannot be operated by multiple users simultaneously. Vice
versa, the owner type should be released after the user access permission
is withdrawn.
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Signed-off-by: Kevin Tian <kevin.tian@xxxxxxxxx>
Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
---
include/linux/iommu.h | 36 +++++++++++++++++
drivers/iommu/iommu.c | 93 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 129 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d2f3435e7d17..24676b498f38 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -162,6 +162,23 @@ enum iommu_dev_features {
IOMMU_DEV_FEAT_IOPF,
};
+/**
+ * enum iommu_dma_owner - IOMMU DMA ownership
+ * @DMA_OWNER_NONE: No DMA ownership.
+ * @DMA_OWNER_DMA_API: Device DMAs are initiated by a kernel driver through
+ * the kernel DMA API.
+ * @DMA_OWNER_PRIVATE_DOMAIN: Device DMAs are initiated by a kernel driver
+ * which provides an UNMANAGED domain.
+ * @DMA_OWNER_PRIVATE_DOMAIN_USER: Device DMAs are initiated by userspace,
+ * kernel ensures that DMAs never go to kernel memory.
+ */
+enum iommu_dma_owner {
+ DMA_OWNER_NONE,
+ DMA_OWNER_DMA_API,
+ DMA_OWNER_PRIVATE_DOMAIN,
+ DMA_OWNER_PRIVATE_DOMAIN_USER,
+};
+
#define IOMMU_PASID_INVALID (-1U)
#ifdef CONFIG_IOMMU_API
@@ -681,6 +698,10 @@ 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_set_dma_owner(struct device *dev, enum iommu_dma_owner owner,
+ void *owner_cookie);
+void iommu_device_release_dma_owner(struct device *dev, enum iommu_dma_owner owner);
+
#else /* CONFIG_IOMMU_API */
struct iommu_ops {};
@@ -1081,6 +1102,21 @@ static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
{
return NULL;
}
+
+static inline int iommu_device_set_dma_owner(struct device *dev,
+ enum iommu_dma_owner owner,
+ void *owner_cookie)
+{
+ if (owner != DMA_OWNER_DMA_API)
+ return -EINVAL;
+
+ return 0;
+}
+
+static inline void iommu_device_release_dma_owner(struct device *dev,
+ enum iommu_dma_owner owner)
+{
+}
#endif /* CONFIG_IOMMU_API */
/**
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8b86406b7162..1de520a07518 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -48,6 +48,9 @@ struct iommu_group {
struct iommu_domain *default_domain;
struct iommu_domain *domain;
struct list_head entry;
+ enum iommu_dma_owner dma_owner;
+ refcount_t owner_cnt;
owner_cnt is only manipulated under group->mutex, not need for a
refcount_t here, a plain unsigned int while do it and will also
simplify a fair bit of code as it avoid the need for atomic add/sub
and test operations.
Fair enough.
+static int __iommu_group_set_dma_owner(struct iommu_group *group,
+ enum iommu_dma_owner owner,
+ void *owner_cookie)
+{
As pointed out last time, please move the group->mutex locking into
this helper, which makes it identical to the later added public
function.
I didn't mean to ignore your comment. :-) As I replied, by placing the
lock out of the function, the helper could easily handle the error paths
(return directly without something like "goto out_unlock").
As the implementation of iommu_group_set_dma_owner() has been greatly
simplified, I agree with you now, we should move the group->mutex
locking into the helper and make it identical to the latter public
interface.
I will work towards this.
+static void __iommu_group_release_dma_owner(struct iommu_group *group,
+ enum iommu_dma_owner owner)
+{
Same here.
Ditto.
Best regards,
baolu