Skip to contentSkip to conversation

Add set_dev_data and unset_dev_data support

RFC PATCH Superseded Nicolin Chen 2 participants 7 messages Apr 23, 2023 – Apr 24, 2023

Subsystems (3)

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v3 cover Add set_dev_data and unset_dev_data support

Changelog
v3:
* Reverted the data in VFIO BIND ioctl to a set of new iommufd ioctls
* Replaced the iommu_device_data_size array with a dev_user_data_len variable in the iommu_ops structure.
* Added has_dev_data check and locking protection
* Added selftest coverage
v2:
https://lore.kernel.org/all/[email protected]/
* Integrated the uAPI into VFIO_DEVICE_BIND_IOMMUFD call
* Renamed the previous set_rid_user to set_dev_data, to decouple from the PCI regime.
v1:
https://lore.kernel.org/all/[email protected]/

This is a pair of new uAPI/ops for user space to set an iommu specific device data for a passthrough device. This is primarily used by SMMUv3 driver for now, to link the vSID and the pSID of a device that's behind the SMMU. The link (lookup table) will be used to verify any ATC_INV command from the user space for that device, and then replace the SID field (virtual SID) with the corresponding physical SID.

This series is available on Github:
https://github.com/nicolinc/iommufd/commits/set_dev_data-rfc-v3

Thanks!
Nicolin

Nicolin Chen (4):
iommu: Add set/unset_dev_user_data ops
iommufd: Add IOMMUFD_CMD_DEVICE_SET_DATA and
IOMMUFD_CMD_DEVICE_UNSET_DATA
iommufd/selftest: Add IOMMU_TEST_OP_DEV_CHECK_DATA iommufd/selftests: Add coverage for IOMMU_DEVICE_SET/UNSET_DATA

 drivers/iommu/iommufd/device.c                | 81 +++++++++++++++++++
 drivers/iommu/iommufd/iommufd_private.h       |  3 +
 drivers/iommu/iommufd/iommufd_test.h          | 15 ++++
 drivers/iommu/iommufd/main.c                  |  4 +
 drivers/iommu/iommufd/selftest.c              | 42 ++++++++++
 include/linux/iommu.h                         | 13 +++
 include/uapi/linux/iommufd.h                  | 32 ++++++++
 tools/testing/selftests/iommu/iommufd.c       | 20 +++++
 tools/testing/selftests/iommu/iommufd_utils.h | 59 ++++++++++++++
 9 files changed, 269 insertions(+)
Signature · 1 line
2.40.0
#1 lore raw

RFC PATCH v3 1/4 iommu: Add set/unset_dev_user_data ops

The device behind an IOMMU might be used in the user space by a VM. So, it might have some user space data. For example, a device behind an SMMU has a static stream ID. In a virtualization use case, both a host environment and a guest environment have their own Stream IDs. A link (a lookup table) between the physical Stream ID and the virtual (user) Stream ID is needed when the host handles the user cache invalidation commands.

Add a pair of new ops to allow user space to forward user_data of a device via iommufd, and a new dev_user_data_len for data structure sanity done by the iommufd core.

---
 include/linux/iommu.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)
