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

Commit Message

Yi Liu July 24, 2023, 10:59 a.m. UTC

From: Lu Baolu <[e-mail]>

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 <[e-mail]>
Co-developed-by: Nicolin Chen <[e-mail]>
Signed-off-by: Nicolin Chen <[e-mail]>
Signed-off-by: Yi Liu <[e-mail]>
---
 include/linux/iommu.h        | 16 ++++++++++++++++
 include/uapi/linux/iommufd.h |  8 ++++++++
 2 files changed, 24 insertions(+)

Comments

Tian, Kevin July 27, 2023, 7:57 a.m. UTC | #1

> From: Liu, Yi L <[e-mail]>
> 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? 

Jason Gunthorpe July 27, 2023, 2:43 p.m. UTC | #2

On Thu, Jul 27, 2023 at 07:57:57AM +0000, Tian, Kevin wrote:
> > From: Liu, Yi L <[e-mail]>
> > 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

Tian, Kevin July 28, 2023, 2:05 a.m. UTC | #3

> From: Jason Gunthorpe <[e-mail]>
> 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 <[e-mail]>
> > > 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.

Yi Liu July 31, 2023, 8:33 a.m. UTC | #4

> From: Jason Gunthorpe <[e-mail]>
> 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 <[e-mail]>
> > > 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

Jason Gunthorpe July 31, 2023, 1:45 p.m. UTC | #5

On Mon, Jul 31, 2023 at 08:33:55AM +0000, Liu, Yi L wrote:
> > From: Jason Gunthorpe <[e-mail]>
> > 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 <[e-mail]>
> > > > 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

Yi Liu Aug. 1, 2023, 3:58 a.m. UTC | #6

> From: Jason Gunthorpe <[e-mail]>
> 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 <[e-mail]>
> > > 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 <[e-mail]>
> > > > > 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/[e-mail]/
https://lore.kernel.org/linux-iommu/[e-mail]/
https://lore.kernel.org/linux-iommu/[e-mail]/
https://lore.kernel.org/linux-iommu/[e-mail]/

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

Patch

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