Add set_dev_data and unset_dev_data support

RFC PATCH Superseded Nicolin Chen 3 participants 15 messages Apr 20, 2023 – Apr 23, 2023

Subsystems (4)

KVM IOMMU VFIO driver Iommufd

lore

Share & tools

lore

Nicolin Chen

author

2023-04-20 07:48 UTC #0 lore raw

v2 cover Add set_dev_data and unset_dev_data support

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-v2

Thanks!
Nicolin

Nicolin Chen (3):
iommu: Add set/unset_dev_data_user ops
iommufd: Add iommufd_device_set_data and iommufd_device_unset_data
APIs
vfio: Add dev_data_len/uptr in struct vfio_device_bind_iommufd

 drivers/iommu/iommufd/device.c | 65 ++++++++++++++++++++++++++++++++++
 drivers/vfio/device_cdev.c     | 19 ++++++++--
 drivers/vfio/iommufd.c         | 13 +++++++
 include/linux/iommu.h          |  6 ++++
 include/linux/iommufd.h        |  4 +++
 include/linux/vfio.h           |  2 ++
 include/uapi/linux/vfio.h      | 13 +++++++
 7 files changed, 120 insertions(+), 2 deletions(-)

Signature · 1 line

2.40.0

Nicolin Chen

author

2023-04-20 07:48 UTC #1 lore raw

RFC PATCH v2 1/3 iommu: Add set/unset_dev_data_user 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.

Signed-off-by Nicolin Chen <[email protected]>

---
 include/linux/iommu.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/iommu.h +6 -0

index 5c99aeaccd1d..c87f0f1527dd 100644
@@ -257,6 +257,9 @@ 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_data_user: 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.
  * @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 +306,9 @@ struct iommu_ops {
 	int (*def_domain_type)(struct device *dev);
 	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
 
+	int (*set_dev_data_user)(struct device *dev, const void *user_data);
+	void (*unset_dev_data_user)(struct device *dev);
+
 	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

Nicolin Chen

author

2023-04-20 07:48 UTC #2 lore raw

RFC PATCH v2 2/3 iommufd: Add iommufd_device_set_data and iommufd_device_unset_data APIs

In reply to Nicolin Chen

The uAPI handlers can call these two functions to set or unset an iommu specific device data. For example, the SMMUv3 driver would get a virtual Stream ID using this interface to tie it to the device's physical Stream ID, for sanity at the device-specific invalidation commands.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/device.c | 65 ++++++++++++++++++++++++++++++++++
 include/linux/iommufd.h        |  4 +++
 2 files changed, 69 insertions(+)

drivers/iommu/iommufd/device.c +65 -0

index 16fdf4a16643..c4e895239486 100644
@@ -610,6 +610,71 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	return destroy_hwpt;
 }
 
+/* size of iommu specific device data, indexed by enum hw_info_type. */
+static const size_t iommufd_device_data_size[] = {
+	[IOMMU_HW_INFO_TYPE_NONE] = 0,
+	[IOMMU_HW_INFO_TYPE_INTEL_VTD] = 0,
+};
+
+/**
+ * iommufd_device_set_data - Set an iommu specific device data
+ * @idev: device to set data
+ * @data_uptr: User pointer to an iommu specific device data
+ * @data_len: Length of the iommu specific device user data
+ *
+ * This sets an iommu specific device data from the user space.
+ */
+int iommufd_device_set_data(struct iommufd_device *idev,
+			    void __user *data_uptr, size_t data_len)
+{
+	const struct iommu_ops *ops = dev_iommu_ops(idev->dev);
+	void *data = NULL;
+	u32 klen = 0;
+	int rc;
+
+	if (!data_uptr || !data_len)
+		return -EINVAL;
+	if (!ops->set_dev_data_user || !ops->unset_dev_data_user)
+		return -EOPNOTSUPP;
+	if (ops->hw_info_type >= ARRAY_SIZE(iommufd_device_data_size))
+		return -EOPNOTSUPP;
+
+	klen = iommufd_device_data_size[ops->hw_info_type];
+	if (!klen)
+		return -EOPNOTSUPP;
+
+	data = kzalloc(klen, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	if (copy_struct_from_user(data, klen, data_uptr, data_len)) {
+		rc = -EFAULT;
+		goto out_free_data;
+	}
+
+	rc = ops->set_dev_data_user(idev->dev, data);
+out_free_data:
+	kfree(data);
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_device_set_data, IOMMUFD);
+
+/**
+ * iommufd_device_unset_data - Unset a device's iommu specific data
+ * @idev: device to unset data
+ *
+ * This unsets an iommu specific device data that is previously set from user
+ * space, calling ops->unset_dev_data_user.
+ */
+void iommufd_device_unset_data(struct iommufd_device *idev)
+{
+	const struct iommu_ops *ops = dev_iommu_ops(idev->dev);
+
+	if (ops->unset_dev_data_user)
+		ops->unset_dev_data_user(idev->dev);
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_device_unset_data, IOMMUFD);
+
 static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
 				    attach_fn do_attach)
 {

include/linux/iommufd.h +4 -0

index 6752c58226d1..4b3a70faa409 100644
@@ -25,6 +25,10 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id);
 int iommufd_device_replace(struct iommufd_device *idev, u32 *pt_id);
 void iommufd_device_detach(struct iommufd_device *idev);
 
