[PATCH v8 0/5] iommufd: Add iommu hardware info reporting

[PATCH v8 0/5] iommufd: Add iommu hardware info reporting

[Yi Liu]

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 implementations 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 adds the
framework for iommu hw info reporting, and adds the vtd implementation. The
complete code is available in [1].

[1] https://github.com/yiliu1765/iommufd/tree/iommufd_hw_info-v8

Change log:

v8:
 - Updated the uAPI by allowing a 0 value at the input @data_len
 - Changed to always report the kernel supported data length instead of the
   length that kernel filled in the user space buffer
 - Updated uAPI doc accordingly
 - Add one more selftest for 0 value @data_len and also check the output @data_len
   with the size kernel supports
 - Fix the usage of clear_user()
 - Rebase on top of Jason's for-next branch (base: 65aaca1 iommufd: Remove iommufd_ref_to_users())
 - Include the vtd hw_info implementation from vtd nesting series
   https://lore.kernel.org/r/20230724111335.107427-12-yi.l.liu@intel.com

v7: https://lore.kernel.org/linux-iommu/20230811071501.4126-1-yi.l.liu@intel.com/
 - Use clear_user() (Jason)
 - Add fail_nth for hw_ifo (Jason)

v6: https://lore.kernel.org/linux-iommu/20230808153510.4170-1-yi.l.liu@intel.com/
 - Add Jingqi's comment on patch 02
 - Add Baolu's r-b to patch 03
 - Address Jason's comment on patch 03

v5: https://lore.kernel.org/linux-iommu/20230803143144.200945-1-yi.l.liu@intel.com/
 - Return hw_info_type in the .hw_info op, hence drop hw_info_type field in iommu_ops (Kevin)
 - Add Jason's r-b for patch 01
 - Address coding style comments from Jason and Kevin w.r.t. patch 02, 03 and 04

v4: https://lore.kernel.org/linux-iommu/20230724105936.107042-1-yi.l.liu@intel.com/
 - 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 (3):
  iommu: Move dev_iommu_ops() to private header
  iommufd: Add IOMMU_GET_HW_INFO
  iommu/vt-d: Implement hw_info for iommu capability query

 drivers/iommu/intel/iommu.c                   | 19 +++++
 drivers/iommu/iommu-priv.h                    | 11 +++
 drivers/iommu/iommufd/device.c                | 73 ++++++++++++++++++
 drivers/iommu/iommufd/iommufd_private.h       |  1 +
 drivers/iommu/iommufd/iommufd_test.h          |  9 +++
 drivers/iommu/iommufd/main.c                  |  3 +
 drivers/iommu/iommufd/selftest.c              | 16 ++++
 include/linux/iommu.h                         | 20 +++--
 include/uapi/linux/iommufd.h                  | 74 +++++++++++++++++++
 tools/testing/selftests/iommu/iommufd.c       | 30 +++++++-
 .../selftests/iommu/iommufd_fail_nth.c        |  4 +
 tools/testing/selftests/iommu/iommufd_utils.h | 56 ++++++++++++++
 12 files changed, 304 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v8 1/5] iommu: Move dev_iommu_ops() to private header

[Yi Liu]

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>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.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 e3e3b2015854..2024a2313348 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -6,6 +6,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

[PATCH v8 2/5] iommu: Add new iommu op to get iommu hardware information

[Yi Liu]

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 and return to caller. For the driver doesn't report
hardware information, caller just uses IOMMU_HW_INFO_TYPE_NONE if a type
is required.

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        | 9 +++++++++
 include/uapi/linux/iommufd.h | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e0245aa82b75..f2d6a3989713 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -228,6 +228,14 @@ 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 the output type of this op. Type is one of
+ *           enum iommu_hw_info_type defined in include/uapi/linux/iommufd.h.
+ *           The drivers that support this op should define a unique type
+ *           in include/uapi/linux/iommufd.h. 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
@@ -257,6 +265,7 @@ struct iommu_iotlb_gather {
  */
 struct iommu_ops {
 	bool (*capable)(struct device *dev, enum iommu_cap);
+	void *(*hw_info)(struct device *dev, u32 *length, u32 *type);
 
 	/* Domain allocation and freeing by the iommu driver */
 	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 8245c01adca6..ac11ace21edb 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -370,4 +370,13 @@ 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 do not report hardware
+ *                           info
+ */
+enum iommu_hw_info_type {
+	IOMMU_HW_INFO_TYPE_NONE,
+};
 #endif
-- 
2.34.1

[PATCH v8 3/5] iommufd: Add IOMMU_GET_HW_INFO

[Yi Liu]

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 compatible 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
(a.k.a capability) for a given device. The returned data is vendor
specific, userspace needs to decode it with the structure by the output
@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.

Reviewed-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>
---
 drivers/iommu/iommufd/device.c          | 73 +++++++++++++++++++++++++
 drivers/iommu/iommufd/iommufd_private.h |  1 +
 drivers/iommu/iommufd/main.c            |  3 +
 include/uapi/linux/iommufd.h            | 42 ++++++++++++++
 4 files changed, 119 insertions(+)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 90f88c295ce0..36dff7ca3ae4 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -4,6 +4,7 @@
 #include <linux/iommufd.h>
 #include <linux/slab.h>
 #include <linux/iommu.h>
+#include <uapi/linux/iommufd.h>
 #include "../iommu-priv.h"
 
 #include "io_pagetable.h"
@@ -1119,3 +1120,75 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
 	return rc;
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_rw, IOMMUFD);
+
+int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_hw_info *cmd = ucmd->cmd;
+	void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr);
+	const struct iommu_ops *ops;
+	struct iommufd_device *idev;
+	unsigned int data_len;
+	unsigned int copy_len;
+	void *data = NULL;
+	int rc;
+
+	if (cmd->flags || cmd->__reserved)
+		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) {
+		data = ops->hw_info(idev->dev, &data_len, &cmd->out_data_type);
+		if (IS_ERR(data)) {
+			rc = PTR_ERR(data);
+			goto err_put;
+		}
+
+		/*
+		 * drivers that have hw_info callback should have a unique
+		 * iommu_hw_info_type.
+		 */
+		if (WARN_ON_ONCE(cmd->out_data_type ==
+				 IOMMU_HW_INFO_TYPE_NONE)) {
+			rc = -ENODEV;
+			goto out;
+		}
+	} else {
+		cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
+		data_len = 0;
+		data = NULL;
+	}
+
+	copy_len = min(cmd->data_len, data_len);
+	if (copy_to_user(user_ptr, data, copy_len)) {
+		rc = -EFAULT;
+		goto out;
+	}
+
+	/*
+	 * Zero the trailing bytes if the user buffer is bigger than the
+	 * data size kernel actually has.
+	 */
+	if (copy_len < cmd->data_len) {
+		if (clear_user(user_ptr + copy_len, cmd->data_len - copy_len)) {
+			rc = -EFAULT;
+			goto out;
+		}
+	}
+
+	/*
+	 * We return the length the kernel supports so userspace may know what
+	 * the kernel capability is. It could be larger than the input buffer.
+	 */
+	cmd->data_len = data_len;
+
+	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
+out:
+	kfree(data);
+err_put:
+	iommufd_put_object(&idev->obj);
+	return rc;
+}
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 5a45b8ba2e26..2c58670011fe 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -296,6 +296,7 @@ iommufd_get_device(struct iommufd_ucmd *ucmd, u32 id)
 }
 
 void iommufd_device_destroy(struct iommufd_object *obj);
+int iommufd_get_hw_info(struct iommufd_ucmd *ucmd);
 
 struct iommufd_access {
 	struct iommufd_object obj;
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 5f7e9fa45502..e71523cbd0de 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -305,6 +305,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;
@@ -337,6 +338,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 ac11ace21edb..09d5e9cff7b3 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,
 };
 
 /**
@@ -379,4 +380,45 @@ 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 a user buffer in bytes. Output the length of
+ *            data that kernel supports
+ * @data_uptr: User pointer to a user-space buffer used by the kernel to fill
+ *             the iommu type specific hardware information data
+ * @out_data_type: Output the iommu hardware info type as defined in the enum
+ *                 iommu_hw_info_type.
+ * @__reserved: Must be 0
+ *
+ * Query an iommu type specific hardware information data from an iommu behind
+ * a given device that has been bound to iommufd. This hardware info data will
+ * be used to sync capabilities between the virtual iommu and the physical
+ * iommu, e.g. a nested translation setup needs to check the hardware info, so
+ * a guest stage-1 page table can be compatible with the physical iommu.
+ *
+ * To capture an iommu type specific hardware information data, @data_uptr and
+ * its length @data_len must be provided. Trailing bytes will be zeroed if the
+ * user buffer is larger than the data that kernel has. Otherwise, kernel only
+ * fills the buffer using the given length in @data_len. If the ioctl succeeds,
+ * @data_len will be updated to the length that kernel actually supports,
+ * @out_data_type will be filled to decode the data filled in the buffer
+ * pointed by @data_uptr. Input @data_len == zero is allowed, no information
+ * data will be filled to user, but user space could get the iommu_hw_info_type
+ * filled in @out_data_type and the iommu hardware information data length
+ * supported by kernel filled in @data_len.
+ */
+struct iommu_hw_info {
+	__u32 size;
+	__u32 flags;
+	__u32 dev_id;
+	__u32 data_len;
+	__aligned_u64 data_uptr;
+	__u32 out_data_type;
+	__u32 __reserved;
+};
+#define IOMMU_GET_HW_INFO _IO(IOMMUFD_TYPE, IOMMUFD_CMD_GET_HW_INFO)
 #endif
-- 
2.34.1

[PATCH v8 4/5] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl

[Yi Liu]

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/iommufd_test.h          |  9 +++
 drivers/iommu/iommufd/selftest.c              | 16 ++++++
 tools/testing/selftests/iommu/iommufd.c       | 30 +++++++++-
 .../selftests/iommu/iommufd_fail_nth.c        |  4 ++
 tools/testing/selftests/iommu/iommufd_utils.h | 56 +++++++++++++++++++
 5 files changed, 114 insertions(+), 1 deletion(-)

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 9223ff3f23f9..90e7c5400282 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -131,6 +131,21 @@ static struct iommu_domain mock_blocking_domain = {
 	.ops = &mock_blocking_ops,
 };
 
+static void *mock_domain_hw_info(struct device *dev, u32 *length, u32 *type)
+{
+	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);
+	*type = IOMMU_HW_INFO_TYPE_SELFTEST;
+
+	return info;
+}
+
 static struct iommu_domain *mock_domain_alloc(unsigned int iommu_domain_type)
 {
 	struct mock_iommu_domain *mock;
@@ -290,6 +305,7 @@ static struct iommu_device *mock_probe_device(struct device *dev)
 static const struct iommu_ops mock_ops = {
 	.owner = THIS_MODULE,
 	.pgsize_bitmap = MOCK_IO_PAGE_SIZE,
+	.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..a068bfbe9f83 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,32 @@ TEST_F(iommufd_ioas, ioas_area_auto_destroy)
 	}
 }
 
+TEST_F(iommufd_ioas, get_hw_info)
+{
+	struct iommu_test_hw_info buffer_exact;
+	struct iommu_test_hw_info_buffer {
+		struct iommu_test_hw_info info;
+		uint64_t trailing_bytes;
+	} buffer_larger;
+
+	if (self->device_id) {
+		/* Provide a zero-size user_buffer */
+		test_cmd_get_hw_info(self->device_id, NULL, 0);
+		/* Provide a user_buffer with exact size */
+		test_cmd_get_hw_info(self->device_id, &buffer_exact, sizeof(buffer_exact));
+		/*
+		 * Provide a user_buffer with size larger than the exact size to check if
+		 * kernel zero the trailing bytes.
+		 */
+		test_cmd_get_hw_info(self->device_id, &buffer_larger, sizeof(buffer_larger));
+	} else {
+		test_err_get_hw_info(ENOENT, self->device_id,
+				     &buffer_exact, sizeof(buffer_exact));
+		test_err_get_hw_info(ENOENT, self->device_id,
+				     &buffer_larger, sizeof(buffer_larger));
+	}
+}
+
 TEST_F(iommufd_ioas, area)
 {
 	int i;
diff --git a/tools/testing/selftests/iommu/iommufd_fail_nth.c b/tools/testing/selftests/iommu/iommufd_fail_nth.c
index d4c552e56948..a220ca2a689d 100644
--- a/tools/testing/selftests/iommu/iommufd_fail_nth.c
+++ b/tools/testing/selftests/iommu/iommufd_fail_nth.c
@@ -576,6 +576,7 @@ TEST_FAIL_NTH(basic_fail_nth, access_pin_domain)
 /* device.c */
 TEST_FAIL_NTH(basic_fail_nth, device)
 {
+	struct iommu_test_hw_info info;
 	uint32_t ioas_id;
 	uint32_t ioas_id2;
 	uint32_t stdev_id;
@@ -611,6 +612,9 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 				  &idev_id))
 		return -1;
 
+	if (_test_cmd_get_hw_info(self->fd, idev_id, &info, sizeof(info)))
+		return -1;
+
 	if (_test_cmd_hwpt_alloc(self->fd, idev_id, ioas_id, &hwpt_id))
 		return -1;
 
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
index 70353e68e599..097418f2fdb3 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -348,3 +348,59 @@ static void teardown_iommufd(int fd, struct __test_metadata *_metadata)
 	})
 
 #endif
