diff mbox series

[v2,2/4] iommu: Add new iommu op to get iommu hardware information

Message ID 20230309075358.571567-3-yi.l.liu@intel.com (mailing list archive)
State New
Headers show
Series iommufd: Add iommu hardware info reporting | expand

Commit Message

Yi Liu March 9, 2023, 7:53 a.m. UTC 
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 oesn't report hardware information, just use
IOMMU_HW_INFO_TYPE_DEFAULT.

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

Comments

Tian, Kevin March 16, 2023, 8:16 a.m. UTC | #1 
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, March 9, 2023 3:54 PM
> @@ -222,6 +223,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
> + *           defined in include/uapi/linux/iommufd.h. The data buffer is

"The type of the returned data is marked by @driver_type".

"defined in include/uapi/linux/iommufd.h" should belong to the comment
of @driver_type

> + *           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
> @@ -246,11 +252,17 @@ 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.
> + * @driver_type: One of enum iommu_hw_info_type. This is used in the
> hw_info
> + *               reporting path. For the drivers that supports it, a unique
> + *               type should be defined. For the driver that does not support
> + *               it, this field is the IOMMU_HW_INFO_TYPE_DEFAULT that is 0.
> + *               Hence, such drivers do not need to care this field.

The meaning of "driver_type" is much broader than reporting hw_info.

let's be accurate to call it as "hw_info_type". and while we have two
separate fields for one feature where is the check enforced on whether
both are provided?

Is it simpler to return the type directly in @hw_info?

btw IOMMU_HW_INFO_TYPE_DEFAULT also sounds misleading.
'default' implies hw_info still available but in a default format.

probably it's clearer to call it IOMMU_HW_INFO_TYPE_NONE.
Baolu Lu March 16, 2023, 8:30 a.m. UTC | #2 
On 2023/3/16 16:16, Tian, Kevin wrote:
>> + *           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
>> @@ -246,11 +252,17 @@ 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.
>> + * @driver_type: One of enum iommu_hw_info_type. This is used in the
>> hw_info
>> + *               reporting path. For the drivers that supports it, a unique
>> + *               type should be defined. For the driver that does not support
>> + *               it, this field is the IOMMU_HW_INFO_TYPE_DEFAULT that is 0.
>> + *               Hence, such drivers do not need to care this field.
> The meaning of "driver_type" is much broader than reporting hw_info.
> 
> let's be accurate to call it as "hw_info_type". and while we have two
> separate fields for one feature where is the check enforced on whether
> both are provided?
> 
> Is it simpler to return the type directly in @hw_info?

If I remember correctly, the vendor iommu type and hardware info are
reported to user space separately.

Best regards,
baolu
Tian, Kevin March 17, 2023, 12:08 a.m. UTC | #3 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, March 16, 2023 4:30 PM
> 
> On 2023/3/16 16:16, Tian, Kevin wrote:
> >> + *           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
> >> @@ -246,11 +252,17 @@ 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.
> >> + * @driver_type: One of enum iommu_hw_info_type. This is used in the
> >> hw_info
> >> + *               reporting path. For the drivers that supports it, a unique
> >> + *               type should be defined. For the driver that does not support
> >> + *               it, this field is the IOMMU_HW_INFO_TYPE_DEFAULT that is 0.
> >> + *               Hence, such drivers do not need to care this field.
> > The meaning of "driver_type" is much broader than reporting hw_info.
> >
> > let's be accurate to call it as "hw_info_type". and while we have two
> > separate fields for one feature where is the check enforced on whether
> > both are provided?
> >
> > Is it simpler to return the type directly in @hw_info?
> 
> If I remember correctly, the vendor iommu type and hardware info are
> reported to user space separately.
> 

there is only one IOMMU_DEVICE_GET_HW_INFO cmd. It's written as:

	data = ops->hw_info(idev->dev, &data_len);
	copy_to_user(u64_to_user_ptr(cmd->data_ptr), data, length);
	cmd->out_data_type = ops->driver_type;
Yi Liu March 29, 2023, 9:46 a.m. UTC | #4 
> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Thursday, March 16, 2023 4:17 PM
> 
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Thursday, March 9, 2023 3:54 PM
> > @@ -222,6 +223,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
> > + *           defined in include/uapi/linux/iommufd.h. The data buffer is
> 
> "The type of the returned data is marked by @driver_type".
> 
> "defined in include/uapi/linux/iommufd.h" should belong to the comment
> of @driver_type

Sure.

> 
> > + *           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
> > @@ -246,11 +252,17 @@ 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.
> > + * @driver_type: One of enum iommu_hw_info_type. This is used in the
> > hw_info
> > + *               reporting path. For the drivers that supports it, a unique
> > + *               type should be defined. For the driver that does not support
> > + *               it, this field is the IOMMU_HW_INFO_TYPE_DEFAULT that is 0.
> > + *               Hence, such drivers do not need to care this field.
> 
> The meaning of "driver_type" is much broader than reporting hw_info.
> 
> let's be accurate to call it as "hw_info_type". and while we have two
> separate fields for one feature where is the check enforced on whether
> both are provided?

It is filled in the uapi structure by referring ops->driver_type in next
patch.

> Is it simpler to return the type directly in @hw_info?

Per the current description, if the iommu driver doesn't implement .hw_info
callback, then it will not set driver_type field neither. Then this field is 0
(IOMMU_HW_INFO_TYPE_NONE). The GET_HW_INFO ioctl in next patch
would fail as well. Under this implementation, returning the driver_type
(a.k.a hw_info_type per your comment) in the hw_info callback may be
simpler.

But I plan to update the implementation per the below remark from Jason.
The GET_HW_INFO needs to succeed even if the underlying iommu driver
does not implement hw_info callback. If so, it's still much more convenient
to get the type by referring ops->driver_type.

https://lore.kernel.org/kvm/ZAcwJSK%2F9UVI9LXu@nvidia.com/

Also, per Nic's other remark, there would be a bitmap named hwpt_types
field added to iommu_ops. Then it is also easier to referring it by
ops->hwpt_types.

https://lore.kernel.org/linux-iommu/ZArgAXMUpNjDfFgZ@Asurada-Nvidia/#t

Surely, we also have another alternative. We can enforce all the iommu
drivers to implement a minimum hw_info callback which just returns the
driver_type if it does not have driver-specific data to report to the user
yet.

> btw IOMMU_HW_INFO_TYPE_DEFAULT also sounds misleading.
> 'default' implies hw_info still available but in a default format.
> 
> probably it's clearer to call it IOMMU_HW_INFO_TYPE_NONE.

Sure. Makes sense. So _NONE means no driver specific info is
Reported back to user.

Regards,
Yi Liu
diff mbox series

Patch

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7202d8ece343..3ef84ee359d2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -15,6 +15,7 @@ 
 #include <linux/of.h>
 #include <linux/ioasid.h>
 #include <uapi/linux/iommu.h>
+#include <uapi/linux/iommufd.h>
 
 #define IOMMU_READ	(1 << 0)
 #define IOMMU_WRITE	(1 << 1)
@@ -222,6 +223,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
+ *           defined in include/uapi/linux/iommufd.h. The data buffer 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
@@ -246,11 +252,17 @@  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.
+ * @driver_type: One of enum iommu_hw_info_type. This is used in the hw_info
+ *               reporting path. For the drivers that supports it, a unique
+ *               type should be defined. For the driver that does not support
+ *               it, this field is the IOMMU_HW_INFO_TYPE_DEFAULT that 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);
@@ -279,6 +291,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 driver_type;
 	unsigned long pgsize_bitmap;
 	struct module *owner;
 };
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index ccd36acad36a..955cbef640da 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -370,4 +370,11 @@  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
+ */
+enum iommu_hw_info_type {
+	IOMMU_HW_INFO_TYPE_DEFAULT,
+};
 #endif