* [PATCH RFC v2 1/3] iommu: Add set/unset_dev_data_user ops
2023-04-20 7:47 [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support Nicolin Chen
@ 2023-04-20 7:47 ` Nicolin Chen
2023-04-20 7:47 ` [PATCH RFC v2 2/3] iommufd: Add iommufd_device_set_data and iommufd_device_unset_data APIs Nicolin Chen
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Nicolin Chen @ 2023-04-20 7:47 UTC (permalink / raw)
To: jgg, kevin.tian, alex.williamson
Cc: robin.murphy, eric.auger, yi.l.liu, baolu.lu, will, joro,
shameerali.kolothum.thodi, jean-philippe, kvm, iommu,
linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH RFC v2 2/3] iommufd: Add iommufd_device_set_data and iommufd_device_unset_data APIs
2023-04-20 7:47 [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support Nicolin Chen
2023-04-20 7:47 ` [PATCH RFC v2 1/3] iommu: Add set/unset_dev_data_user ops Nicolin Chen
@ 2023-04-20 7:47 ` Nicolin Chen
2023-04-20 7:47 ` [PATCH RFC v2 3/3] vfio: Add dev_data_len/uptr in struct vfio_device_bind_iommufd Nicolin Chen
2023-04-21 7:35 ` [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support Tian, Kevin
3 siblings, 0 replies; 15+ messages in thread
From: Nicolin Chen @ 2023-04-20 7:47 UTC (permalink / raw)
To: jgg, kevin.tian, alex.williamson
Cc: robin.murphy, eric.auger, yi.l.liu, baolu.lu, will, joro,
shameerali.kolothum.thodi, jean-philippe, kvm, iommu,
linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH RFC v2 3/3] vfio: Add dev_data_len/uptr in struct vfio_device_bind_iommufd
2023-04-20 7:47 [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support Nicolin Chen
2023-04-20 7:47 ` [PATCH RFC v2 1/3] iommu: Add set/unset_dev_data_user ops Nicolin Chen
2023-04-20 7:47 ` [PATCH RFC v2 2/3] iommufd: Add iommufd_device_set_data and iommufd_device_unset_data APIs Nicolin Chen
@ 2023-04-20 7:47 ` Nicolin Chen
2023-04-21 7:35 ` [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support Tian, Kevin
3 siblings, 0 replies; 15+ messages in thread
From: Nicolin Chen @ 2023-04-20 7:47 UTC (permalink / raw)
To: jgg, kevin.tian, alex.williamson
Cc: robin.murphy, eric.auger, yi.l.liu, baolu.lu, will, joro,
shameerali.kolothum.thodi, jean-philippe, kvm, iommu,
linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* RE: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-20 7:47 [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support Nicolin Chen
` (2 preceding siblings ...)
2023-04-20 7:47 ` [PATCH RFC v2 3/3] vfio: Add dev_data_len/uptr in struct vfio_device_bind_iommufd Nicolin Chen
@ 2023-04-21 7:35 ` Tian, Kevin
2023-04-21 7:41 ` Nicolin Chen
3 siblings, 1 reply; 15+ messages in thread
From: Tian, Kevin @ 2023-04-21 7:35 UTC (permalink / raw)
To: Nicolin Chen, jgg@nvidia.com, alex.williamson@redhat.com
Cc: robin.murphy@arm.com, eric.auger@redhat.com, Liu, Yi L,
baolu.lu@linux.intel.com, will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
> 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...
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 7:35 ` [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support Tian, Kevin
@ 2023-04-21 7:41 ` Nicolin Chen
2023-04-21 7:47 ` Tian, Kevin
0 siblings, 1 reply; 15+ messages in thread
From: Nicolin Chen @ 2023-04-21 7:41 UTC (permalink / raw)
To: Tian, Kevin
Cc: jgg@nvidia.com, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 7:41 ` Nicolin Chen
@ 2023-04-21 7:47 ` Tian, Kevin
2023-04-21 7:56 ` Nicolin Chen
0 siblings, 1 reply; 15+ messages in thread
From: Tian, Kevin @ 2023-04-21 7:47 UTC (permalink / raw)
To: Nicolin Chen
Cc: jgg@nvidia.com, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
> 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?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 7:47 ` Tian, Kevin
@ 2023-04-21 7:56 ` Nicolin Chen
2023-04-21 8:07 ` Tian, Kevin
0 siblings, 1 reply; 15+ messages in thread
From: Nicolin Chen @ 2023-04-21 7:56 UTC (permalink / raw)
To: Tian, Kevin
Cc: jgg@nvidia.com, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 7:56 ` Nicolin Chen
@ 2023-04-21 8:07 ` Tian, Kevin
2023-04-21 8:20 ` Nicolin Chen
0 siblings, 1 reply; 15+ messages in thread
From: Tian, Kevin @ 2023-04-21 8:07 UTC (permalink / raw)
To: Nicolin Chen
Cc: jgg@nvidia.com, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
> 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...
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 8:07 ` Tian, Kevin
@ 2023-04-21 8:20 ` Nicolin Chen
2023-04-21 13:09 ` Jason Gunthorpe
0 siblings, 1 reply; 15+ messages in thread
From: Nicolin Chen @ 2023-04-21 8:20 UTC (permalink / raw)
To: Tian, Kevin
Cc: jgg@nvidia.com, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 8:20 ` Nicolin Chen
@ 2023-04-21 13:09 ` Jason Gunthorpe
2023-04-21 17:37 ` Nicolin Chen
0 siblings, 1 reply; 15+ messages in thread
From: Jason Gunthorpe @ 2023-04-21 13:09 UTC (permalink / raw)
To: Nicolin Chen
Cc: Tian, Kevin, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 13:09 ` Jason Gunthorpe
@ 2023-04-21 17:37 ` Nicolin Chen
2023-04-21 17:59 ` Jason Gunthorpe
0 siblings, 1 reply; 15+ messages in thread
From: Nicolin Chen @ 2023-04-21 17:37 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Tian, Kevin, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
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;
}
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 17:37 ` Nicolin Chen
@ 2023-04-21 17:59 ` Jason Gunthorpe
2023-04-21 18:19 ` Nicolin Chen
0 siblings, 1 reply; 15+ messages in thread
From: Jason Gunthorpe @ 2023-04-21 17:59 UTC (permalink / raw)
To: Nicolin Chen
Cc: Tian, Kevin, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 17:59 ` Jason Gunthorpe
@ 2023-04-21 18:19 ` Nicolin Chen
2023-04-23 7:44 ` Nicolin Chen
0 siblings, 1 reply; 15+ messages in thread
From: Nicolin Chen @ 2023-04-21 18:19 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Tian, Kevin, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH RFC v2 0/3] Add set_dev_data and unset_dev_data support
2023-04-21 18:19 ` Nicolin Chen
@ 2023-04-23 7:44 ` Nicolin Chen
0 siblings, 0 replies; 15+ messages in thread
From: Nicolin Chen @ 2023-04-23 7:44 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Tian, Kevin, alex.williamson@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, Liu, Yi L, baolu.lu@linux.intel.com,
will@kernel.org, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
kvm@vger.kernel.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread