[PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Nicolin Chen]

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(-)

-- 
2.40.0

[PATCH RFC v2 1/3] iommu: Add set/unset_dev_data_user ops

[Nicolin Chen]

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 <nicolinc@nvidia.com>
---
 include/linux/iommu.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5c99aeaccd1d..c87f0f1527dd 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -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;
-- 
2.40.0

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

[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 <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/device.c | 65 ++++++++++++++++++++++++++++++++++
 include/linux/iommufd.h        |  4 +++
 2 files changed, 69 insertions(+)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 16fdf4a16643..c4e895239486 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -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)
 {
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 6752c58226d1..4b3a70faa409 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -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);
 
-- 
2.40.0

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

[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 <nicolinc@nvidia.com>
---
 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(-)

diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
index b5824179cd48..70241d9c0fa9 100644
--- a/drivers/vfio/device_cdev.c
+++ b/drivers/vfio/device_cdev.c
@@ -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;
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index 16d6aac06180..0b985ccffcbe 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -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);
 
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 46b313f8bfaf..e4bb63801472 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -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;
 };
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 17c5a1dadd08..9cec823c2829 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -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)
-- 
2.40.0

RE: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> 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.
> 
> 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...

Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Nicolin Chen]

On Fri, Apr 21, 2023 at 07:35:52AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > 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.
> >
> > 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/cover.1680762112.git.nicolinc@nvidia.com/
--------------------------------------------------------

> 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

RE: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, April 21, 2023 3:42 PM
> 
> On Fri, Apr 21, 2023 at 07:35:52AM +0000, Tian, Kevin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > 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.
> > >
> > > 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/cover.1680762112.git.nicolinc@nvidia.com/
> --------------------------------------------------------
> 
> > 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?

Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Nicolin Chen]

On Fri, Apr 21, 2023 at 07:47:13AM +0000, Tian, Kevin wrote:

> > 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/cover.1680762112.git.nicolinc@nvidia.com/
> > --------------------------------------------------------
> >
> > > 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?

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

RE: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, April 21, 2023 3:56 PM
> 
> On Fri, Apr 21, 2023 at 07:47:13AM +0000, Tian, Kevin wrote:
> 
> > > 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/cover.1680762112.git.nicolinc@nvidia.com/
> > > --------------------------------------------------------
> > >
> > > > 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?
> 
> 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...

Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Nicolin Chen]

On Fri, Apr 21, 2023 at 08:07:19AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Friday, April 21, 2023 3:56 PM
> >
> > On Fri, Apr 21, 2023 at 07:47:13AM +0000, Tian, Kevin wrote:
> >
> > > > 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/cover.1680762112.git.nicolinc@nvidia.com/
> > > > --------------------------------------------------------
> > > >
> > > > > 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?
> >
> > 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...

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

Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Jason Gunthorpe]

On Fri, Apr 21, 2023 at 01:20:13AM -0700, Nicolin Chen wrote:

> +/**
> + * 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?

How would the iommu_ops backing this work?

Jason

Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Nicolin Chen]

On Fri, Apr 21, 2023 at 10:09:35AM -0300, Jason Gunthorpe wrote:
> On Fri, Apr 21, 2023 at 01:20:13AM -0700, Nicolin Chen wrote:
> 
> > +/**
> > + * 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?
> 
> 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;
}

Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Jason Gunthorpe]

On Fri, Apr 21, 2023 at 10:37:22AM -0700, Nicolin Chen wrote:

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

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

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

Jason

Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Nicolin Chen]

On Fri, Apr 21, 2023 at 02:59:37PM -0300, Jason Gunthorpe wrote:
> On Fri, Apr 21, 2023 at 10:37:22AM -0700, Nicolin Chen wrote:
> 
> > 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..

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.

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

Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support

[Nicolin Chen]

On Fri, Apr 21, 2023 at 11:19:23AM -0700, Nicolin Chen wrote:
> On Fri, Apr 21, 2023 at 02:59:37PM -0300, Jason Gunthorpe wrote:
> > On Fri, Apr 21, 2023 at 10:37:22AM -0700, Nicolin Chen wrote:
> > 
> > > 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..
> 
> How about adding ops->device_data_size to store the value?

https://lore.kernel.org/linux-iommu/cover.1682234302.git.nicolinc@nvidia.com/

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