+
+/* @data can be NULL */
+static int _test_cmd_get_hw_info(int fd, __u32 device_id,
+				 void *data, size_t data_len)
+{
+	struct iommu_hw_info cmd = {
+		.size = sizeof(cmd),
+		.dev_id = device_id,
+		.data_len = data_len,
+		.data_uptr = (uint64_t)data,
+	};
+	struct iommu_test_hw_info *info = (struct iommu_test_hw_info *)data;
+	int ret;
+
+	ret = ioctl(fd, IOMMU_GET_HW_INFO, &cmd);
+	if (ret)
+		return ret;
+
+	assert(cmd.out_data_type == IOMMU_HW_INFO_TYPE_SELFTEST);
+
+	/*
+	 * The struct iommu_test_hw_info should be the one defined
+	 * by the current kernel.
+	 */
+	assert(cmd.data_len == sizeof(struct iommu_test_hw_info));
+
+	/*
+	 * Trailing bytes should be 0 if user buffer is larger than
+	 * the data that kernel reports.
+	 */
+	if (data_len > cmd.data_len) {
+		char *ptr = (char *)(data + cmd.data_len);
+		int idx = 0;
+
+		while (idx < data_len - cmd.data_len) {
+			assert(!*(ptr + idx));
+			idx++;
+		}
+	}
+
+	if (info) {
+		assert(info->test_reg == IOMMU_HW_INFO_SELFTEST_REGVAL);
+		assert(!info->flags);
+	}
+
+	return 0;
+}
+
+#define test_cmd_get_hw_info(device_id, data, data_len)         \
+	ASSERT_EQ(0, _test_cmd_get_hw_info(self->fd, device_id, \
+					   data, data_len))
+
+#define test_err_get_hw_info(_errno, device_id, data, data_len) \
+	EXPECT_ERRNO(_errno,                                    \
+		     _test_cmd_get_hw_info(self->fd, device_id, \
+					   data, data_len))
-- 
2.34.1

[PATCH v8 5/5] iommu/vt-d: Implement hw_info for iommu capability query

[Yi Liu]

Add intel_iommu_hw_info() to report cap_reg and ecap_reg information.

Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c  | 19 +++++++++++++++++++
 include/uapi/linux/iommufd.h | 23 +++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 5c8c5cdc36cf..9e6f78830ece 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -22,6 +22,7 @@
 #include <linux/spinlock.h>
 #include <linux/syscore_ops.h>
 #include <linux/tboot.h>
+#include <uapi/linux/iommufd.h>
 
 #include "iommu.h"
 #include "../dma-iommu.h"
@@ -4732,8 +4733,26 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
 }
 
+static void *intel_iommu_hw_info(struct device *dev, u32 *length, u32 *type)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+	struct iommu_hw_info_vtd *vtd;
+
+	vtd = kzalloc(sizeof(*vtd), GFP_KERNEL);
+	if (!vtd)
+		return ERR_PTR(-ENOMEM);
+
+	vtd->cap_reg = iommu->cap;
+	vtd->ecap_reg = iommu->ecap;
+	*length = sizeof(*vtd);
+	*type = IOMMU_HW_INFO_TYPE_INTEL_VTD;
+	return vtd;
+}
+
 const struct iommu_ops intel_iommu_ops = {
 	.capable		= intel_iommu_capable,
+	.hw_info		= intel_iommu_hw_info,
 	.domain_alloc		= intel_iommu_domain_alloc,
 	.probe_device		= intel_iommu_probe_device,
 	.probe_finalize		= intel_iommu_probe_finalize,
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 09d5e9cff7b3..42f024c07006 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -372,13 +372,36 @@ struct iommu_hwpt_alloc {
 };
 #define IOMMU_HWPT_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_ALLOC)
 
+/**
+ * struct iommu_hw_info_vtd - Intel VT-d hardware information
+ *
+ * @flags: Must be 0
+ * @__reserved: Must be 0
+ *
+ * @cap_reg: Value of Intel VT-d capability register defined in VT-d spec
+ *           section 11.4.2 Capability Register.
+ * @ecap_reg: Value of Intel VT-d capability register defined in VT-d spec
+ *            section 11.4.3 Extended Capability Register.
+ *
+ * User needs to understand the Intel VT-d specification to decode the
+ * register value.
+ */
+struct iommu_hw_info_vtd {
+	__u32 flags;
+	__u32 __reserved;
+	__aligned_u64 cap_reg;
+	__aligned_u64 ecap_reg;
+};
+
 /**
  * enum iommu_hw_info_type - IOMMU Hardware Info Types
  * @IOMMU_HW_INFO_TYPE_NONE: Used by the drivers that do not report hardware
  *                           info
+ * @IOMMU_HW_INFO_TYPE_INTEL_VTD: Intel VT-d iommu info type
  */
 enum iommu_hw_info_type {
 	IOMMU_HW_INFO_TYPE_NONE,
+	IOMMU_HW_INFO_TYPE_INTEL_VTD,
 };
 
 /**
-- 
2.34.1

RE: [PATCH v8 2/5] iommu: Add new iommu op to get iommu hardware information

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, August 16, 2023 8:14 PM
> 
> 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

s/difference. enum/difference, enum/

> + * @hw_info: IOMMU hardware information. The type of the returned data
> is
> + *           marked by the output type of this op. Type is one of
> + *           enum iommu_hw_info_type defined in
> include/uapi/linux/iommufd.h.
> + *           The drivers that support this op should define a unique type
> + *           in include/uapi/linux/iommufd.h. 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.

simplified as:

 @hw_info: report iommu hardware information. The data buffer returned by
           this op is allocated in the iommu driver and freed by the caller
           after use. The information type is one of enum iommu_hw_info_type
           defined in include/uapi/linux/iommufd.h.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v8 3/5] iommufd: Add IOMMU_GET_HW_INFO

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, August 16, 2023 8:14 PM
> 
> 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 compatible 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
> (a.k.a capability) for a given device. The returned data is vendor
> specific, userspace needs to decode it with the structure by the output
> @out_data_type field.

"The format of the returned data is vendor specific and must be decoded
according to @out_data_type field".

> +
> +int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
> +{
> +	struct iommu_hw_info *cmd = ucmd->cmd;
> +	void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr);
> +	const struct iommu_ops *ops;
> +	struct iommufd_device *idev;
> +	unsigned int data_len;
> +	unsigned int copy_len;
> +	void *data = NULL;
> +	int rc;
> +
> +	if (cmd->flags || cmd->__reserved)
> +		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) {
> +		data = ops->hw_info(idev->dev, &data_len, &cmd-
> >out_data_type);
> +		if (IS_ERR(data)) {
> +			rc = PTR_ERR(data);
> +			goto err_put;
> +		}
> +
> +		/*
> +		 * drivers that have hw_info callback should have a unique
> +		 * iommu_hw_info_type.
> +		 */
> +		if (WARN_ON_ONCE(cmd->out_data_type ==
> +				 IOMMU_HW_INFO_TYPE_NONE)) {
> +			rc = -ENODEV;
> +			goto out;
> +		}
> +	} else {
> +		cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
> +		data_len = 0;
> +		data = NULL;

data is already initialized as NULL.

> +
> +	/*
> +	 * We return the length the kernel supports so userspace may know
> what
> +	 * the kernel capability is. It could be larger than the input buffer.
> +	 */
> +	cmd->data_len = data_len;
> +
> +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> +out:

out_free:

> +	kfree(data);
> +err_put:

out_put: (since this is also used in the success path)

> + * To capture an iommu type specific hardware information data,
> @data_uptr and
> + * its length @data_len must be provided. Trailing bytes will be zeroed if the
> + * user buffer is larger than the data that kernel has. Otherwise, kernel only
> + * fills the buffer using the given length in @data_len. If the ioctl succeeds,
> + * @data_len will be updated to the length that kernel actually supports,
> + * @out_data_type will be filled to decode the data filled in the buffer
> + * pointed by @data_uptr. Input @data_len == zero is allowed, no
> information
> + * data will be filled to user, but user space could get the
> iommu_hw_info_type
> + * filled in @out_data_type and the iommu hardware information data
> length
> + * supported by kernel filled in @data_len.

I'd just keep "Input @data_len == zero is allowed" and remove all the
trailing words which just duplicate with the former context.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v8 4/5] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, August 16, 2023 8:14 PM
> 
> 
> +TEST_F(iommufd_ioas, get_hw_info)
> +{
> +	struct iommu_test_hw_info buffer_exact;
> +	struct iommu_test_hw_info_buffer {
> +		struct iommu_test_hw_info info;
> +		uint64_t trailing_bytes;
> +	} buffer_larger;
> +
> +	if (self->device_id) {
> +		/* Provide a zero-size user_buffer */
> +		test_cmd_get_hw_info(self->device_id, NULL, 0);
> +		/* Provide a user_buffer with exact size */
> +		test_cmd_get_hw_info(self->device_id, &buffer_exact,
> sizeof(buffer_exact));
> +		/*
> +		 * Provide a user_buffer with size larger than the exact size to
> check if
> +		 * kernel zero the trailing bytes.
> +		 */
> +		test_cmd_get_hw_info(self->device_id, &buffer_larger,
> sizeof(buffer_larger));

Do we also want to test a case where size is smaller than the exact size?

RE: [PATCH v8 5/5] iommu/vt-d: Implement hw_info for iommu capability query

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, August 16, 2023 8:14 PM
> 
> Add intel_iommu_hw_info() to report cap_reg and ecap_reg information.
> 
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v8 3/5] iommufd: Add IOMMU_GET_HW_INFO

[Nicolin Chen]

Looks like Yi's latest code has not addressed these comments.

On Thu, Aug 17, 2023 at 07:31:42AM +0000, Tian, Kevin wrote:
 
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Wednesday, August 16, 2023 8:14 PM
> >
> > 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 compatible 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
> > (a.k.a capability) for a given device. The returned data is vendor
> > specific, userspace needs to decode it with the structure by the output
> > @out_data_type field.
> 
> "The format of the returned data is vendor specific and must be decoded
> according to @out_data_type field".

Ack.

> > +int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
> > +{
> > +     struct iommu_hw_info *cmd = ucmd->cmd;
> > +     void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr);
> > +     const struct iommu_ops *ops;
> > +     struct iommufd_device *idev;
> > +     unsigned int data_len;
> > +     unsigned int copy_len;
> > +     void *data = NULL;
[..]
> > +     } else {
> > +             cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
> > +             data_len = 0;
> > +             data = NULL;
> 
> data is already initialized as NULL.

Will drop.

> > +
> > +     /*
> > +      * We return the length the kernel supports so userspace may know
> > what
> > +      * the kernel capability is. It could be larger than the input buffer.
> > +      */
> > +     cmd->data_len = data_len;
> > +
> > +     rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> > +out:
> 
> out_free:
> 
> > +     kfree(data);
> > +err_put:
> 
> out_put: (since this is also used in the success path)

Ack for both.

> > + * To capture an iommu type specific hardware information data,
> > @data_uptr and
> > + * its length @data_len must be provided. Trailing bytes will be zeroed if the
> > + * user buffer is larger than the data that kernel has. Otherwise, kernel only
> > + * fills the buffer using the given length in @data_len. If the ioctl succeeds,
> > + * @data_len will be updated to the length that kernel actually supports,
> > + * @out_data_type will be filled to decode the data filled in the buffer
> > + * pointed by @data_uptr. Input @data_len == zero is allowed, no
> > information
> > + * data will be filled to user, but user space could get the
> > iommu_hw_info_type
> > + * filled in @out_data_type and the iommu hardware information data
> > length
> > + * supported by kernel filled in @data_len.
> 
> I'd just keep "Input @data_len == zero is allowed" and remove all the
> trailing words which just duplicate with the former context.

Will do.

> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Adding this.

Thanks
Nic

Re: [PATCH v8 4/5] iommufd/selftest: Add coverage for IOMMU_GET_HW_INFO ioctl

[Nicolin Chen]

On Thu, Aug 17, 2023 at 07:32:52AM +0000, Tian, Kevin wrote:
> > +TEST_F(iommufd_ioas, get_hw_info)
> > +{
> > +     struct iommu_test_hw_info buffer_exact;
> > +     struct iommu_test_hw_info_buffer {
> > +             struct iommu_test_hw_info info;
> > +             uint64_t trailing_bytes;
> > +     } buffer_larger;
> > +
> > +     if (self->device_id) {
> > +             /* Provide a zero-size user_buffer */
> > +             test_cmd_get_hw_info(self->device_id, NULL, 0);
> > +             /* Provide a user_buffer with exact size */
> > +             test_cmd_get_hw_info(self->device_id, &buffer_exact,
> > sizeof(buffer_exact));
> > +             /*
> > +              * Provide a user_buffer with size larger than the exact size to
> > check if
> > +              * kernel zero the trailing bytes.
> > +              */
> > +             test_cmd_get_hw_info(self->device_id, &buffer_larger,
> > sizeof(buffer_larger));
> 
> Do we also want to test a case where size is smaller than the exact size?

Added one with the following diff:

diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index a068bfbe9f83..33d08600be13 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -295,10 +295,13 @@ TEST_F(iommufd_ioas, ioas_area_auto_destroy)
 TEST_F(iommufd_ioas, get_hw_info)
 {
 	struct iommu_test_hw_info buffer_exact;
-	struct iommu_test_hw_info_buffer {
+	struct iommu_test_hw_info_buffer_larger {
 		struct iommu_test_hw_info info;
 		uint64_t trailing_bytes;
 	} buffer_larger;
+	struct iommu_test_hw_info_buffer_smaller {
+		__u32 flags;
+	} buffer_smaller;
 
 	if (self->device_id) {
 		/* Provide a zero-size user_buffer */
@@ -310,6 +313,11 @@ TEST_F(iommufd_ioas, get_hw_info)
 		 * kernel zero the trailing bytes.
 		 */
 		test_cmd_get_hw_info(self->device_id, &buffer_larger, sizeof(buffer_larger));
+		/*
+		 * Provide a user_buffer with size smaller than the exact size to check if
+		 * the fields within the size range still gets updated.
+		 */
+		test_cmd_get_hw_info(self->device_id, &buffer_smaller, sizeof(buffer_smaller));
 	} else {
 		test_err_get_hw_info(ENOENT, self->device_id,
 				     &buffer_exact, sizeof(buffer_exact));
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
index 097418f2fdb3..e0753d03ecaa 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -21,6 +21,10 @@ static unsigned long BUFFER_SIZE;
 
 static unsigned long PAGE_SIZE;
 
+#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
+#define offsetofend(TYPE, MEMBER) \
+	(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
+
 /*
  * Have the kernel check the refcount on pages. I don't know why a freshly
  * mmap'd anon non-compound page starts out with a ref of 3
@@ -353,13 +357,13 @@ static void teardown_iommufd(int fd, struct __test_metadata *_metadata)
 static int _test_cmd_get_hw_info(int fd, __u32 device_id,
 				 void *data, size_t data_len)
 {
+	struct iommu_test_hw_info *info = (struct iommu_test_hw_info *)data;
 	struct iommu_hw_info cmd = {
 		.size = sizeof(cmd),
 		.dev_id = device_id,
 		.data_len = data_len,
 		.data_uptr = (uint64_t)data,
 	};
-	struct iommu_test_hw_info *info = (struct iommu_test_hw_info *)data;
 	int ret;
 
 	ret = ioctl(fd, IOMMU_GET_HW_INFO, &cmd);
@@ -389,8 +393,10 @@ static int _test_cmd_get_hw_info(int fd, __u32 device_id,
 	}
 
 	if (info) {
-		assert(info->test_reg == IOMMU_HW_INFO_SELFTEST_REGVAL);
-		assert(!info->flags);
+		if (data_len >= offsetofend(struct iommu_test_hw_info, test_reg))
+			assert(info->test_reg == IOMMU_HW_INFO_SELFTEST_REGVAL);
+		if (data_len >= offsetofend(struct iommu_test_hw_info, flags))
+			assert(!info->flags);
 	}
 
 	return 0;

Re: [PATCH v8 2/5] iommu: Add new iommu op to get iommu hardware information

[Nicolin Chen]

On Thu, Aug 17, 2023 at 07:24:44AM +0000, Tian, Kevin wrote:
> 
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Wednesday, August 16, 2023 8:14 PM
> >
> > 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
> 
> s/difference. enum/difference, enum/
> 
> > + * @hw_info: IOMMU hardware information. The type of the returned data
> > is
> > + *           marked by the output type of this op. Type is one of
> > + *           enum iommu_hw_info_type defined in
> > include/uapi/linux/iommufd.h.
> > + *           The drivers that support this op should define a unique type
> > + *           in include/uapi/linux/iommufd.h. 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.
> 
> simplified as:
> 
>  @hw_info: report iommu hardware information. The data buffer returned by
>            this op is allocated in the iommu driver and freed by the caller
>            after use. The information type is one of enum iommu_hw_info_type
>            defined in include/uapi/linux/iommufd.h.
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Done. Thanks!

RE: [PATCH v8 3/5] iommufd: Add IOMMU_GET_HW_INFO

[Liu, Yi L]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, August 18, 2023 5:08 AM
> 
> Looks like Yi's latest code has not addressed these comments.

Yeah. Not yet. In progress to incorporate them. 😊

> 
> On Thu, Aug 17, 2023 at 07:31:42AM +0000, Tian, Kevin wrote:
> 
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Wednesday, August 16, 2023 8:14 PM
> > >
> > > 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 compatible 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
> > > (a.k.a capability) for a given device. The returned data is vendor
> > > specific, userspace needs to decode it with the structure by the output
> > > @out_data_type field.
> >
> > "The format of the returned data is vendor specific and must be decoded
> > according to @out_data_type field".
> 
> Ack.
> 
> > > +int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
> > > +{
> > > +     struct iommu_hw_info *cmd = ucmd->cmd;
> > > +     void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr);
> > > +     const struct iommu_ops *ops;
> > > +     struct iommufd_device *idev;
> > > +     unsigned int data_len;
> > > +     unsigned int copy_len;
> > > +     void *data = NULL;
> [..]
> > > +     } else {
> > > +             cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
> > > +             data_len = 0;
> > > +             data = NULL;
> >
> > data is already initialized as NULL.

Probably we can set data_len = 0 and the out_data_type to _NONE is
the top as well. Any preference?

> 
> Will drop.
> 
> > > +
> > > +     /*
> > > +      * We return the length the kernel supports so userspace may know
> > > what
> > > +      * the kernel capability is. It could be larger than the input buffer.
> > > +      */
> > > +     cmd->data_len = data_len;
> > > +
> > > +     rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> > > +out:
> >
> > out_free:
> >
> > > +     kfree(data);
> > > +err_put:
> >
> > out_put: (since this is also used in the success path)
> 
> Ack for both.
> 
> > > + * To capture an iommu type specific hardware information data,
> > > @data_uptr and
> > > + * its length @data_len must be provided. Trailing bytes will be zeroed if the
> > > + * user buffer is larger than the data that kernel has. Otherwise, kernel only
> > > + * fills the buffer using the given length in @data_len. If the ioctl succeeds,
> > > + * @data_len will be updated to the length that kernel actually supports,
> > > + * @out_data_type will be filled to decode the data filled in the buffer
> > > + * pointed by @data_uptr. Input @data_len == zero is allowed, no
> > > information
> > > + * data will be filled to user, but user space could get the
> > > iommu_hw_info_type
> > > + * filled in @out_data_type and the iommu hardware information data
> > > length
> > > + * supported by kernel filled in @data_len.
> >
> > I'd just keep "Input @data_len == zero is allowed" and remove all the
> > trailing words which just duplicate with the former context.
> 
> Will do.
> 
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> Adding this.
> 
> Thanks
> Nic

Re: [PATCH v8 3/5] iommufd: Add IOMMU_GET_HW_INFO

[Nicolin Chen]

On Fri, Aug 18, 2023 at 12:04:29AM +0000, Liu, Yi L wrote:

> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Friday, August 18, 2023 5:08 AM
> >
> > Looks like Yi's latest code has not addressed these comments.
> 
> Yeah. Not yet. In progress to incorporate them. 😊

I fixed them all in my local tree. I'm finalizing with alloc_user.
Let me send my branch after that, and you can edit upon :)

> > > > +int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
> > > > +{
> > > > +     struct iommu_hw_info *cmd = ucmd->cmd;
> > > > +     void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr);
> > > > +     const struct iommu_ops *ops;
> > > > +     struct iommufd_device *idev;
> > > > +     unsigned int data_len;
> > > > +     unsigned int copy_len;
> > > > +     void *data = NULL;
> > [..]
> > > > +     } else {
> > > > +             cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
> > > > +             data_len = 0;
> > > > +             data = NULL;
> > >
> > > data is already initialized as NULL.
> 
> Probably we can set data_len = 0 and the out_data_type to _NONE is
> the top as well. Any preference?

Yea we can do that. Let me change it.

Thanks
Nic

Re: [PATCH v8 3/5] iommufd: Add IOMMU_GET_HW_INFO

[Nicolin Chen]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

On Thu, Aug 17, 2023 at 05:08:34PM -0700, Nicolin Chen wrote:
> On Fri, Aug 18, 2023 at 12:04:29AM +0000, Liu, Yi L wrote:
> 
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Friday, August 18, 2023 5:08 AM
> > >
> > > Looks like Yi's latest code has not addressed these comments.
> > 
> > Yeah. Not yet. In progress to incorporate them. 😊
> 
> I fixed them all in my local tree. I'm finalizing with alloc_user.
> Let me send my branch after that, and you can edit upon :)

https://github.com/nicolinc/iommufd/tree/wip/iommufd_alloc_user-08172023-nic
Attached two sets of git-diff for the updates that I made to the
two series. Note that I didn't make any change to the VT-d patch.

Thanks
Nic

[-- Attachment #2: diff-hw_info.patch --]
[-- Type: text/plain, Size: 6319 bytes --]

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 36dff7ca3ae4..f5836a5bdbbc 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -1144,7 +1144,7 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
 		data = ops->hw_info(idev->dev, &data_len, &cmd->out_data_type);
 		if (IS_ERR(data)) {
 			rc = PTR_ERR(data);
-			goto err_put;
+			goto out_put;
 		}
 
 		/*
@@ -1154,18 +1154,17 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
 		if (WARN_ON_ONCE(cmd->out_data_type ==
 				 IOMMU_HW_INFO_TYPE_NONE)) {
 			rc = -ENODEV;
-			goto out;
+			goto out_free;
 		}
 	} else {
 		cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
 		data_len = 0;
-		data = NULL;
 	}
 
 	copy_len = min(cmd->data_len, data_len);
 	if (copy_to_user(user_ptr, data, copy_len)) {
 		rc = -EFAULT;
-		goto out;
+		goto out_free;
 	}
 
 	/*
@@ -1175,7 +1174,7 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
 	if (copy_len < cmd->data_len) {
 		if (clear_user(user_ptr + copy_len, cmd->data_len - copy_len)) {
 			rc = -EFAULT;
-			goto out;
+			goto out_free;
 		}
 	}
 
@@ -1186,9 +1185,9 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
 	cmd->data_len = data_len;
 
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
-out:
+out_free:
 	kfree(data);
-err_put:
+out_put:
 	iommufd_put_object(&idev->obj);
 	return rc;
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f2d6a3989713..bd6a1110b294 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -228,14 +228,10 @@ 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 the output type of this op. Type is one of
- *           enum iommu_hw_info_type defined in include/uapi/linux/iommufd.h.
- *           The drivers that support this op should define a unique type
- *           in include/uapi/linux/iommufd.h. 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.
+ * @hw_info: report iommu hardware information. The data buffer returned by this
+ *           op is allocated in the iommu driver and freed by the caller after
+ *           use. The information type is one of enum iommu_hw_info_type defined
+ *           in include/uapi/linux/iommufd.h.
  * @domain_alloc: allocate iommu domain
  * @probe_device: Add device to iommu driver handling
  * @release_device: Remove device from iommu driver handling
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 42f024c07006..b4ba0c0cbab6 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -429,10 +429,7 @@ enum iommu_hw_info_type {
  * fills the buffer using the given length in @data_len. If the ioctl succeeds,
  * @data_len will be updated to the length that kernel actually supports,
  * @out_data_type will be filled to decode the data filled in the buffer
- * pointed by @data_uptr. Input @data_len == zero is allowed, no information
- * data will be filled to user, but user space could get the iommu_hw_info_type
- * filled in @out_data_type and the iommu hardware information data length
- * supported by kernel filled in @data_len.
+ * pointed by @data_uptr. Input @data_len == zero is allowed.
  */
 struct iommu_hw_info {
 	__u32 size;
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index a068bfbe9f83..33d08600be13 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -295,10 +295,13 @@ TEST_F(iommufd_ioas, ioas_area_auto_destroy)
 TEST_F(iommufd_ioas, get_hw_info)
 {
 	struct iommu_test_hw_info buffer_exact;
-	struct iommu_test_hw_info_buffer {
+	struct iommu_test_hw_info_buffer_larger {
 		struct iommu_test_hw_info info;
 		uint64_t trailing_bytes;
 	} buffer_larger;
+	struct iommu_test_hw_info_buffer_smaller {
+		__u32 flags;
+	} buffer_smaller;
 
 	if (self->device_id) {
 		/* Provide a zero-size user_buffer */
@@ -310,6 +313,11 @@ TEST_F(iommufd_ioas, get_hw_info)
 		 * kernel zero the trailing bytes.
 		 */
 		test_cmd_get_hw_info(self->device_id, &buffer_larger, sizeof(buffer_larger));
+		/*
+		 * Provide a user_buffer with size smaller than the exact size to check if
+		 * the fields within the size range still gets updated.
+		 */
+		test_cmd_get_hw_info(self->device_id, &buffer_smaller, sizeof(buffer_smaller));
 	} else {
 		test_err_get_hw_info(ENOENT, self->device_id,
 				     &buffer_exact, sizeof(buffer_exact));
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
index 097418f2fdb3..e0753d03ecaa 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -21,6 +21,10 @@ static unsigned long BUFFER_SIZE;
 
 static unsigned long PAGE_SIZE;
 
+#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
+#define offsetofend(TYPE, MEMBER) \
+	(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
+
 /*
  * Have the kernel check the refcount on pages. I don't know why a freshly
  * mmap'd anon non-compound page starts out with a ref of 3
@@ -353,13 +357,13 @@ static void teardown_iommufd(int fd, struct __test_metadata *_metadata)
 static int _test_cmd_get_hw_info(int fd, __u32 device_id,
 				 void *data, size_t data_len)
 {
+	struct iommu_test_hw_info *info = (struct iommu_test_hw_info *)data;
 	struct iommu_hw_info cmd = {
 		.size = sizeof(cmd),
 		.dev_id = device_id,
 		.data_len = data_len,
 		.data_uptr = (uint64_t)data,
 	};
-	struct iommu_test_hw_info *info = (struct iommu_test_hw_info *)data;
 	int ret;
 
 	ret = ioctl(fd, IOMMU_GET_HW_INFO, &cmd);
@@ -389,8 +393,10 @@ static int _test_cmd_get_hw_info(int fd, __u32 device_id,
 	}
 
 	if (info) {
-		assert(info->test_reg == IOMMU_HW_INFO_SELFTEST_REGVAL);
-		assert(!info->flags);
+		if (data_len >= offsetofend(struct iommu_test_hw_info, test_reg))
+			assert(info->test_reg == IOMMU_HW_INFO_SELFTEST_REGVAL);
+		if (data_len >= offsetofend(struct iommu_test_hw_info, flags))
+			assert(!info->flags);
 	}
 
 	return 0;

[-- Attachment #3: diff-alloc_user.patch --]
[-- Type: text/plain, Size: 7356 bytes --]

diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index 1d9ba7454605..3f3644375bf1 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -109,18 +109,4 @@ struct iommu_test_hw_info {
 	__u32 test_reg;
 };
 
-/* Should not be equal to any defined value in enum iommu_hwpt_type */
-#define IOMMU_HWPT_TYPE_SELFTEST		0xdead
-
-/**
- * struct iommu_hwpt_selftest
- *
- * @flags: page table entry attributes, must be 0
- * @test_type: can be either IOMMU_HW_INFO_TYPE_NONE or IOMMU_HWPT_TYPE_SELFTEST
- */
-struct iommu_hwpt_selftest {
-	__u64 flags;
-	__u32 test_type;
-};
-
 #endif
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 3cd1b17638bc..0bd7534be257 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -88,8 +88,6 @@ void iommufd_test_syz_conv_iova_id(struct iommufd_ucmd *ucmd,
 struct mock_iommu_domain {
 	struct iommu_domain domain;
 	struct xarray pfns;
-	/* mock domain test data */
-	enum iommu_hwpt_type hwpt_type;
 };
 
 enum selftest_obj_type {
@@ -150,10 +148,17 @@ static void *mock_domain_hw_info(struct device *dev, u32 *length, u32 *type)
 
 static const struct iommu_ops mock_ops;
 
+union mock_domain_alloc_data {
+	struct iommu_hwpt_default default_data;
+};
+
 static struct iommu_domain *
-__mock_domain_alloc(unsigned int iommu_domain_type,
-		    const struct iommu_hwpt_selftest *user_cfg)
+__mock_domain_alloc_default(unsigned int iommu_domain_type,
+			    const union mock_domain_alloc_data *data)
 {
+	const struct iommu_hwpt_default *user_cfg =
+		(const struct iommu_hwpt_default *)data;
+	dma_addr_t aperture_end = MOCK_APERTURE_LAST;
 	struct mock_iommu_domain *mock;
 
 	if (iommu_domain_type == IOMMU_DOMAIN_BLOCKED)
@@ -162,16 +167,21 @@ __mock_domain_alloc(unsigned int iommu_domain_type,
 	if (iommu_domain_type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
+	if (user_cfg) {
+		if (user_cfg->max_addr > MOCK_APERTURE_LAST ||
+		    user_cfg->max_addr <= MOCK_APERTURE_START)
+			return ERR_PTR(-EINVAL);
+		aperture_end = user_cfg->max_addr;
+	}
+
 	mock = kzalloc(sizeof(*mock), GFP_KERNEL);
 	if (!mock)
 		return ERR_PTR(-ENOMEM);
 	mock->domain.type = iommu_domain_type;
 	mock->domain.geometry.aperture_start = MOCK_APERTURE_START;
-	mock->domain.geometry.aperture_end = MOCK_APERTURE_LAST;
+	mock->domain.geometry.aperture_end = aperture_end;
 	mock->domain.ops = mock_ops.default_domain_ops;
 	mock->domain.pgsize_bitmap = MOCK_IO_PAGE_SIZE;
-	if (user_cfg)
-		mock->hwpt_type = user_cfg->test_type;
 	xa_init(&mock->pfns);
 	return &mock->domain;
 }
@@ -183,7 +193,7 @@ static struct iommu_domain *mock_domain_alloc(unsigned int iommu_domain_type)
 	if (iommu_domain_type != IOMMU_DOMAIN_BLOCKED &&
 	    iommu_domain_type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
-	domain = __mock_domain_alloc(iommu_domain_type, NULL);
+	domain = __mock_domain_alloc_default(iommu_domain_type, NULL);
 	if (IS_ERR(domain))
 		domain = NULL;
 	return domain;
@@ -193,27 +203,31 @@ static struct iommu_domain *mock_domain_alloc_user(struct device *dev,
 						   enum iommu_hwpt_type hwpt_type,
 						   const struct iommu_user_data *user_data)
 {
-	const size_t min_len =
-		offsetofend(struct iommu_hwpt_selftest, test_type);
+	struct iommu_domain *(*alloc_fn)(unsigned int iommu_domain_type,
+					 const union mock_domain_alloc_data *data);
 	unsigned int iommu_domain_type = IOMMU_DOMAIN_UNMANAGED;
-	struct iommu_hwpt_selftest data, *user_cfg = NULL;
-
-	if (hwpt_type != IOMMU_HWPT_TYPE_DEFAULT &&
-	    hwpt_type != IOMMU_HWPT_TYPE_SELFTEST)
+	union mock_domain_alloc_data data, *user_cfg = NULL;
+	size_t data_len, min_len;
+
+	switch (hwpt_type) {
+	case IOMMU_HWPT_TYPE_DEFAULT:
+		alloc_fn = __mock_domain_alloc_default;
+		data_len = sizeof(struct iommu_hwpt_default);
+		min_len = offsetofend(struct iommu_hwpt_default, max_addr);
+		break;
+	default:
 		return ERR_PTR(-EINVAL);
+	}
 
 	if (user_data) {
 		int rc = iommu_copy_user_data(&data, user_data,
-					      sizeof(data), min_len);
+					      data_len, min_len);
 		if (rc)
 			return ERR_PTR(rc);
-		/* Expecting test program to pass hwpt_type in test data too */
-		if (data.test_type != hwpt_type)
-			return ERR_PTR(rc);
 		user_cfg = &data;
 	}
 
-	return __mock_domain_alloc(iommu_domain_type, user_cfg);
+	return alloc_fn(iommu_domain_type, user_cfg);
 }
 
 static void mock_domain_free(struct iommu_domain *domain)
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 7eb7b88459bd..2cbe47c80c09 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -274,20 +274,31 @@ TEST_F(iommufd_ioas, ioas_destroy)
 TEST_F(iommufd_ioas, hwpt_alloc)
 {
 	const uint32_t min_data_len =
-		offsetofend(struct iommu_hwpt_selftest, test_type);
-	struct iommu_hwpt_selftest data = {
+		offsetofend(struct iommu_hwpt_default, max_addr);
+	struct iommu_hwpt_default data = {
 		.flags = 0,
-		.test_type = IOMMU_HWPT_TYPE_SELFTEST,
 	};
 	uint32_t new_hwpt_id[2] = {};
 
 	if (self->stdev_id && self->device_id) {
 		test_cmd_hwpt_alloc(self->device_id,
 				    self->ioas_id, &new_hwpt_id[0]);
+		/* Try alloc_user w/o data */
+		test_err_cmd_hwpt_alloc_user(EINVAL,
+					     self->device_id, self->ioas_id,
+					     &new_hwpt_id[1],
+					     IOMMU_HWPT_TYPE_DEFAULT,
+					     0, &data);
+		test_err_cmd_hwpt_alloc_user(EINVAL,
+					     self->device_id, self->ioas_id,
+					     &new_hwpt_id[1],
+					     IOMMU_HWPT_TYPE_DEFAULT,
+					     min_data_len, NULL);
 		test_cmd_hwpt_alloc_user(self->device_id, self->ioas_id,
 					 &new_hwpt_id[1],
-					 IOMMU_HWPT_TYPE_SELFTEST,
-					 min_data_len, &data);
+					 IOMMU_HWPT_TYPE_DEFAULT, 0, NULL);
+
+		/* Replace the auto domain with new_hwpt_id[0] */
 		test_cmd_mock_domain_replace(self->stdev_id, new_hwpt_id[0]);
 		/* hw_pagetable cannot be freed if a device is attached to it */
 		EXPECT_ERRNO(EBUSY,
@@ -302,6 +313,25 @@ TEST_F(iommufd_ioas, hwpt_alloc)
 		/* Detach from the new hw_pagetable[1] and try again */
 		test_cmd_mock_domain_replace(self->stdev_id, self->ioas_id);
 		test_ioctl_destroy(new_hwpt_id[1]);
+
+		/* Try alloc_user with data */
+		data.max_addr = MOCK_APERTURE_START - 1;
+		test_err_cmd_hwpt_alloc_user(EINVAL,
+					     self->device_id, self->ioas_id,
+					     &new_hwpt_id[1],
+					     IOMMU_HWPT_TYPE_DEFAULT,
+					     min_data_len, &data);
+		data.max_addr = (uint64_t)MOCK_APERTURE_LAST + 1;
+		test_err_cmd_hwpt_alloc_user(EINVAL,
+					     self->device_id, self->ioas_id,
+					     &new_hwpt_id[1],
+					     IOMMU_HWPT_TYPE_DEFAULT,
+					     min_data_len, &data);
+		data.max_addr = MOCK_APERTURE_LAST;
+		test_cmd_hwpt_alloc_user(self->device_id, self->ioas_id,
+					 &new_hwpt_id[1],
+					 IOMMU_HWPT_TYPE_DEFAULT,
+					 min_data_len, &data);
 	} else {
 		test_err_cmd_hwpt_alloc(ENOENT,
 					self->device_id, self->ioas_id,
@@ -309,7 +339,7 @@ TEST_F(iommufd_ioas, hwpt_alloc)
 		test_err_cmd_hwpt_alloc_user(ENOENT,
 					     self->device_id, self->ioas_id,
 					     &new_hwpt_id[1],
-					     IOMMU_HWPT_TYPE_SELFTEST,
+					     IOMMU_HWPT_TYPE_DEFAULT,
 					     min_data_len, &data);
 		test_err_mock_domain_replace(ENOENT,
 					     self->stdev_id, new_hwpt_id[0]);

Re: [PATCH v8 3/5] iommufd: Add IOMMU_GET_HW_INFO

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 12:04:29AM +0000, Liu, Yi L wrote:
> > > > +int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
> > > > +{
> > > > +     struct iommu_hw_info *cmd = ucmd->cmd;
> > > > +     void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr);
> > > > +     const struct iommu_ops *ops;
> > > > +     struct iommufd_device *idev;
> > > > +     unsigned int data_len;
> > > > +     unsigned int copy_len;
> > > > +     void *data = NULL;
> > [..]
> > > > +     } else {
> > > > +             cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
> > > > +             data_len = 0;
> > > > +             data = NULL;
> > >
> > > data is already initialized as NULL.
> 
> Probably we can set data_len = 0 and the out_data_type to _NONE is
> the top as well. Any preference?

I think it is clear to remove the variable initialization so this
branch is more explicit

Jason

Re: [PATCH v8 3/5] iommufd: Add IOMMU_GET_HW_INFO

[Nicolin Chen]

On Thu, Aug 17, 2023 at 05:21:43PM -0700, Nicolin Chen wrote:
> On Thu, Aug 17, 2023 at 05:08:34PM -0700, Nicolin Chen wrote:
> > On Fri, Aug 18, 2023 at 12:04:29AM +0000, Liu, Yi L wrote:
> > 
> > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > Sent: Friday, August 18, 2023 5:08 AM
> > > >
> > > > Looks like Yi's latest code has not addressed these comments.
> > > 
> > > Yeah. Not yet. In progress to incorporate them. 😊
> > 
> > I fixed them all in my local tree. I'm finalizing with alloc_user.
> > Let me send my branch after that, and you can edit upon :)
> 
> https://github.com/nicolinc/iommufd/tree/wip/iommufd_alloc_user-08172023-nic
> Attached two sets of git-diff for the updates that I made to the
> two series. Note that I didn't make any change to the VT-d patch.

>  	} else {
>  		cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
>  		data_len = 0;
> -		data = NULL;

Reverted this one and removed its initialization in my remote
branch, following Jason's latest email.

Thanks
Nic