All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/4] iommufd: Add iommu hardware info reporting
@ 2023-07-24 10:59 Yi Liu
  2023-07-24 10:59 ` [PATCH v4 1/4] iommu: Move dev_iommu_ops() to private header Yi Liu
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Yi Liu @ 2023-07-24 10:59 UTC (permalink / raw)
  To: joro, alex.williamson, jgg, kevin.tian, robin.murphy, baolu.lu
  Cc: cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.l.liu, yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi,
	lulu, suravee.suthikulpanit, iommu, linux-kernel,
	linux-kselftest, zhenzhong.duan

iommufd gives userspace the capability to manipulate iommu subsytem.
e.g. DMA map/unmap etc. In the near future, it will support iommu nested
translation. Different platform vendors have different implementation for
the nested translation. For example, Intel VT-d supports using guest I/O
page table as the stage-1 translation table. This requires guest I/O page
table be compatible with hardware IOMMU. So before set up nested translation,
userspace needs to know the hardware iommu information to understand the
nested translation requirements.

This series reports the iommu hardware information for a given device
which has been bound to iommufd. It is preparation work for userspace to
allocate hwpt for given device. Like the nested translation support[1].

This series introduces an iommu op to report the iommu hardware info,
and an ioctl IOMMU_GET_HW_INFO is added to report such hardware info to
user. enum iommu_hw_info_type is defined to differentiate the iommu hardware
info reported to user hence user can decode them. This series only adds the
framework for iommu hw info reporting, the complete reporting path needs vendor
specific definition and driver support. The full code is available in [1]
as well.

[1] https://github.com/yiliu1765/iommufd/tree/iommufd_nesting

Change log:

v4:
 - Rename ioctl to IOMMU_GET_HW_INFO and structure to iommu_hw_info
 - Move the iommufd_get_hw_info handler to main.c
 - Place iommu_hw_info prior to iommu_hwpt_alloc
 - Update the function namings accordingly
 - Update uapi kdocs

v3: https://lore.kernel.org/linux-iommu/20230511143024.19542-1-yi.l.liu@intel.com/#t
 - Add r-b from Baolu
 - Rename IOMMU_HW_INFO_TYPE_DEFAULT to be IOMMU_HW_INFO_TYPE_NONE to
   better suit what it means
 - Let IOMMU_DEVICE_GET_HW_INFO succeed even the underlying iommu driver
   does not have driver-specific data to report per below remark.
   https://lore.kernel.org/kvm/ZAcwJSK%2F9UVI9LXu@nvidia.com/

v2: https://lore.kernel.org/linux-iommu/20230309075358.571567-1-yi.l.liu@intel.com/
 - Drop patch 05 of v1 as it is already covered by other series
 - Rename the capability info to be iommu hardware info

v1: https://lore.kernel.org/linux-iommu/20230209041642.9346-1-yi.l.liu@intel.com/

Regards,
	Yi Liu

Lu Baolu (1):
  iommu: Add new iommu op to get iommu hardware information

Nicolin Chen (1):
  iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl

Yi Liu (2):
  iommu: Move dev_iommu_ops() to private header
  iommufd: Add IOMMU_GET_HW_INFO

 drivers/iommu/iommu-priv.h                    | 11 +++
 drivers/iommu/iommufd/device.c                |  1 +
 drivers/iommu/iommufd/iommufd_test.h          |  9 +++
 drivers/iommu/iommufd/main.c                  | 76 +++++++++++++++++++
 drivers/iommu/iommufd/selftest.c              | 16 ++++
 include/linux/iommu.h                         | 27 ++++---
 include/uapi/linux/iommufd.h                  | 44 +++++++++++
 tools/testing/selftests/iommu/iommufd.c       | 17 ++++-
 tools/testing/selftests/iommu/iommufd_utils.h | 26 +++++++
 9 files changed, 215 insertions(+), 12 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 1/4] iommu: Move dev_iommu_ops() to private header
  2023-07-24 10:59 [PATCH v4 0/4] iommufd: Add iommu hardware info reporting Yi Liu
@ 2023-07-24 10:59 ` Yi Liu
  2023-07-24 17:43   ` Jason Gunthorpe
  2023-07-24 10:59 ` [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information Yi Liu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Yi Liu @ 2023-07-24 10:59 UTC (permalink / raw)
  To: joro, alex.williamson, jgg, kevin.tian, robin.murphy, baolu.lu
  Cc: cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.l.liu, yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi,
	lulu, suravee.suthikulpanit, iommu, linux-kernel,
	linux-kselftest, zhenzhong.duan

dev_iommu_ops() is essentially only used in iommu subsystem, so
move to a private header to avoid being abused by other drivers.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu-priv.h | 11 +++++++++++
 include/linux/iommu.h      | 11 -----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index 7c8011bfd153..a6e694f59f64 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -4,6 +4,17 @@
 
 #include <linux/iommu.h>
 
+static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
+{
+	/*
+	 * Assume that valid ops must be installed if iommu_probe_device()
+	 * has succeeded. The device ops are essentially for internal use
+	 * within the IOMMU subsystem itself, so we should be able to trust
+	 * ourselves not to misuse the helper.
+	 */
+	return dev->iommu->iommu_dev->ops;
+}
+
 int iommu_group_replace_domain(struct iommu_group *group,
 			       struct iommu_domain *new_domain);
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d31642596675..e0245aa82b75 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -450,17 +450,6 @@ static inline void iommu_iotlb_gather_init(struct iommu_iotlb_gather *gather)
 	};
 }
 
-static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
-{
-	/*
-	 * Assume that valid ops must be installed if iommu_probe_device()
-	 * has succeeded. The device ops are essentially for internal use
-	 * within the IOMMU subsystem itself, so we should be able to trust
-	 * ourselves not to misuse the helper.
-	 */
-	return dev->iommu->iommu_dev->ops;
-}
-
 extern int bus_iommu_probe(const struct bus_type *bus);
 extern bool iommu_present(const struct bus_type *bus);
 extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information
  2023-07-24 10:59 [PATCH v4 0/4] iommufd: Add iommu hardware info reporting Yi Liu
  2023-07-24 10:59 ` [PATCH v4 1/4] iommu: Move dev_iommu_ops() to private header Yi Liu
@ 2023-07-24 10:59 ` Yi Liu
  2023-07-27  7:57   ` Tian, Kevin
  2023-07-24 10:59 ` [PATCH v4 3/4] iommufd: Add IOMMU_GET_HW_INFO Yi Liu
  2023-07-24 10:59 ` [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl Yi Liu
  3 siblings, 1 reply; 19+ messages in thread
From: Yi Liu @ 2023-07-24 10:59 UTC (permalink / raw)
  To: joro, alex.williamson, jgg, kevin.tian, robin.murphy, baolu.lu
  Cc: cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.l.liu, yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi,
	lulu, suravee.suthikulpanit, iommu, linux-kernel,
	linux-kselftest, zhenzhong.duan

From: Lu Baolu <baolu.lu@linux.intel.com>

Introduce a new iommu op to get the IOMMU hardware capabilities for
iommufd. This information will be used by any vIOMMU driver which is
owned by userspace.

This op chooses to make the special parameters opaque to the core. This
suits the current usage model where accessing any of the IOMMU device
special parameters does require a userspace driver that matches the kernel
driver. If a need for common parameters, implemented similarly by several
drivers, arises then there's room in the design to grow a generic parameter
set as well. No wrapper API is added as it is supposed to be used by
iommufd only.

Different IOMMU hardware would have different hardware information. So the
information reported differs as well. To let the external user understand
the difference. enum iommu_hw_info_type is defined. For the iommu drivers
that are capable to report hardware information, it should have a unique
iommu_hw_info_type. The iommu_hw_info_type is stored in struct iommu_ops.
For the driver doesn't report hardware information, just use
IOMMU_HW_INFO_TYPE_NONE.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Co-developed-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 include/linux/iommu.h        | 16 ++++++++++++++++
 include/uapi/linux/iommufd.h |  8 ++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e0245aa82b75..4199e13b34e6 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/of.h>
 #include <uapi/linux/iommu.h>
+#include <uapi/linux/iommufd.h>
 
 #define IOMMU_READ	(1 << 0)
 #define IOMMU_WRITE	(1 << 1)
@@ -228,6 +229,11 @@ struct iommu_iotlb_gather {
 /**
  * struct iommu_ops - iommu ops and capabilities
  * @capable: check capability
+ * @hw_info: IOMMU hardware information. The type of the returned data is
+ *           marked by @hw_info_type. The data buffer returned by this op
+ *           is allocated in the IOMMU driver and the caller should free it
+ *           after use. Return the data buffer if success, or ERR_PTR on
+ *           failure.
  * @domain_alloc: allocate iommu domain
  * @probe_device: Add device to iommu driver handling
  * @release_device: Remove device from iommu driver handling
@@ -252,11 +258,20 @@ 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.
+ * @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
+ *                support .hw_info callback should define a unique type
+ *                in include/uapi/linux/iommufd.h. For the drivers that do
+ *                not implement .hw_info callback, this field is
+ *                IOMMU_HW_INFO_TYPE_NONE which is 0. Hence, such drivers
+ *                do not need to care this field.
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  * @owner: Driver module providing these ops
  */
 struct iommu_ops {
 	bool (*capable)(struct device *dev, enum iommu_cap);
+	void *(*hw_info)(struct device *dev, u32 *length);
 
 	/* Domain allocation and freeing by the iommu driver */
 	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
@@ -285,6 +300,7 @@ struct iommu_ops {
 	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
 
 	const struct iommu_domain_ops *default_domain_ops;
+	enum iommu_hw_info_type hw_info_type;
 	unsigned long pgsize_bitmap;
 	struct module *owner;
 };
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 8245c01adca6..1f616b0f8ae0 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -370,4 +370,12 @@ struct iommu_hwpt_alloc {
 	__u32 __reserved;
 };
 #define IOMMU_HWPT_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_ALLOC)
+
+/**
+ * enum iommu_hw_info_type - IOMMU Hardware Info Types
+ * @IOMMU_HW_INFO_TYPE_NONE: Used by the drivers that does not report hardware info
+ */
+enum iommu_hw_info_type {
+	IOMMU_HW_INFO_TYPE_NONE,
+};
 #endif
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 3/4] iommufd: Add IOMMU_GET_HW_INFO
  2023-07-24 10:59 [PATCH v4 0/4] iommufd: Add iommu hardware info reporting Yi Liu
  2023-07-24 10:59 ` [PATCH v4 1/4] iommu: Move dev_iommu_ops() to private header Yi Liu
  2023-07-24 10:59 ` [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information Yi Liu
@ 2023-07-24 10:59 ` Yi Liu
  2023-07-24 17:50   ` Jason Gunthorpe
  2023-07-24 10:59 ` [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl Yi Liu
  3 siblings, 1 reply; 19+ messages in thread
From: Yi Liu @ 2023-07-24 10:59 UTC (permalink / raw)
  To: joro, alex.williamson, jgg, kevin.tian, robin.murphy, baolu.lu
  Cc: cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.l.liu, yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi,
	lulu, suravee.suthikulpanit, iommu, linux-kernel,
	linux-kselftest, zhenzhong.duan

Under nested IOMMU translation, userspace owns the stage-1 translation
table (e.g. the stage-1 page table of Intel VT-d or the context table
of ARM SMMUv3, and etc.). Stage-1 translation tables are vendor specific,
and need to be compatiable with the underlying IOMMU hardware. Hence,
userspace should know the IOMMU hardware capability before creating and
configuring the stage-1 translation table to kernel.

This adds IOMMU_GET_HW_INFO ioctl to query the IOMMU hardware information
for a given device. The returned data is vendor specific, userspace needs
to decode it with the structure mapped by the @out_data_type field.

As only physical devices have IOMMU hardware, so this will return error
if the given device is not a physical device.

Co-developed-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/main.c | 76 ++++++++++++++++++++++++++++++++++++
 include/uapi/linux/iommufd.h | 36 +++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 94c498b8fdf6..bd3efc1d8509 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -17,6 +17,7 @@
 #include <linux/bug.h>
 #include <uapi/linux/iommufd.h>
 #include <linux/iommufd.h>
+#include "../iommu-priv.h"
 
 #include "io_pagetable.h"
 #include "iommufd_private.h"
@@ -177,6 +178,78 @@ static int iommufd_destroy(struct iommufd_ucmd *ucmd)
 	return 0;
 }
 
+static int iommufd_zero_fill_user(u64 ptr, int bytes)
+{
+	int index = 0;
+
+	for (; index < bytes; index++) {
+		if (put_user(0, (uint8_t __user *)u64_to_user_ptr(ptr + index)))
+			return -EFAULT;
+	}
+	return 0;
+}
+
+static int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_hw_info *cmd = ucmd->cmd;
+	unsigned int length = 0, data_len;
+	struct iommufd_device *idev;
+	const struct iommu_ops *ops;
+	void *data = NULL;
+	int rc = 0;
+
+	if (cmd->flags || cmd->__reserved || !cmd->data_len)
+		return -EOPNOTSUPP;
+
+	idev = iommufd_get_device(ucmd, cmd->dev_id);
+	if (IS_ERR(idev))
+		return PTR_ERR(idev);
+
+	ops = dev_iommu_ops(idev->dev);
+	if (!ops->hw_info)
+		goto done;
+
+	/* driver has hw_info callback should have a unique hw_info_type */
+	if (WARN_ON_ONCE(ops->hw_info_type == IOMMU_HW_INFO_TYPE_NONE)) {
+		pr_warn_ratelimited("iommu driver set an invalid type\n");
+		rc = -ENODEV;
+		goto out_err;
+	}
+
+	data = ops->hw_info(idev->dev, &data_len);
+	if (IS_ERR(data)) {
+		rc = PTR_ERR(data);
+		goto out_err;
+	}
+
+	length = min(cmd->data_len, data_len);
+	if (copy_to_user(u64_to_user_ptr(cmd->data_ptr), data, length)) {
+		rc = -EFAULT;
+		goto out_err;
+	}
+
+done:
+	/*
+	 * Zero the trailing bytes if the user buffer is bigger than the
+	 * data size kernel actually has.
+	 */
+	if (length < cmd->data_len) {
+		rc = iommufd_zero_fill_user(cmd->data_ptr + length,
+					    cmd->data_len - length);
+		if (rc)
+			goto out_err;
+	}
+
+	cmd->data_len = length;
+	cmd->out_data_type = ops->hw_info_type;
+	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
+
+out_err:
+	kfree(data);
+	iommufd_put_object(&idev->obj);
+	return rc;
+}
+
 static int iommufd_fops_open(struct inode *inode, struct file *filp)
 {
 	struct iommufd_ctx *ictx;
@@ -265,6 +338,7 @@ static int iommufd_option(struct iommufd_ucmd *ucmd)
 
 union ucmd_buffer {
 	struct iommu_destroy destroy;
+	struct iommu_hw_info info;
 	struct iommu_hwpt_alloc hwpt;
 	struct iommu_ioas_alloc alloc;
 	struct iommu_ioas_allow_iovas allow_iovas;
@@ -297,6 +371,8 @@ struct iommufd_ioctl_op {
 	}
 static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
 	IOCTL_OP(IOMMU_DESTROY, iommufd_destroy, struct iommu_destroy, id),
+	IOCTL_OP(IOMMU_GET_HW_INFO, iommufd_get_hw_info, struct iommu_hw_info,
+		 __reserved),
 	IOCTL_OP(IOMMU_HWPT_ALLOC, iommufd_hwpt_alloc, struct iommu_hwpt_alloc,
 		 __reserved),
 	IOCTL_OP(IOMMU_IOAS_ALLOC, iommufd_ioas_alloc_ioctl,
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 1f616b0f8ae0..4295362e7b44 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -46,6 +46,7 @@ enum {
 	IOMMUFD_CMD_OPTION,
 	IOMMUFD_CMD_VFIO_IOAS,
 	IOMMUFD_CMD_HWPT_ALLOC,
+	IOMMUFD_CMD_GET_HW_INFO,
 };
 
 /**
@@ -378,4 +379,39 @@ struct iommu_hwpt_alloc {
 enum iommu_hw_info_type {
 	IOMMU_HW_INFO_TYPE_NONE,
 };
+
+/**
+ * struct iommu_hw_info - ioctl(IOMMU_GET_HW_INFO)
+ * @size: sizeof(struct iommu_hw_info)
+ * @flags: Must be 0
+ * @dev_id: The device bound to the iommufd
+ * @data_len: Input the length of the user buffer in bytes. Output the length
+ *            of data filled in the user buffer.
+ * @data_ptr: Pointer to the user buffer
+ * @out_data_type: Output the iommu hardware info type as defined in the enum
+ *                 iommu_hw_info_type.
+ * @__reserved: Must be 0
+ *
+ * Query the hardware information from an iommu behind a given device that has
+ * been bound to iommufd. @data_len is the size of the buffer, which captures an
+ * iommu type specific input data and a filled output data. Trailing bytes will
+ * be zeroed if the user buffer is larger than the data kernel has.
+ *
+ * The type specific data would be used to sync capabilities between the virtual
+ * IOMMU and the hardware IOMMU, e.g. a nested translation setup needs to check
+ * the hardware information, so the guest stage-1 page table will be compatible.
+ *
+ * The @out_data_type will be filled if the ioctl succeeds. It would be used to
+ * decode the data filled in the buffer pointed by @data_ptr.
+ */
+struct iommu_hw_info {
+	__u32 size;
+	__u32 flags;
+	__u32 dev_id;
+	__u32 data_len;
+	__aligned_u64 data_ptr;
+	__u32 out_data_type;
+	__u32 __reserved;
+};
+#define IOMMU_GET_HW_INFO _IO(IOMMUFD_TYPE, IOMMUFD_CMD_GET_HW_INFO)
 #endif
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl
  2023-07-24 10:59 [PATCH v4 0/4] iommufd: Add iommu hardware info reporting Yi Liu
                   ` (2 preceding siblings ...)
  2023-07-24 10:59 ` [PATCH v4 3/4] iommufd: Add IOMMU_GET_HW_INFO Yi Liu
@ 2023-07-24 10:59 ` Yi Liu
  2023-07-24 17:52   ` Jason Gunthorpe
  2023-07-27  8:14   ` Tian, Kevin
  3 siblings, 2 replies; 19+ messages in thread
From: Yi Liu @ 2023-07-24 10:59 UTC (permalink / raw)
  To: joro, alex.williamson, jgg, kevin.tian, robin.murphy, baolu.lu
  Cc: cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.l.liu, yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi,
	lulu, suravee.suthikulpanit, iommu, linux-kernel,
	linux-kselftest, zhenzhong.duan

From: Nicolin Chen <nicolinc@nvidia.com>

Add a mock_domain_hw_info function and an iommu_test_hw_info data
structure. This allows to test the IOMMU_GET_HW_INFO ioctl passing
the test_reg value for the mock_dev.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/device.c                |  1 +
 drivers/iommu/iommufd/iommufd_test.h          |  9 +++++++
 drivers/iommu/iommufd/selftest.c              | 16 ++++++++++++
 tools/testing/selftests/iommu/iommufd.c       | 17 +++++++++++-
 tools/testing/selftests/iommu/iommufd_utils.h | 26 +++++++++++++++++++
 5 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 1015d6c42e2b..0deb2a2ec01a 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -8,6 +8,7 @@
 
 #include "io_pagetable.h"
 #include "iommufd_private.h"
+#include "iommufd_test.h"
 
 static bool allow_unsafe_interrupts;
 module_param(allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR);
diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index 258de2253b61..3f3644375bf1 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -100,4 +100,13 @@ struct iommu_test_cmd {
 };
 #define IOMMU_TEST_CMD _IO(IOMMUFD_TYPE, IOMMUFD_CMD_BASE + 32)
 
+/* Mock structs for IOMMU_DEVICE_GET_HW_INFO ioctl */
+#define IOMMU_HW_INFO_TYPE_SELFTEST	0xfeedbeef
+#define IOMMU_HW_INFO_SELFTEST_REGVAL	0xdeadbeef
+
+struct iommu_test_hw_info {
+	__u32 flags;
+	__u32 test_reg;
+};
+
 #endif
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index bb2cd54ca7b6..af7459e211ad 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -128,6 +128,20 @@ static struct iommu_domain mock_blocking_domain = {
 	.ops = &mock_blocking_ops,
 };
 
+static void *mock_domain_hw_info(struct device *dev, u32 *length)
+{
+	struct iommu_test_hw_info *info;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	info->test_reg = IOMMU_HW_INFO_SELFTEST_REGVAL;
+	*length = sizeof(*info);
+
+	return info;
+}
+
 static struct iommu_domain *mock_domain_alloc(unsigned int iommu_domain_type)
 {
 	struct mock_iommu_domain *mock;
@@ -279,6 +293,8 @@ static void mock_domain_set_plaform_dma_ops(struct device *dev)
 static const struct iommu_ops mock_ops = {
 	.owner = THIS_MODULE,
 	.pgsize_bitmap = MOCK_IO_PAGE_SIZE,
+	.hw_info_type = IOMMU_HW_INFO_TYPE_SELFTEST,
+	.hw_info = mock_domain_hw_info,
 	.domain_alloc = mock_domain_alloc,
 	.capable = mock_domain_capable,
 	.set_platform_dma_ops = mock_domain_set_plaform_dma_ops,
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 8acd0af37aa5..6b075a68b928 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -113,6 +113,7 @@ TEST_F(iommufd, cmd_length)
 	}
 
 	TEST_LENGTH(iommu_destroy, IOMMU_DESTROY);
+	TEST_LENGTH(iommu_hw_info, IOMMU_GET_HW_INFO);
 	TEST_LENGTH(iommu_ioas_alloc, IOMMU_IOAS_ALLOC);
 	TEST_LENGTH(iommu_ioas_iova_ranges, IOMMU_IOAS_IOVA_RANGES);
 	TEST_LENGTH(iommu_ioas_allow_iovas, IOMMU_IOAS_ALLOW_IOVAS);
@@ -185,6 +186,7 @@ FIXTURE(iommufd_ioas)
 	uint32_t ioas_id;
 	uint32_t stdev_id;
 	uint32_t hwpt_id;
+	uint32_t device_id;
 	uint64_t base_iova;
 };
 
@@ -211,7 +213,7 @@ FIXTURE_SETUP(iommufd_ioas)
 
 	for (i = 0; i != variant->mock_domains; i++) {
 		test_cmd_mock_domain(self->ioas_id, &self->stdev_id,
-				     &self->hwpt_id, NULL);
+				     &self->hwpt_id, &self->device_id);
 		self->base_iova = MOCK_APERTURE_START;
 	}
 }
@@ -290,6 +292,19 @@ TEST_F(iommufd_ioas, ioas_area_auto_destroy)
 	}
 }
 
+TEST_F(iommufd_ioas, get_hw_info)
+{
+	struct iommu_test_hw_info info;
+
+	if (self->device_id) {
+		test_cmd_get_hw_info(self->device_id, sizeof(info), &info);
+		assert(info.test_reg == IOMMU_HW_INFO_SELFTEST_REGVAL);
+	} else {
+		test_err_get_hw_info(ENOENT, self->device_id,
+				     sizeof(info), &info);
+	}
+}
+
 TEST_F(iommufd_ioas, area)
 {
 	int i;
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
index 70353e68e599..f13df84f6b42 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -348,3 +348,29 @@ static void teardown_iommufd(int fd, struct __test_metadata *_metadata)
 	})
 
 #endif
+
+static int _test_cmd_get_hw_info(int fd, __u32 device_id,
+				 __u32 data_len, void *data)
+{
+	struct iommu_hw_info cmd = {
+		.size = sizeof(cmd),
+		.dev_id = device_id,
+		.data_len = data_len,
+		.data_ptr = (uint64_t)data,
+	};
+	int ret;
+
+	ret = ioctl(fd, IOMMU_GET_HW_INFO, &cmd);
+	if (ret)
+		return ret;
+	return 0;
+}
+
+#define test_cmd_get_hw_info(device_id, data_len, data)         \
+	ASSERT_EQ(0, _test_cmd_get_hw_info(self->fd, device_id, \
+						  data_len, data))
+
+#define test_err_get_hw_info(_errno, device_id, data_len, data) \
+	EXPECT_ERRNO(_errno,                                    \
+		     _test_cmd_get_hw_info(self->fd, device_id, \
+						  data_len, data))
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/4] iommu: Move dev_iommu_ops() to private header
  2023-07-24 10:59 ` [PATCH v4 1/4] iommu: Move dev_iommu_ops() to private header Yi Liu
@ 2023-07-24 17:43   ` Jason Gunthorpe
  0 siblings, 0 replies; 19+ messages in thread
From: Jason Gunthorpe @ 2023-07-24 17:43 UTC (permalink / raw)
  To: Yi Liu
  Cc: joro, alex.williamson, kevin.tian, robin.murphy, baolu.lu,
	cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	zhenzhong.duan

On Mon, Jul 24, 2023 at 03:59:33AM -0700, Yi Liu wrote:
> dev_iommu_ops() is essentially only used in iommu subsystem, so
> move to a private header to avoid being abused by other drivers.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu-priv.h | 11 +++++++++++
>  include/linux/iommu.h      | 11 -----------
>  2 files changed, 11 insertions(+), 11 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/4] iommufd: Add IOMMU_GET_HW_INFO
  2023-07-24 10:59 ` [PATCH v4 3/4] iommufd: Add IOMMU_GET_HW_INFO Yi Liu
@ 2023-07-24 17:50   ` Jason Gunthorpe
  2023-07-31  8:35     ` Liu, Yi L
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Gunthorpe @ 2023-07-24 17:50 UTC (permalink / raw)
  To: Yi Liu
  Cc: joro, alex.williamson, kevin.tian, robin.murphy, baolu.lu,
	cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	zhenzhong.duan

On Mon, Jul 24, 2023 at 03:59:35AM -0700, Yi Liu wrote:
> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
> index 94c498b8fdf6..bd3efc1d8509 100644
> --- a/drivers/iommu/iommufd/main.c
> +++ b/drivers/iommu/iommufd/main.c
> @@ -17,6 +17,7 @@
>  #include <linux/bug.h>
>  #include <uapi/linux/iommufd.h>
>  #include <linux/iommufd.h>
> +#include "../iommu-priv.h"
>  
>  #include "io_pagetable.h"
>  #include "iommufd_private.h"
> @@ -177,6 +178,78 @@ static int iommufd_destroy(struct iommufd_ucmd *ucmd)
>  	return 0;
>  }
>  
> +static int iommufd_zero_fill_user(u64 ptr, int bytes)

(void __user * ptr, size_t bytes)

> +{
> +	int index = 0;
> +
> +	for (; index < bytes; index++) {
> +		if (put_user(0, (uint8_t __user *)u64_to_user_ptr(ptr + index)))
> +			return -EFAULT;
> +	}
> +	return 0;
> +}
> +
> +static int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
> +{
> +	struct iommu_hw_info *cmd = ucmd->cmd;
> +	unsigned int length = 0, data_len;
> +	struct iommufd_device *idev;
> +	const struct iommu_ops *ops;
> +	void *data = NULL;
> +	int rc = 0;
> +
> +	if (cmd->flags || cmd->__reserved || !cmd->data_len)
> +		return -EOPNOTSUPP;
> +
> +	idev = iommufd_get_device(ucmd, cmd->dev_id);
> +	if (IS_ERR(idev))
> +		return PTR_ERR(idev);
> +
> +	ops = dev_iommu_ops(idev->dev);
> +	if (!ops->hw_info)
> +		goto done;
> +
> +	/* driver has hw_info callback should have a unique hw_info_type */
> +	if (WARN_ON_ONCE(ops->hw_info_type == IOMMU_HW_INFO_TYPE_NONE)) {
> +		pr_warn_ratelimited("iommu driver set an invalid type\n");

Don't really need both a WARN and pr_warn(), just keep the WARN_ON

Jason

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl
  2023-07-24 10:59 ` [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl Yi Liu
@ 2023-07-24 17:52   ` Jason Gunthorpe
  2023-07-27  8:35     ` Nicolin Chen
  2023-07-27  8:14   ` Tian, Kevin
  1 sibling, 1 reply; 19+ messages in thread
From: Jason Gunthorpe @ 2023-07-24 17:52 UTC (permalink / raw)
  To: Yi Liu
  Cc: joro, alex.williamson, kevin.tian, robin.murphy, baolu.lu,
	cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	zhenzhong.duan

On Mon, Jul 24, 2023 at 03:59:36AM -0700, Yi Liu wrote:
> diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
> index 70353e68e599..f13df84f6b42 100644
> --- a/tools/testing/selftests/iommu/iommufd_utils.h
> +++ b/tools/testing/selftests/iommu/iommufd_utils.h
> @@ -348,3 +348,29 @@ static void teardown_iommufd(int fd, struct __test_metadata *_metadata)
>  	})
>  
>  #endif
> +
> +static int _test_cmd_get_hw_info(int fd, __u32 device_id,
> +				 __u32 data_len, void *data)

void * data,size_t data_len

Jason

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information
  2023-07-24 10:59 ` [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information Yi Liu
@ 2023-07-27  7:57   ` Tian, Kevin
  2023-07-27 14:43     ` Jason Gunthorpe
  0 siblings, 1 reply; 19+ messages in thread
From: Tian, Kevin @ 2023-07-27  7:57 UTC (permalink / raw)
  To: Liu, Yi L, joro, alex.williamson, jgg, robin.murphy, baolu.lu
  Cc: cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	Duan, Zhenzhong

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, July 24, 2023 7:00 PM
> 
> @@ -252,11 +258,20 @@ 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.
> + * @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
> + *                support .hw_info callback should define a unique type
> + *                in include/uapi/linux/iommufd.h. For the drivers that do
> + *                not implement .hw_info callback, this field is
> + *                IOMMU_HW_INFO_TYPE_NONE which is 0. Hence, such drivers
> + *                do not need to care this field.

every time looking at this field the same question came out why it is required
(and looks I forgot your previous response).

e.g. why cannot the type be returned in @hw_info():

	void *(*hw_info)(struct device *dev, u32 *length, int *type);

NULL callback implies IOMMU_HW_INFO_TYPE_NONE.

otherwise if there is a reason could you update the commit msg to reflect
it so I don't need to ask again next time? 😊

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl
  2023-07-24 10:59 ` [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl Yi Liu
  2023-07-24 17:52   ` Jason Gunthorpe
@ 2023-07-27  8:14   ` Tian, Kevin
  2023-07-27  8:38     ` Nicolin Chen
  1 sibling, 1 reply; 19+ messages in thread
From: Tian, Kevin @ 2023-07-27  8:14 UTC (permalink / raw)
  To: Liu, Yi L, joro, alex.williamson, jgg, robin.murphy, baolu.lu
  Cc: cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	Duan, Zhenzhong

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, July 24, 2023 7:00 PM
> 
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -8,6 +8,7 @@
> 
>  #include "io_pagetable.h"
>  #include "iommufd_private.h"
> +#include "iommufd_test.h"

Is it stale?

> @@ -211,7 +213,7 @@ FIXTURE_SETUP(iommufd_ioas)
> 
>  	for (i = 0; i != variant->mock_domains; i++) {
>  		test_cmd_mock_domain(self->ioas_id, &self->stdev_id,
> -				     &self->hwpt_id, NULL);
> +				     &self->hwpt_id, &self->device_id);
>  		self->base_iova = MOCK_APERTURE_START;
>  	}
>  }
> @@ -290,6 +292,19 @@ TEST_F(iommufd_ioas, ioas_area_auto_destroy)
>  	}
>  }
> 
> +TEST_F(iommufd_ioas, get_hw_info)
> +{
> +	struct iommu_test_hw_info info;
> +
> +	if (self->device_id) {
> +		test_cmd_get_hw_info(self->device_id, sizeof(info), &info);
> +		assert(info.test_reg ==
> IOMMU_HW_INFO_SELFTEST_REGVAL);
> +	} else {
> +		test_err_get_hw_info(ENOENT, self->device_id,
> +				     sizeof(info), &info);
> +	}

If self->device_id is invalid it should be reported right after
test_cmd_mock_domain()?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl
  2023-07-24 17:52   ` Jason Gunthorpe
@ 2023-07-27  8:35     ` Nicolin Chen
  0 siblings, 0 replies; 19+ messages in thread
From: Nicolin Chen @ 2023-07-27  8:35 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Yi Liu, joro, alex.williamson, kevin.tian, robin.murphy,
	baolu.lu, cohuck, eric.auger, kvm, mjrosato, chao.p.peng,
	yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	zhenzhong.duan

On Mon, Jul 24, 2023 at 02:52:14PM -0300, Jason Gunthorpe wrote:
> On Mon, Jul 24, 2023 at 03:59:36AM -0700, Yi Liu wrote:
> > diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
> > index 70353e68e599..f13df84f6b42 100644
> > --- a/tools/testing/selftests/iommu/iommufd_utils.h
> > +++ b/tools/testing/selftests/iommu/iommufd_utils.h
> > @@ -348,3 +348,29 @@ static void teardown_iommufd(int fd, struct __test_metadata *_metadata)
> >  	})
> >  
> >  #endif
> > +
> > +static int _test_cmd_get_hw_info(int fd, __u32 device_id,
> > +				 __u32 data_len, void *data)
> 
> void * data,size_t data_len

OK!

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl
  2023-07-27  8:14   ` Tian, Kevin
@ 2023-07-27  8:38     ` Nicolin Chen
  2023-07-27  9:14       ` Tian, Kevin
  0 siblings, 1 reply; 19+ messages in thread
From: Nicolin Chen @ 2023-07-27  8:38 UTC (permalink / raw)
  To: Tian, Kevin
  Cc: Liu, Yi L, joro, alex.williamson, jgg, robin.murphy, baolu.lu,
	cohuck, eric.auger, kvm, mjrosato, chao.p.peng, yi.y.sun, peterx,
	jasowang, shameerali.kolothum.thodi, lulu, suravee.suthikulpanit,
	iommu, linux-kernel, linux-kselftest, Duan, Zhenzhong

Hi Kevin,

On Thu, Jul 27, 2023 at 08:14:45AM +0000, Tian, Kevin wrote:
 
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Monday, July 24, 2023 7:00 PM
> >
> > --- a/drivers/iommu/iommufd/device.c
> > +++ b/drivers/iommu/iommufd/device.c
> > @@ -8,6 +8,7 @@
> >
> >  #include "io_pagetable.h"
> >  #include "iommufd_private.h"
> > +#include "iommufd_test.h"
> 
> Is it stale?

Ah, should've dropped it.

> > @@ -211,7 +213,7 @@ FIXTURE_SETUP(iommufd_ioas)
> >
> >       for (i = 0; i != variant->mock_domains; i++) {
> >               test_cmd_mock_domain(self->ioas_id, &self->stdev_id,
> > -                                  &self->hwpt_id, NULL);
> > +                                  &self->hwpt_id, &self->device_id);
> >               self->base_iova = MOCK_APERTURE_START;
> >       }
> >  }
> > @@ -290,6 +292,19 @@ TEST_F(iommufd_ioas, ioas_area_auto_destroy)
> >       }
> >  }
> >
> > +TEST_F(iommufd_ioas, get_hw_info)
> > +{
> > +     struct iommu_test_hw_info info;
> > +
> > +     if (self->device_id) {
> > +             test_cmd_get_hw_info(self->device_id, sizeof(info), &info);
> > +             assert(info.test_reg ==
> > IOMMU_HW_INFO_SELFTEST_REGVAL);
> > +     } else {
> > +             test_err_get_hw_info(ENOENT, self->device_id,
> > +                                  sizeof(info), &info);
> > +     }
> 
> If self->device_id is invalid it should be reported right after
> test_cmd_mock_domain()?

A device_id is created per mock_domain. And mock_domain is a
variant that could be 0, so a device_id being 0 could be a
normal case. Here the invalid test is for negative coverage.

Thanks!
Nic

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl
  2023-07-27  8:38     ` Nicolin Chen
@ 2023-07-27  9:14       ` Tian, Kevin
  0 siblings, 0 replies; 19+ messages in thread
From: Tian, Kevin @ 2023-07-27  9:14 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: Liu, Yi L, joro, alex.williamson, jgg, robin.murphy, baolu.lu,
	cohuck, eric.auger, kvm, mjrosato, chao.p.peng, yi.y.sun, peterx,
	jasowang, shameerali.kolothum.thodi, lulu, suravee.suthikulpanit,
	iommu, linux-kernel, linux-kselftest, Duan, Zhenzhong

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, July 27, 2023 4:38 PM
> > >
> > > +TEST_F(iommufd_ioas, get_hw_info)
> > > +{
> > > +     struct iommu_test_hw_info info;
> > > +
> > > +     if (self->device_id) {
> > > +             test_cmd_get_hw_info(self->device_id, sizeof(info), &info);
> > > +             assert(info.test_reg ==
> > > IOMMU_HW_INFO_SELFTEST_REGVAL);
> > > +     } else {
> > > +             test_err_get_hw_info(ENOENT, self->device_id,
> > > +                                  sizeof(info), &info);
> > > +     }
> >
> > If self->device_id is invalid it should be reported right after
> > test_cmd_mock_domain()?
> 
> A device_id is created per mock_domain. And mock_domain is a
> variant that could be 0, so a device_id being 0 could be a
> normal case. Here the invalid test is for negative coverage.
> 

I see. thanks

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information
  2023-07-27  7:57   ` Tian, Kevin
@ 2023-07-27 14:43     ` Jason Gunthorpe
  2023-07-28  2:05       ` Tian, Kevin
  2023-07-31  8:33       ` Liu, Yi L
  0 siblings, 2 replies; 19+ messages in thread
From: Jason Gunthorpe @ 2023-07-27 14:43 UTC (permalink / raw)
  To: Tian, Kevin
  Cc: Liu, Yi L, joro, alex.williamson, robin.murphy, baolu.lu, cohuck,
	eric.auger, nicolinc, kvm, mjrosato, chao.p.peng, yi.y.sun,
	peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	Duan, Zhenzhong

On Thu, Jul 27, 2023 at 07:57:57AM +0000, Tian, Kevin wrote:
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Monday, July 24, 2023 7:00 PM
> > 
> > @@ -252,11 +258,20 @@ 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.
> > + * @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
> > + *                support .hw_info callback should define a unique type
> > + *                in include/uapi/linux/iommufd.h. For the drivers that do
> > + *                not implement .hw_info callback, this field is
> > + *                IOMMU_HW_INFO_TYPE_NONE which is 0. Hence, such drivers
> > + *                do not need to care this field.
> 
> every time looking at this field the same question came out why it is required
> (and looks I forgot your previous response).
> 
> e.g. why cannot the type be returned in @hw_info():
> 
> 	void *(*hw_info)(struct device *dev, u32 *length, int *type);

u32 *type
 
> NULL callback implies IOMMU_HW_INFO_TYPE_NONE.

If every one of these queries has its own type it makes sense

Though, is it not possible that we can have a type for the entire
driver?

Jason

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information
  2023-07-27 14:43     ` Jason Gunthorpe
@ 2023-07-28  2:05       ` Tian, Kevin
  2023-07-31  8:33       ` Liu, Yi L
  1 sibling, 0 replies; 19+ messages in thread
From: Tian, Kevin @ 2023-07-28  2:05 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Liu, Yi L, joro, alex.williamson, robin.murphy, baolu.lu, cohuck,
	eric.auger, nicolinc, kvm, mjrosato, chao.p.peng, yi.y.sun,
	peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	Duan, Zhenzhong

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, July 27, 2023 10:43 PM
> 
> On Thu, Jul 27, 2023 at 07:57:57AM +0000, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Monday, July 24, 2023 7:00 PM
> > >
> > > @@ -252,11 +258,20 @@ 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.
> > > + * @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
> > > + *                support .hw_info callback should define a unique type
> > > + *                in include/uapi/linux/iommufd.h. For the drivers that do
> > > + *                not implement .hw_info callback, this field is
> > > + *                IOMMU_HW_INFO_TYPE_NONE which is 0. Hence, such
> drivers
> > > + *                do not need to care this field.
> >
> > every time looking at this field the same question came out why it is
> required
> > (and looks I forgot your previous response).
> >
> > e.g. why cannot the type be returned in @hw_info():
> >
> > 	void *(*hw_info)(struct device *dev, u32 *length, int *type);
> 
> u32 *type
> 
> > NULL callback implies IOMMU_HW_INFO_TYPE_NONE.
> 
> If every one of these queries has its own type it makes sense
> 
> Though, is it not possible that we can have a type for the entire
> driver?
> 

sure. I just prefer to introducing new field only when it's strictly
necessary.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information
  2023-07-27 14:43     ` Jason Gunthorpe
  2023-07-28  2:05       ` Tian, Kevin
@ 2023-07-31  8:33       ` Liu, Yi L
  2023-07-31 13:45         ` Jason Gunthorpe
  1 sibling, 1 reply; 19+ messages in thread
From: Liu, Yi L @ 2023-07-31  8:33 UTC (permalink / raw)
  To: Jason Gunthorpe, Tian, Kevin
  Cc: joro, alex.williamson, robin.murphy, baolu.lu, cohuck,
	eric.auger, nicolinc, kvm, mjrosato, chao.p.peng, yi.y.sun,
	peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	Duan, Zhenzhong

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, July 27, 2023 10:43 PM
> 
> On Thu, Jul 27, 2023 at 07:57:57AM +0000, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Monday, July 24, 2023 7:00 PM
> > >
> > > @@ -252,11 +258,20 @@ 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.
> > > + * @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
> > > + *                support .hw_info callback should define a unique type
> > > + *                in include/uapi/linux/iommufd.h. For the drivers that do
> > > + *                not implement .hw_info callback, this field is
> > > + *                IOMMU_HW_INFO_TYPE_NONE which is 0. Hence, such drivers
> > > + *                do not need to care this field.
> >
> > every time looking at this field the same question came out why it is required
> > (and looks I forgot your previous response).
> >

The major reason is that not every driver implements the hw_info
callback.

> > e.g. why cannot the type be returned in @hw_info():
> >
> > 	void *(*hw_info)(struct device *dev, u32 *length, int *type);
> 
> u32 *type
> 
> > NULL callback implies IOMMU_HW_INFO_TYPE_NONE.
> 
> If every one of these queries has its own type it makes sense
> 
> Though, is it not possible that we can have a type for the entire
> driver?

Not quite sure if I got your point. Is it acceptable to define the
callabck in the current version? or Kevin's suggestion makes
more sense?

Regards,
Yi Liu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v4 3/4] iommufd: Add IOMMU_GET_HW_INFO
  2023-07-24 17:50   ` Jason Gunthorpe
@ 2023-07-31  8:35     ` Liu, Yi L
  0 siblings, 0 replies; 19+ messages in thread
From: Liu, Yi L @ 2023-07-31  8:35 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: joro, alex.williamson, Tian, Kevin, robin.murphy, baolu.lu,
	cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	Duan, Zhenzhong

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, July 25, 2023 1:50 AM
> 
> On Mon, Jul 24, 2023 at 03:59:35AM -0700, Yi Liu wrote:
> > diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
> > index 94c498b8fdf6..bd3efc1d8509 100644
> > --- a/drivers/iommu/iommufd/main.c
> > +++ b/drivers/iommu/iommufd/main.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/bug.h>
> >  #include <uapi/linux/iommufd.h>
> >  #include <linux/iommufd.h>
> > +#include "../iommu-priv.h"
> >
> >  #include "io_pagetable.h"
> >  #include "iommufd_private.h"
> > @@ -177,6 +178,78 @@ static int iommufd_destroy(struct iommufd_ucmd *ucmd)
> >  	return 0;
> >  }
> >
> > +static int iommufd_zero_fill_user(u64 ptr, int bytes)
> 
> (void __user * ptr, size_t bytes)
> 
> > +{
> > +	int index = 0;
> > +
> > +	for (; index < bytes; index++) {
> > +		if (put_user(0, (uint8_t __user *)u64_to_user_ptr(ptr + index)))
> > +			return -EFAULT;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
> > +{
> > +	struct iommu_hw_info *cmd = ucmd->cmd;
> > +	unsigned int length = 0, data_len;
> > +	struct iommufd_device *idev;
> > +	const struct iommu_ops *ops;
> > +	void *data = NULL;
> > +	int rc = 0;
> > +
> > +	if (cmd->flags || cmd->__reserved || !cmd->data_len)
> > +		return -EOPNOTSUPP;
> > +
> > +	idev = iommufd_get_device(ucmd, cmd->dev_id);
> > +	if (IS_ERR(idev))
> > +		return PTR_ERR(idev);
> > +
> > +	ops = dev_iommu_ops(idev->dev);
> > +	if (!ops->hw_info)
> > +		goto done;
> > +
> > +	/* driver has hw_info callback should have a unique hw_info_type */
> > +	if (WARN_ON_ONCE(ops->hw_info_type == IOMMU_HW_INFO_TYPE_NONE)) {
> > +		pr_warn_ratelimited("iommu driver set an invalid type\n");
> 
> Don't really need both a WARN and pr_warn(), just keep the WARN_ON

Above two comments well received.

Thanks,
Yi Liu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information
  2023-07-31  8:33       ` Liu, Yi L
@ 2023-07-31 13:45         ` Jason Gunthorpe
  2023-08-01  3:58           ` Liu, Yi L
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Gunthorpe @ 2023-07-31 13:45 UTC (permalink / raw)
  To: Liu, Yi L
  Cc: Tian, Kevin, joro, alex.williamson, robin.murphy, baolu.lu,
	cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	Duan, Zhenzhong

On Mon, Jul 31, 2023 at 08:33:55AM +0000, Liu, Yi L wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, July 27, 2023 10:43 PM
> > 
> > On Thu, Jul 27, 2023 at 07:57:57AM +0000, Tian, Kevin wrote:
> > > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > > Sent: Monday, July 24, 2023 7:00 PM
> > > >
> > > > @@ -252,11 +258,20 @@ 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.
> > > > + * @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
> > > > + *                support .hw_info callback should define a unique type
> > > > + *                in include/uapi/linux/iommufd.h. For the drivers that do
> > > > + *                not implement .hw_info callback, this field is
> > > > + *                IOMMU_HW_INFO_TYPE_NONE which is 0. Hence, such drivers
> > > > + *                do not need to care this field.
> > >
> > > every time looking at this field the same question came out why it is required
> > > (and looks I forgot your previous response).
> > >
> 
> The major reason is that not every driver implements the hw_info
> callback.
> 
> > > e.g. why cannot the type be returned in @hw_info():
> > >
> > > 	void *(*hw_info)(struct device *dev, u32 *length, int *type);
> > 
> > u32 *type
> > 
> > > NULL callback implies IOMMU_HW_INFO_TYPE_NONE.
> > 
> > If every one of these queries has its own type it makes sense
> > 
> > Though, is it not possible that we can have a type for the entire
> > driver?
> 
> Not quite sure if I got your point. Is it acceptable to define the
> callabck in the current version? or Kevin's suggestion makes
> more sense?

I'm trying to remember if there is a reason we need unique types for
the domain and the invalidation or if we can get bye with a single
type just for the whole iommu driver.

I suppose if we ever want to to "virtio-iommu invalidation" we'd want
to use a new type for it?

Jason

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information
  2023-07-31 13:45         ` Jason Gunthorpe
@ 2023-08-01  3:58           ` Liu, Yi L
  0 siblings, 0 replies; 19+ messages in thread
From: Liu, Yi L @ 2023-08-01  3:58 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Tian, Kevin, joro, alex.williamson, robin.murphy, baolu.lu,
	cohuck, eric.auger, nicolinc, kvm, mjrosato, chao.p.peng,
	yi.y.sun, peterx, jasowang, shameerali.kolothum.thodi, lulu,
	suravee.suthikulpanit, iommu, linux-kernel, linux-kselftest,
	Duan, Zhenzhong

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Monday, July 31, 2023 9:46 PM
> 
> On Mon, Jul 31, 2023 at 08:33:55AM +0000, Liu, Yi L wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Thursday, July 27, 2023 10:43 PM
> > >
> > > On Thu, Jul 27, 2023 at 07:57:57AM +0000, Tian, Kevin wrote:
> > > > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > > > Sent: Monday, July 24, 2023 7:00 PM
> > > > >
> > > > > @@ -252,11 +258,20 @@ 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.
> > > > > + * @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
> > > > > + *                support .hw_info callback should define a unique type
> > > > > + *                in include/uapi/linux/iommufd.h. For the drivers that do
> > > > > + *                not implement .hw_info callback, this field is
> > > > > + *                IOMMU_HW_INFO_TYPE_NONE which is 0. Hence, such drivers
> > > > > + *                do not need to care this field.
> > > >
> > > > every time looking at this field the same question came out why it is required
> > > > (and looks I forgot your previous response).
> > > >
> >
> > The major reason is that not every driver implements the hw_info
> > callback.
> >
> > > > e.g. why cannot the type be returned in @hw_info():
> > > >
> > > > 	void *(*hw_info)(struct device *dev, u32 *length, int *type);
> > >
> > > u32 *type
> > >
> > > > NULL callback implies IOMMU_HW_INFO_TYPE_NONE.
> > >
> > > If every one of these queries has its own type it makes sense
> > >
> > > Though, is it not possible that we can have a type for the entire
> > > driver?
> >
> > Not quite sure if I got your point. Is it acceptable to define the
> > callabck in the current version? or Kevin's suggestion makes
> > more sense?
> 
> I'm trying to remember if there is a reason we need unique types for
> the domain and the invalidation or if we can get bye with a single
> type just for the whole iommu driver.

I see. Seems like your comment is more related to the below patches.

https://lore.kernel.org/linux-iommu/20230724110406.107212-2-yi.l.liu@intel.com/
https://lore.kernel.org/linux-iommu/20230724110406.107212-10-yi.l.liu@intel.com/
https://lore.kernel.org/linux-iommu/20230724111335.107427-2-yi.l.liu@intel.com/
https://lore.kernel.org/linux-iommu/20230724111335.107427-8-yi.l.liu@intel.com/

I think we unique types fort the domain and invalidation.
E.g. IOMMU_HWPT_TYPE_VTD_S1. The reason is that different vendors have
different stage1 format, and require different user parameters to allocate.
So needs to define unique types.

> I suppose if we ever want to to "virtio-iommu invalidation" we'd want
> to use a new type for it?

Yes. needed in the domain allocation path as well. IIRC, there was a
discussion on whether have a general cache invalidation data structure
or not[1], and the conclusion was to have separate invalidation data
structures instead of a generic structure for all types of stage1 page tables.

[1] https://lore.kernel.org/linux-iommu/20230309134217.GA1673607@myrica/

Regards,
Yi Liu

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2023-08-01  3:59 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-24 10:59 [PATCH v4 0/4] iommufd: Add iommu hardware info reporting Yi Liu
2023-07-24 10:59 ` [PATCH v4 1/4] iommu: Move dev_iommu_ops() to private header Yi Liu
2023-07-24 17:43   ` Jason Gunthorpe
2023-07-24 10:59 ` [PATCH v4 2/4] iommu: Add new iommu op to get iommu hardware information Yi Liu
2023-07-27  7:57   ` Tian, Kevin
2023-07-27 14:43     ` Jason Gunthorpe
2023-07-28  2:05       ` Tian, Kevin
2023-07-31  8:33       ` Liu, Yi L
2023-07-31 13:45         ` Jason Gunthorpe
2023-08-01  3:58           ` Liu, Yi L
2023-07-24 10:59 ` [PATCH v4 3/4] iommufd: Add IOMMU_GET_HW_INFO Yi Liu
2023-07-24 17:50   ` Jason Gunthorpe
2023-07-31  8:35     ` Liu, Yi L
2023-07-24 10:59 ` [PATCH v4 4/4] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl Yi Liu
2023-07-24 17:52   ` Jason Gunthorpe
2023-07-27  8:35     ` Nicolin Chen
2023-07-27  8:14   ` Tian, Kevin
2023-07-27  8:38     ` Nicolin Chen
2023-07-27  9:14       ` Tian, Kevin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.