+int iommufd_device_set_data(struct iommufd_device *idev,
+			    void __user *data_uptr, size_t data_len);
+void iommufd_device_unset_data(struct iommufd_device *idev);
+
 struct iommufd_ctx *iommufd_device_to_ictx(struct iommufd_device *idev);
 u32 iommufd_device_to_id(struct iommufd_device *idev);

Signature · 1 line

2.40.0

Nicolin Chen

author

2023-04-20 07:48 UTC #3 lore raw

RFC PATCH v2 3/3 vfio: Add dev_data_len/uptr in struct vfio_device_bind_iommufd

In reply to Nicolin Chen

This allows user space to pass in an iommu specific device data with the VFIO_DEVICE_BIND_IOMMUFD ioctl. The data is not mandatory. But it must be provided if a non-zero data_len is passed along.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/vfio/device_cdev.c | 19 +++++++++++++++++--
 drivers/vfio/iommufd.c     | 13 +++++++++++++
 include/linux/vfio.h       |  2 ++
 include/uapi/linux/vfio.h  | 13 +++++++++++++
 4 files changed, 45 insertions(+), 2 deletions(-)

drivers/vfio/device_cdev.c +17 -2

index b5824179cd48..70241d9c0fa9 100644
@@ -104,9 +104,10 @@ static struct iommufd_ctx *vfio_get_iommufd_from_fd(int fd)
 long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
 				    struct vfio_device_bind_iommufd __user *arg)
 {
+	uint32_t mask = VFIO_DEVICE_BIND_IOMMUFD_FLAG_DATA;
 	struct vfio_device *device = df->device;
 	struct vfio_device_bind_iommufd bind;
-	unsigned long minsz;
+	unsigned long minsz, datasz;
 	bool is_noiommu;
 	int ret;

@@ -117,9 +118,23 @@ long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
 	if (copy_from_user(&bind, arg, minsz))
 		return -EFAULT;
 
-	if (bind.argsz < minsz || bind.flags)
+	if (bind.argsz < minsz || bind.flags & ~mask)
 		return -EINVAL;
 
+	if (bind.flags & VFIO_DEVICE_BIND_IOMMUFD_FLAG_DATA) {
+		datasz = offsetofend(struct vfio_device_bind_iommufd,
+				     dev_data_len);
+		if (bind.argsz < datasz)
+			return -EINVAL;
+		if (copy_from_user((void *)&bind + minsz,
+				   (void __user *)arg + minsz, datasz - minsz))
+			return -EFAULT;
+		if (!bind.dev_data_uptr ^ !bind.dev_data_len)
+			return -EINVAL;
+		device->user_data = u64_to_user_ptr(bind.dev_data_uptr);
+		device->user_data_len = bind.dev_data_len;
+	}
+
 	/* BIND_IOMMUFD only allowed for cdev fds */
 	if (df->group)
 		return -EINVAL;

drivers/vfio/iommufd.c +13 -0

index 16d6aac06180..0b985ccffcbe 100644
@@ -91,6 +91,16 @@ int vfio_iommufd_physical_bind(struct vfio_device *vdev,
 	idev = iommufd_device_bind(ictx, vdev->dev, out_device_id);
 	if (IS_ERR(idev))
 		return PTR_ERR(idev);
+
+	if (vdev->user_data) {
+		int rc = iommufd_device_set_data(idev, vdev->user_data,
+						 vdev->user_data_len);
+		if (rc) {
+			iommufd_device_unbind(idev);
+			return rc;
+		}
+	}
+
 	vdev->iommufd_device = idev;
 	return 0;
 }

@@ -104,8 +114,11 @@ void vfio_iommufd_physical_unbind(struct vfio_device *vdev)
 		iommufd_device_detach(vdev->iommufd_device);
 		vdev->iommufd_attached = false;
 	}
+	iommufd_device_unset_data(vdev->iommufd_device);
 	iommufd_device_unbind(vdev->iommufd_device);
 	vdev->iommufd_device = NULL;
+	vdev->user_data = NULL;
+	vdev->user_data_len = 0;
 }
 EXPORT_SYMBOL_GPL(vfio_iommufd_physical_unbind);

include/linux/vfio.h +2 -0

index 46b313f8bfaf..e4bb63801472 100644
@@ -68,6 +68,8 @@ struct vfio_device {
 #if IS_ENABLED(CONFIG_IOMMUFD)
 	struct iommufd_device *iommufd_device;
 	bool iommufd_attached;
+	void *user_data;
+	u32 user_data_len;
 #endif
 	bool noiommu;
 };

include/uapi/linux/vfio.h +13 -0

index 17c5a1dadd08..9cec823c2829 100644
@@ -212,15 +212,28 @@ struct vfio_group_status {
  *		as long as the input @iommufd is valid. Otherwise, it is
  *		meaningless. devid is a handle for this device and can be
  *		used in IOMMUFD commands.
+ * @dev_data_uptr: User pointer of the device user data.
+ * @dev_data_len: Length of the device user data.
+ *
+ * A device user data is an iommu specific structure that must be defined in
+ * the include/uapi/linux/iommufd.h file. On some platform enabling the iommu
+ * nested translation configuration, a device behind the iommu, while working
+ * in a guest VM, needs to provide the host kernel a certain virtual ID in the
+ * guest VM. For example, ARM SMMUv3 requires a virtual Stream ID to sanity a
+ * cache invalidation command from the user space. User space wanting to pass a
+ * user data must set VFIO_DEVICE_BIND_IOMMUFD_FLAG_DATA flag.
  *
  * Return: 0 on success, -errno on failure.
  */
 struct vfio_device_bind_iommufd {
 	__u32		argsz;
 	__u32		flags;
+#define VFIO_DEVICE_BIND_IOMMUFD_FLAG_DATA		(1 << 0)
 	__s32		iommufd;
 #define VFIO_NOIOMMU_FD	(-1)
 	__u32		out_devid;
+	__aligned_u64	dev_data_uptr;
+	__u32		dev_data_len;
 };
 
 #define VFIO_DEVICE_BIND_IOMMUFD	_IO(VFIO_TYPE, VFIO_BASE + 19)

Signature · 1 line

2.40.0

Tian, Kevin

maintainer

2023-04-21 07:36 UTC #4 lore raw

In reply to Nicolin Chen

Quoted 10 lines

From: Nicolin Chen <[email protected]>
Sent: Thursday, April 20, 2023 3:48 PM
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.

Quoted 6 lines

This series is available on Github:
https://github.com/nicolinc/iommufd/commits/set_dev_data-rfc-v2
Thanks!
Nicolin

there is no changelog compared to v1.

Could you add some words why changing from passing the information in an iommufd ioctl to bind_iommufd? My gut-feeling leans toward the latter option...

Nicolin Chen

author

2023-04-21 07:41 UTC #5 lore raw

Quoted 13 lines · Kevin

External email: Use caution opening links or attachments
Quoted 10 lines
From: Nicolin Chen <[email protected]>
Sent: Thursday, April 20, 2023 3:48 PM
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.

Quoted 8 lines

Quoted 6 lines
This series is available on Github:
https://github.com/nicolinc/iommufd/commits/set_dev_data-rfc-v2
Thanks!
Nicolin
there is no changelog compared to v1.

Weird! How could it be missed during copy-n-paste..
I recalled that I had it but seemingly lost it after an update.

It is in the commit message of the cover-letter though:
https://github.com/nicolinc/iommufd/commit/5e17d270bfca2a5e3e7401d4bf58ae53eb7a8a55
--------------------------------------------------------
Changelog
v2:
* 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]/
--------------------------------------------------------

Quoted 3 lines

Could you add some words why changing from passing the information in an iommufd ioctl to bind_iommufd? My gut-feeling leans toward the latter option...

Yea. Jason told me to decouple it from PCI. And merge it into a general uAPI. So I picked the BIND ioctl.

Thanks
Nic

Tian, Kevin

maintainer

2023-04-21 07:47 UTC #6 lore raw

Quoted 42 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, April 21, 2023 3:42 PM
Quoted 13 lines · Kevin
External email: Use caution opening links or attachments
Quoted 10 lines
From: Nicolin Chen <[email protected]>
Sent: Thursday, April 20, 2023 3:48 PM
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.
Quoted 8 lines
Quoted 6 lines
This series is available on Github:
https://github.com/nicolinc/iommufd/commits/set_dev_data-rfc-v2
Thanks!
Nicolin
there is no changelog compared to v1.
Weird! How could it be missed during copy-n-paste..
I recalled that I had it but seemingly lost it after an update.
It is in the commit message of the cover-letter though:
https://github.com/nicolinc/iommufd/commit/5e17d270bfca2a5e3e7401d4b
f58ae53eb7a8a55
--------------------------------------------------------
Changelog
v2:
* 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]/
--------------------------------------------------------

Quoted 7 lines

Quoted 3 lines
Could you add some words why changing from passing the information in an iommufd ioctl to bind_iommufd? My gut-feeling leans toward the latter option...
Yea. Jason told me to decouple it from PCI. And merge it into a general uAPI. So I picked the BIND ioctl.

'decouple it from PCI' is kind of covered by renaming set_rid to set_data. but I didn't get why this has to be merged with another uAPI. Once iommufd_device is created we could have separate ioctls to poke its attributes individually. What'd be broken if this is not done at BIND time?

Nicolin Chen

author

2023-04-21 07:56 UTC #7 lore raw

Quoted 21 lines · Kevin

Quoted 13 lines
It is in the commit message of the cover-letter though:
https://github.com/nicolinc/iommufd/commit/5e17d270bfca2a5e3e7401d4b
f58ae53eb7a8a55
--------------------------------------------------------
Changelog
v2:
* 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]/
--------------------------------------------------------
Quoted 7 lines
Quoted 3 lines
Could you add some words why changing from passing the information in an iommufd ioctl to bind_iommufd? My gut-feeling leans toward the latter option...
Yea. Jason told me to decouple it from PCI. And merge it into a general uAPI. So I picked the BIND ioctl.

Quoted 5 lines

'decouple it from PCI' is kind of covered by renaming set_rid to set_data. but I didn't get why this has to be merged with another uAPI. Once iommufd_device is created we could have separate ioctls to poke its attributes individually. What'd be broken if this is not done at BIND time?

Oh, sorry. He didn't literally told me to merge, but commented "make sense" at my proposal of reusing BIND. So, I don't think adding to the BIND is a must here.

The BIND is done in vfio_realize() where the RID (dev_data) is available also. And the new uAPI in my v1 actually gets called near the BIND. So, I feel we may just do it once? I am open to a better idea.

Thanks
Nic

Tian, Kevin

maintainer

2023-04-21 08:08 UTC #8 lore raw

Quoted 33 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, April 21, 2023 3:56 PM
Quoted 2 lines · Kevin
Quoted 2 lines
It is in the commit message of the cover-letter though:
https://github.com/nicolinc/iommufd/commit/5e17d270bfca2a5e3e7401d4b
Quoted 19 lines
Quoted 11 lines
f58ae53eb7a8a55
--------------------------------------------------------
Changelog
v2:
* 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]/
--------------------------------------------------------
Quoted 7 lines
Quoted 3 lines
Could you add some words why changing from passing the information in an iommufd ioctl to bind_iommufd? My gut-feeling leans toward the latter option...
Yea. Jason told me to decouple it from PCI. And merge it into a general uAPI. So I picked the BIND ioctl.
Quoted 5 lines
'decouple it from PCI' is kind of covered by renaming set_rid to set_data. but I didn't get why this has to be merged with another uAPI. Once iommufd_device is created we could have separate ioctls to poke its attributes individually. What'd be broken if this is not done at BIND time?

Quoted 9 lines

Oh, sorry. He didn't literally told me to merge, but commented "make sense" at my proposal of reusing BIND. So, I don't think adding to the BIND is a must here.
The BIND is done in vfio_realize() where the RID (dev_data) is available also. And the new uAPI in my v1 actually gets called near the BIND. So, I feel we may just do it once? I am open to a better idea.

IMHO if this can be done within iommufd then that should be the choice. vfio doesn't need to know this data at all and doing so means vdpa or a 3rd driver also needs to implement similar logic in their uAPI...

Nicolin Chen

author

2023-04-21 08:20 UTC #9 lore raw

Quoted 46 lines · Kevin

External email: Use caution opening links or attachments
Quoted 33 lines
From: Nicolin Chen <[email protected]>
Sent: Friday, April 21, 2023 3:56 PM
Quoted 2 lines · Kevin
Quoted 2 lines
It is in the commit message of the cover-letter though:
https://github.com/nicolinc/iommufd/commit/5e17d270bfca2a5e3e7401d4b
Quoted 19 lines
Quoted 11 lines
f58ae53eb7a8a55
--------------------------------------------------------
Changelog
v2:
* 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]/
--------------------------------------------------------
Quoted 7 lines
Quoted 3 lines
Could you add some words why changing from passing the information in an iommufd ioctl to bind_iommufd? My gut-feeling leans toward the latter option...
Yea. Jason told me to decouple it from PCI. And merge it into a general uAPI. So I picked the BIND ioctl.
Quoted 5 lines
'decouple it from PCI' is kind of covered by renaming set_rid to set_data. but I didn't get why this has to be merged with another uAPI. Once iommufd_device is created we could have separate ioctls to poke its attributes individually. What'd be broken if this is not done at BIND time?
Quoted 9 lines
Oh, sorry. He didn't literally told me to merge, but commented "make sense" at my proposal of reusing BIND. So, I don't think adding to the BIND is a must here.
The BIND is done in vfio_realize() where the RID (dev_data) is available also. And the new uAPI in my v1 actually gets called near the BIND. So, I feel we may just do it once? I am open to a better idea.

Quoted 4 lines

IMHO if this can be done within iommufd then that should be the choice. vfio doesn't need to know this data at all and doing so means vdpa or a 3rd driver also needs to implement similar logic in their uAPI...

Reusing the VFIO ioctl is because the device is a VFIO device. But doing it within iommufd could save us a lot of efforts, as you said.

So...

+/**
+ * struct iommufd_device_set_data - ioctl(IOMMU_DEVICE_SET_DATA)
+ * @size: sizeof(struct iommufd_device_set_data)
+ * @dev_id: The device to set a device data
+ * @data_uptr: User pointer of the device user data.
+ * @data_len: Length of the device user data.
+ */
+struct iommufd_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 iommufd_device_unset_data - ioctl(IOMMU_DEVICE_UNSET_DATA)
+ * @size: sizeof(struct iommufd_device_unset_data)
+ * @dev_id: The device to unset its device data
+ */
+struct iommufd_device_unset_data {
+ __u32 size;
+ __u32 dev_id;
+};
+#define IOMMU_DEVICE_UNSET_DATA _IO(IOMMUFD_TYPE, IOMMUFD_CMD_DEVICE_UNSET_DATA)

Maybe just like this?

Thanks
Nic

Jason Gunthorpe

maintainer

2023-04-21 13:09 UTC #10 lore raw

Quoted 26 lines · Nicolin Chen

(quoted diff) +25 -0

+/**
+ * struct iommufd_device_set_data - ioctl(IOMMU_DEVICE_SET_DATA)
+ * @size: sizeof(struct iommufd_device_set_data)
+ * @dev_id: The device to set a device data
+ * @data_uptr: User pointer of the device user data.
+ * @data_len: Length of the device user data.
+ */
+struct iommufd_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 iommufd_device_unset_data - ioctl(IOMMU_DEVICE_UNSET_DATA)
+ * @size: sizeof(struct iommufd_device_unset_data)
+ * @dev_id: The device to unset its device data
+ */
+struct iommufd_device_unset_data {
+	__u32 size;
+	__u32 dev_id;
+};
+#define IOMMU_DEVICE_UNSET_DATA _IO(IOMMUFD_TYPE, IOMMUFD_CMD_DEVICE_UNSET_DATA)

Quoted 1 line

Maybe just like this?

How would the iommu_ops backing this work?

Jason

Nicolin Chen

author

2023-04-21 17:37 UTC #11 lore raw

Quoted 28 lines · Jason Gunthorpe

Quoted 26 lines · Nicolin Chen

(quoted diff) +25 -0

+/**
+ * struct iommufd_device_set_data - ioctl(IOMMU_DEVICE_SET_DATA)
+ * @size: sizeof(struct iommufd_device_set_data)
+ * @dev_id: The device to set a device data
+ * @data_uptr: User pointer of the device user data.
+ * @data_len: Length of the device user data.
+ */
+struct iommufd_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 iommufd_device_unset_data - ioctl(IOMMU_DEVICE_UNSET_DATA)
+ * @size: sizeof(struct iommufd_device_unset_data)
+ * @dev_id: The device to unset its device data
+ */
+struct iommufd_device_unset_data {
+	__u32 size;
+	__u32 dev_id;
+};
+#define IOMMU_DEVICE_UNSET_DATA _IO(IOMMUFD_TYPE, IOMMUFD_CMD_DEVICE_UNSET_DATA)

Quoted 3 lines

Quoted 1 line
Maybe just like this?
How would the iommu_ops backing this work?

How about the following piece? Needs a test with QEMU though..

static const size_t iommufd_device_data_size[] = {
	[IOMMU_HW_INFO_TYPE_NONE] = 0,
	[IOMMU_HW_INFO_TYPE_INTEL_VTD] = 0,
	[IOMMU_HW_INFO_TYPE_ARM_SMMUV3] =
		sizeof(struct iommu_device_data_arm_smmuv3),
};

int iommufd_device_set_data(struct iommufd_ucmd *ucmd)

{
	struct iommufd_device_set_data *cmd = ucmd->cmd;
	struct iommufd_device *idev;
	const struct iommu_ops *ops;
	void *data = NULL;
	u32 klen = 0;
	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);

	ops = dev_iommu_ops(idev->dev);
	if (!ops || !ops->set_dev_data_user || !ops->unset_dev_data_user ||
	    ops->hw_info_type >= ARRAY_SIZE(iommufd_device_data_size)) {
		rc = -EOPNOTSUPP;
		goto out_put_idev;
	}

	klen = iommufd_device_data_size[ops->hw_info_type];
	if (!klen) {
		rc = -EOPNOTSUPP;
		goto out_put_idev;
	}

	data = kzalloc(klen, GFP_KERNEL);
	if (!data) {
		rc = -ENOMEM;
		goto out_put_idev;
	}

	if (copy_struct_from_user(data, klen, u64_to_user_ptr(cmd->data_uptr),
				  cmd->data_len)) {
		rc = -EFAULT;
		goto out_free_data;
	}

	rc = ops->set_dev_data_user(idev->dev, data);
out_free_data:
	kfree(data);
out_put_idev:
	iommufd_put_object(&idev->obj);
	return rc;
}

Jason Gunthorpe

maintainer

2023-04-21 17:59 UTC #12 lore raw

Quoted 8 lines · Nicolin Chen

How about the following piece? Needs a test with QEMU though..

static const size_t iommufd_device_data_size[] = {
	[IOMMU_HW_INFO_TYPE_NONE] = 0,
	[IOMMU_HW_INFO_TYPE_INTEL_VTD] = 0,
	[IOMMU_HW_INFO_TYPE_ARM_SMMUV3] =
		sizeof(struct iommu_device_data_arm_smmuv3),
};

If we need more than one of these things we'll need a better solution..

Quoted 1 line

	rc = ops->set_dev_data_user(idev->dev, data);

Where will the iommu driver store the vsid to sid xarray from these arguments?

Jason

Nicolin Chen

author

2023-04-21 18:19 UTC #13 lore raw

Quoted 11 lines · Jason Gunthorpe

Quoted 8 lines · Nicolin Chen
How about the following piece? Needs a test with QEMU though..
static const size_t iommufd_device_data_size[] = {
	[IOMMU_HW_INFO_TYPE_NONE] = 0,
	[IOMMU_HW_INFO_TYPE_INTEL_VTD] = 0,
	[IOMMU_HW_INFO_TYPE_ARM_SMMUV3] =
		sizeof(struct iommu_device_data_arm_smmuv3),
};

Quoted 2 lines

If we need more than one of these things we'll need a better solution..

How about adding ops->device_data_size to store the value?

And, since we have a few size arrays in hw_pagetable.c too, perhaps a new structure in ops packing all these sizes can clean up a bit things too? For example,

static struct iommu_user_data_size arm_smmu_user_data_size = {
.device_data_size = sizeof(iommu_device_data_arm_smmuv3), .hwpt_alloc_data_size = sizeof(iommu_hwpt_alloc_arm_smmuv3), .hwpt_invalidate_data_size = sizeof(iommu_hwpt_invalidate_arm_smmuv3),
}

The hwpt_xxx_data_size might be in form of arrays for multi- HWPT_TYPE support.

Quoted 4 lines

Quoted 1 line
	rc = ops->set_dev_data_user(idev->dev, data);
Where will the iommu driver store the vsid to sid xarray from these arguments?

The ARM structure packs a vsid. For example:

static int arm_smmu_set_data(struct device *dev, const void *user_data)

{
	const struct iommufd_device_data_arm_smmuv3 *data = user_data;
	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
	struct arm_smmu_stream *stream = &master->streams[0];
	struct arm_smmu_device *smmu = master->smmu;
	u32 sid_user = data->sid;
	int ret = 0;

	if (!sid_user)
		return -EINVAL;

	ret = xa_alloc(&smmu->streams_user, &sid_user, stream,
			XA_LIMIT(sid_user, sid_user), GFP_KERNEL_ACCOUNT);
	if (ret)
		return ret;
	stream->id_user = sid_user;
	return 0;
}

Thanks
Nic

Nicolin Chen

author

2023-04-23 07:45 UTC #14 lore raw

Quoted 12 lines · Nicolin Chen

Quoted 11 lines · Jason Gunthorpe
Quoted 8 lines · Nicolin Chen
How about the following piece? Needs a test with QEMU though..
static const size_t iommufd_device_data_size[] = {
	[IOMMU_HW_INFO_TYPE_NONE] = 0,
	[IOMMU_HW_INFO_TYPE_INTEL_VTD] = 0,
	[IOMMU_HW_INFO_TYPE_ARM_SMMUV3] =
		sizeof(struct iommu_device_data_arm_smmuv3),
};

Quoted 4 lines

Quoted 2 lines
If we need more than one of these things we'll need a better solution..
How about adding ops->device_data_size to store the value?

https://lore.kernel.org/linux-iommu/[email protected]/

I sent a v3 that includes this replacing the data_size array. If it looks good, we can drop the other two data_size arrays for hwpt in the nesting series too.

Thanks
Nic