include/linux/iommu.h +13 -0
index 5c99aeaccd1d..33b11d87b374 100644
@@ -257,6 +257,15 @@ struct iommu_iotlb_gather {
  * @remove_dev_pasid: Remove any translation configurations of a specific
  *                    pasid, so that any DMA transactions with this pasid
  *                    will be blocked by the hardware.
+ * @set/unset_dev_user_data: set/unset an iommu specific device data from user
+ *                           space. The user device data info will be used by
+ *                           the driver to take care of user space requests.
+ *                           The device data structure must be defined in
+ *                           include/uapi/linux/iommufd.h.
+ * @dev_user_data_len: Length of the device data from user space (in bytes),
+ *                     simply the "sizeof" the data structure defined in the
+ *                     include/uapi/linux/iommufd.h. This is used by iommufd
+ *                     core to run a data length validation.
  * @hw_info_type: One of enum iommu_hw_info_type defined in
  *                include/uapi/linux/iommufd.h. It is used to tag the type
  *                of data returned by .hw_info callback. The drivers that
@@ -303,6 +312,10 @@ struct iommu_ops {
 	int (*def_domain_type)(struct device *dev);
 	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
 
+	int (*set_dev_user_data)(struct device *dev, const void *user_data);
+	void (*unset_dev_user_data)(struct device *dev);
+	size_t dev_user_data_len;
+
 	const struct iommu_domain_ops *default_domain_ops;
 	enum iommu_hw_info_type hw_info_type;
 	unsigned long long hwpt_type_bitmap;
Signature · 1 line
2.40.0
#2 lore raw

RFC PATCH v3 2/4 iommufd: Add IOMMUFD_CMD_DEVICE_SET_DATA and IOMMUFD_CMD_DEVICE_UNSET_DATA

In reply to Nicolin Chen

Add a new pair of ioctls to allow user space to set and unset its iommu- specific device data for a passthrough device that's behind the iommu.

On platforms with SMMUv3, this new uAPIs will be used to forward a user space virtual Stream ID of a passthrough device to link to its physical Stream ID and log into a lookup table, in order for the host kernel to later run sanity on ATC invalidation requests from the user space, with ATC_INV commands that have SID fields (virtual Stream IDs).

---
 drivers/iommu/iommufd/device.c          | 81 +++++++++++++++++++++++++
 drivers/iommu/iommufd/iommufd_private.h |  3 +
 drivers/iommu/iommufd/main.c            |  4 ++
 include/uapi/linux/iommufd.h            | 32 ++++++++++
 4 files changed, 120 insertions(+)
drivers/iommu/iommufd/device.c +81 -0
index c649a3403797..9480cd36a8bd 100644
@@ -136,6 +136,8 @@ void iommufd_device_destroy(struct iommufd_object *obj)
 	struct iommufd_device *idev =
 		container_of(obj, struct iommufd_device, obj);
 
+	if (WARN_ON(idev->has_user_data))
+		dev_iommu_ops(idev->dev)->unset_dev_user_data(idev->dev);
 	iommu_device_release_dma_owner(idev->dev);
 	iommufd_put_group(idev->igroup);
 	if (!iommufd_selftest_is_mock_dev(idev->dev))
@@ -726,6 +728,85 @@ void iommufd_device_detach(struct iommufd_device *idev)
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_device_detach, IOMMUFD);
 
+int iommufd_device_set_data(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_device_set_data *cmd = ucmd->cmd;
+	struct iommufd_device *idev;
+	const struct iommu_ops *ops;
+	void *data = NULL;
+	int rc;
+
+	if (!cmd->data_uptr || !cmd->data_len)
+		return -EINVAL;
+
+	idev = iommufd_get_device(ucmd, cmd->dev_id);
+	if (IS_ERR(idev))
+		return PTR_ERR(idev);
+
+	mutex_lock(&idev->igroup->lock);
+	if (idev->has_user_data) {
+		rc = -EEXIST;
+		goto out_unlock;
+	}
+
+	ops = dev_iommu_ops(idev->dev);
+	if (!ops->dev_user_data_len ||
+	    !ops->set_dev_user_data ||
+	    !ops->unset_dev_user_data) {
+		rc = -EOPNOTSUPP;
+		goto out_unlock;
+	}
+
+	data = kzalloc(ops->dev_user_data_len, GFP_KERNEL);
+	if (!data) {
+		rc = -ENOMEM;
+		goto out_unlock;
+	}
+
+	if (copy_struct_from_user(data, ops->dev_user_data_len,
+				  u64_to_user_ptr(cmd->data_uptr),
+				  cmd->data_len)) {
+		rc = -EFAULT;
+		goto out_free_data;
+	}
+
+	rc = ops->set_dev_user_data(idev->dev, data);
+	if (rc)
+		goto out_free_data;
+
+	idev->has_user_data = true;
+out_free_data:
+	kfree(data);
+out_unlock:
+	mutex_unlock(&idev->igroup->lock);
+	iommufd_put_object(&idev->obj);
+	return rc;
+}
+
+int iommufd_device_unset_data(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_device_unset_data *cmd = ucmd->cmd;
+	struct iommufd_device *idev;
+	int rc = 0;
+
+	idev = iommufd_get_device(ucmd, cmd->dev_id);
+	if (IS_ERR(idev))
+		return PTR_ERR(idev);
+
+	mutex_lock(&idev->igroup->lock);
+	if (!idev->has_user_data) {
+		rc = -ENOENT;
+		goto out_unlock;
+	}
+
+	dev_iommu_ops(idev->dev)->unset_dev_user_data(idev->dev);
+	idev->has_user_data = false;
+out_unlock:
+	mutex_unlock(&idev->igroup->lock);
+	iommufd_put_object(&idev->obj);
+	return rc;
+}
+
 void iommufd_access_destroy_object(struct iommufd_object *obj)
 {
 	struct iommufd_access *access =
drivers/iommu/iommufd/iommufd_private.h +3 -0
index 35d1294b2f06..98eaef9c41d3 100644
@@ -309,6 +309,7 @@ struct iommufd_device {
 	/* always the physical device */
 	struct device *dev;
 	bool enforce_cache_coherency;
+	bool has_user_data;
 };
 
 static inline struct iommufd_device *
@@ -321,6 +322,8 @@ iommufd_get_device(struct iommufd_ucmd *ucmd, u32 id)
 
 void iommufd_device_destroy(struct iommufd_object *obj);
 int iommufd_device_get_hw_info(struct iommufd_ucmd *ucmd);
+int iommufd_device_set_data(struct iommufd_ucmd *ucmd);
+int iommufd_device_unset_data(struct iommufd_ucmd *ucmd);
 
 struct iommufd_access {
 	struct iommufd_object obj;
drivers/iommu/iommufd/main.c +4 -0
index b84b22487b7d..bd4753512990 100644
@@ -326,6 +326,10 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
 		 val64),
 	IOCTL_OP(IOMMU_VFIO_IOAS, iommufd_vfio_ioas, struct iommu_vfio_ioas,
 		 __reserved),
+	IOCTL_OP(IOMMU_DEVICE_SET_DATA, iommufd_device_set_data,
+		 struct iommu_device_set_data, data_len),
+	IOCTL_OP(IOMMU_DEVICE_UNSET_DATA, iommufd_device_unset_data,
+		 struct iommu_device_unset_data, dev_id),
 #ifdef CONFIG_IOMMUFD_TEST
 	IOCTL_OP(IOMMU_TEST_CMD, iommufd_test, struct iommu_test_cmd, last),
 #endif
include/uapi/linux/iommufd.h +32 -0
index ac023ea6040a..b39b0e2e46b1 100644
@@ -51,6 +51,8 @@ enum {
 	IOMMUFD_CMD_HWPT_ALLOC,
 	IOMMUFD_CMD_DEVICE_GET_HW_INFO,
 	IOMMUFD_CMD_HWPT_INVALIDATE,
+	IOMMUFD_CMD_DEVICE_SET_DATA,
+	IOMMUFD_CMD_DEVICE_UNSET_DATA,
 };
 
 /**
@@ -626,4 +628,34 @@ struct iommu_hwpt_invalidate {
 	__aligned_u64 data_uptr;
 };
 #define IOMMU_HWPT_INVALIDATE _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_INVALIDATE)
+
+/**
+ * struct iommu_device_set_data - ioctl(IOMMU_DEVICE_SET_DATA)
+ * @size: sizeof(struct iommu_device_set_data)
+ * @dev_id: The device to set an iommu specific device data
+ * @data_uptr: User pointer of the device user data
+ * @data_len: Length of the device user data
+ *
+ * The device data must be unset using ioctl(IOMMU_DEVICE_UNSET_DATA), before
+ * another ioctl(IOMMU_DEVICE_SET_DATA) call or before the device itself gets
+ * unbind'd from the iommufd context.
+ */
+struct iommu_device_set_data {
+	__u32 size;
+	__u32 dev_id;
+	__aligned_u64 data_uptr;
+	__u32 data_len;
+};
+#define IOMMU_DEVICE_SET_DATA _IO(IOMMUFD_TYPE, IOMMUFD_CMD_DEVICE_SET_DATA)
+
+/**
+ * struct iommu_device_unset_data - ioctl(IOMMU_DEVICE_UNSET_DATA)
+ * @size: sizeof(struct iommu_device_unset_data)
+ * @dev_id: The device to unset its device user data
+ */
+struct iommu_device_unset_data {
+	__u32 size;
+	__u32 dev_id;
+};
+#define IOMMU_DEVICE_UNSET_DATA _IO(IOMMUFD_TYPE, IOMMUFD_CMD_DEVICE_UNSET_DATA)
 #endif
Signature · 1 line
2.40.0
#3 lore raw

Re: [PATCH RFC v3 2/4] iommufd: Add IOMMUFD_CMD_DEVICE_SET_DATA and IOMMUFD_CMD_DEVICE_UNSET_DATA

Quoted 22 lines · Nicolin Chen

Add a new pair of ioctls to allow user space to set and unset its iommu- specific device data for a passthrough device that's behind the iommu.

On platforms with SMMUv3, this new uAPIs will be used to forward a user space virtual Stream ID of a passthrough device to link to its physical Stream ID and log into a lookup table, in order for the host kernel to later run sanity on ATC invalidation requests from the user space, with ATC_INV commands that have SID fields (virtual Stream IDs).

---
  drivers/iommu/iommufd/device.c          | 81 +++++++++++++++++++++++++
  drivers/iommu/iommufd/iommufd_private.h |  3 +
  drivers/iommu/iommufd/main.c            |  4 ++
  include/uapi/linux/iommufd.h            | 32 ++++++++++
  4 files changed, 120 insertions(+)
drivers/iommu/iommufd/device.c +0 -0
index c649a3403797..9480cd36a8bd 100644

drivers/iommu/iommufd/device.c void iommufd_device_destroy(struct iommufd_object *obj) · line 140

Quoted 5 lines
drivers/iommu/iommufd/device.c +2 -0
@@ -136,6 +136,8 @@ void iommufd_device_destroy(struct iommufd_object *obj)
  	struct iommufd_device *idev =
  		container_of(obj, struct iommufd_device, obj);
 
+	if (WARN_ON(idev->has_user_data))
+		dev_iommu_ops(idev->dev)->unset_dev_user_data(idev->dev);

Do you really need this WARN_ON()? The user space application can easily trigger this kernel WARN() by setting the user data and forgetting to unset it.

Best regards,
baolu

#4 lore raw

Re: [PATCH RFC v3 2/4] iommufd: Add IOMMUFD_CMD_DEVICE_SET_DATA and IOMMUFD_CMD_DEVICE_UNSET_DATA

Hi Baolu,

Quoted 26 lines · Baolu Lu
Quoted 25 lines · Nicolin Chen

Add a new pair of ioctls to allow user space to set and unset its iommu- specific device data for a passthrough device that's behind the iommu.

On platforms with SMMUv3, this new uAPIs will be used to forward a user space virtual Stream ID of a passthrough device to link to its physical Stream ID and log into a lookup table, in order for the host kernel to later run sanity on ATC invalidation requests from the user space, with ATC_INV commands that have SID fields (virtual Stream IDs).

---
  drivers/iommu/iommufd/device.c          | 81 +++++++++++++++++++++++++
  drivers/iommu/iommufd/iommufd_private.h |  3 +
  drivers/iommu/iommufd/main.c            |  4 ++
  include/uapi/linux/iommufd.h            | 32 ++++++++++
  4 files changed, 120 insertions(+)
drivers/iommu/iommufd/device.c +0 -0
index c649a3403797..9480cd36a8bd 100644
@@ -136,6 +136,8 @@ void iommufd_device_destroy(struct iommufd_object *obj)
      struct iommufd_device *idev =
              container_of(obj, struct iommufd_device, obj);
 
Quoted 6 lines

drivers/iommu/iommufd/device.c void iommufd_device_destroy(struct iommufd_object *obj) · line 140

Quoted 2 lines
drivers/iommu/iommufd/device.c +2 -0
+     if (WARN_ON(idev->has_user_data))
+             dev_iommu_ops(idev->dev)->unset_dev_user_data(idev->dev);

Do you really need this WARN_ON()? The user space application can easily trigger this kernel WARN() by setting the user data and forgetting to unset it.

I can drop that, since it's a user triggerable one.

Thanks
Nic

#5 lore raw

RFC PATCH v3 3/4 iommufd/selftest: Add IOMMU_TEST_OP_DEV_CHECK_DATA

In reply to Nicolin Chen

Add mock_domain_set/unset_dev_user_data and iommufd_test_dev_check_data to allow testing on IOMMUFD_CMD_DEVICE_SET/UNSET_DATA ioctls from user space selftest via IOMMU_TEST_OP_DEV_CHECK_DATA.

---
 drivers/iommu/iommufd/iommufd_test.h | 15 ++++++++++
 drivers/iommu/iommufd/selftest.c     | 42 ++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)
drivers/iommu/iommufd/iommufd_test.h +15 -0
index 8f93d83fb52a..b27db4cfb230 100644
@@ -20,6 +20,7 @@ enum {
 	IOMMU_TEST_OP_MOCK_DOMAIN_REPLACE,
 	IOMMU_TEST_OP_ACCESS_REPLACE_IOAS,
 	IOMMU_TEST_OP_MD_CHECK_IOTLB,
+	IOMMU_TEST_OP_DEV_CHECK_DATA,
 };
 
 enum {
@@ -99,6 +100,9 @@ struct iommu_test_cmd {
 		struct {
 			__u32 iotlb;
 		} check_iotlb;
+		struct {
+			__u32 val;
+		} check_dev_data;
 	};
 	__u32 last;
 };
@@ -119,6 +123,17 @@ struct iommu_test_hw_info {
 	__u32 test_reg;
 };
 
+#define IOMMU_DEVICE_DATA_SELFTEST	0xdadbeef
+
+/**
+ * struct iommu_test_device_data
+ *
+ * @val: Should be set to IOMMU_DEVICE_DATA_SELFTEST or unset to 0x0
+ */
+struct iommu_test_device_data {
+	__u32 val;
+};
+
 /* Should not be equal to any defined value in enum iommu_hwpt_type */
 #define IOMMU_HWPT_TYPE_SELFTTEST	0xbadbeef
 #define IOMMU_HWPT_TYPE_BITMAP_SELFTTEST	U64_MAX
drivers/iommu/iommufd/selftest.c +42 -0

72 lines not on this page · raw message

Signature · 1 line
2.40.0
#6 lore raw

RFC PATCH v3 4/4 iommufd/selftests: Add coverage for IOMMU_DEVICE_SET/UNSET_DATA

In reply to Nicolin Chen

Add a new IOMMU_TEST_OP_DEV_CHECK_DATA to verify whether the dev_data is properly set/unset by the IOMMU_DEVICE_SET/UNSET_DATA.

---
 tools/testing/selftests/iommu/iommufd.c       | 20 +++++++
 tools/testing/selftests/iommu/iommufd_utils.h | 59 +++++++++++++++++++
 2 files changed, 79 insertions(+)
tools/testing/selftests/iommu/iommufd.c +20 -0
index 7291e1c4242a..8265ec6a0606 100644
@@ -1483,6 +1483,26 @@ TEST_F(iommufd_mock_domain, alloc_hwpt)
 	}
 }
 
+TEST_F(iommufd_mock_domain, set_dev_data)
+{
+	struct iommu_test_device_data dev_data = {
+		.val = IOMMU_DEVICE_DATA_SELFTEST,
+	};
+	int i;
+
+	for (i = 0; i != variant->mock_domains; i++) {
+		test_err_device_set_data(ENOENT, 0, &dev_data);
+		test_err_device_set_data(EINVAL, self->idev_ids[i], NULL);
+		test_cmd_device_set_data(self->idev_ids[i], &dev_data);
+		test_err_device_set_data(EEXIST, self->idev_ids[i], &dev_data);
+		test_cmd_dev_check_data(self->idev_ids[i], dev_data.val);
+		test_err_device_unset_data(ENOENT, 0);
+		test_cmd_device_unset_data(self->idev_ids[i]);
+		test_err_device_unset_data(ENOENT, self->idev_ids[i]);
+		test_cmd_dev_check_data(self->idev_ids[i], 0);
+	}
+}
+
 /* VFIO compatibility IOCTLs */
 
 TEST_F(iommufd, simple_ioctls)
tools/testing/selftests/iommu/iommufd_utils.h +59 -0

62 lines not on this page · raw message

Signature · 1 line
2.40.0