[v4,01/17] iommu: Add hwpt_type with user_data for domain_alloc_user op

domain_alloc_user op already accepts user flags for domain allocation,
add hwpt_type and user_data support as well. 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
is room in the design to grow a generic parameter set as well.

Define enum iommu_hwpt_type to differentiate the user parameters used by
different iommu vendor drivers in the future. For backward compatibility
and the allocations without any specified hwpt_type or user parameters,
add an IOMMU_HWPT_TYPE_DEFAULT in the list.

Also, add a struct iommu_user_data as a package of data_ptr and data_len
from an iommufd core uAPI structure, then provide a helper function for
a driver to run data length sanity and copy a defined hwpt_type specific
driver user data.

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>
---
 drivers/iommu/intel/iommu.c          |  5 ++-
 drivers/iommu/iommufd/hw_pagetable.c |  4 ++-
 drivers/iommu/iommufd/selftest.c     |  5 ++-
 include/linux/iommu.h                | 51 ++++++++++++++++++++++++++--
 include/uapi/linux/iommufd.h         |  8 +++++
 5 files changed, 67 insertions(+), 6 deletions(-)

On 2023/9/21 15:51, Yi Liu wrote:
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index 4a7c5c8fdbb4..3c8660fe9bb1 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -357,6 +357,14 @@ enum iommufd_hwpt_alloc_flags {
>   	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>   };
>   
> +/**
> + * enum iommu_hwpt_type - IOMMU HWPT Type
> + * @IOMMU_HWPT_TYPE_DEFAULT: default

How about s/default/vendor agnostic/ ?

> + */
> +enum iommu_hwpt_type {
> +	IOMMU_HWPT_TYPE_DEFAULT,
> +};
> +
>   /**
>    * struct iommu_hwpt_alloc - ioctl(IOMMU_HWPT_ALLOC)
>    * @size: sizeof(struct iommu_hwpt_alloc)

Best regards,
baolu

On Thu, Sep 21, 2023 at 08:10:58PM +0800, Baolu Lu wrote:
> On 2023/9/21 15:51, Yi Liu wrote:
> > +/**
> > + * iommu_copy_user_data - Copy iommu driver specific user space data
> > + * @dst_data: Pointer to an iommu driver specific user data that is defined in
> > + *            include/uapi/linux/iommufd.h
> > + * @src_data: Pointer to a struct iommu_user_data for user space data info
> > + * @data_len: Length of current user data structure, i.e. sizeof(struct _dst)
> > + * @min_len: Initial length of user data structure for backward compatibility.
> > + *           This should be offsetofend using the last member in the user data
> > + *           struct that was initially added to include/uapi/linux/iommufd.h
> > + */
> > +static inline int iommu_copy_user_data(void *dst_data,
> > +                                    const struct iommu_user_data *src_data,
> > +                                    size_t data_len, size_t min_len)
> > +{
> > +     if (WARN_ON(!dst_data || !src_data))
> > +             return -EINVAL;
> > +     if (src_data->len < min_len || data_len < src_data->len)
> > +             return -EINVAL;
> > +     return copy_struct_from_user(dst_data, data_len,
> > +                                  src_data->uptr, src_data->len);
> > +}
> 
> I am not sure that I understand the purpose of "min_len" correctly. It
> seems like it would always be equal to data_len?
> 
> Or, it means the minimal data length that the iommu driver requires?

Hmm, I thought I had made it quite clear with the kdoc that it's
the "initial" length (min_len) v.s. "current" length (data_len),
i.e. min_len was set when the structure was introduced and would
never change while data_len can grow if the structure introduces
a new member. E.g. after this series struct iommu_hwpt_alloc has
a min_len fixed to offsetofend(..., __reserved) but its data_len
is actually increased to offsetofend(..., data_uptr).

Perhaps we could put all min_len defines in uAPI header, like:
include/uapi/linux/gfs2_ondisk.h:442:#define LH_V1_SIZE (offsetofend(struct gfs2_log_header, lh_hash))
In this way, drivers won't need to deal with that nor have risks
of breaking ABI by changing a min_len.

Also, if we go a bit further to ease the drivers, we could do:

========================================================================================
diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index 65a363f5e81e..13234e67409c 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -147,6 +147,9 @@ struct iommu_hwpt_selftest {
 	__u32 iotlb;
 };
 
+#define iommu_hwpt_selftest_min_len \
+	(offsetofend(struct iommu_hwpt_selftest, iotlb))
+
 /**
  * struct iommu_hwpt_invalidate_selftest
  *
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 117776d236dc..2cc3a8a3355b 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -263,8 +263,8 @@ mock_domain_alloc_user(struct device *dev, u32 flags,
 	}
 
 	if (user_data) {
-		int rc = iommu_copy_user_data(&data, user_data,
-					      data_len, min_len);
+		int rc = iommu_copy_user_data2(iommu_hwpt_selftest, &data,
+					       user_data);
 		if (rc)
 			return ERR_PTR(rc);
 		user_cfg = &data;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fb2febe7b8d8..db82803b026f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -282,6 +282,10 @@ static inline int iommu_copy_user_data(void *dst_data,
 				     src_data->uptr, src_data->len);
 }
 
+#define iommu_copy_user_data2(dst_struct, dst, src)               \
+	iommu_copy_user_data(dst, src, sizeof(struct dst_struct), \
+			     dst_struct##_min_len)
+
 /**
  * iommu_copy_user_data_from_array - Copy iommu driver specific user space data
  *                                   from an iommu_user_data_array input
========================================================================================

Surely, the end product of the test code above can do:
	iommu_copy_user_data = > __iommu_copy_user_data
	iommu_copy_user_data2 = > iommu_copy_user_data

Thanks
Nicolin

On 2023-09-21 17:44, Jason Gunthorpe wrote:
> On Thu, Sep 21, 2023 at 08:12:03PM +0800, Baolu Lu wrote:
>> On 2023/9/21 15:51, Yi Liu wrote:
>>> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
>>> index 4a7c5c8fdbb4..3c8660fe9bb1 100644
>>> --- a/include/uapi/linux/iommufd.h
>>> +++ b/include/uapi/linux/iommufd.h
>>> @@ -357,6 +357,14 @@ enum iommufd_hwpt_alloc_flags {
>>>    	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>>>    };
>>> +/**
>>> + * enum iommu_hwpt_type - IOMMU HWPT Type
>>> + * @IOMMU_HWPT_TYPE_DEFAULT: default
>>
>> How about s/default/vendor agnostic/ ?
> 
> Please don't use the word vendor :)
> 
> IOMMU_HWPT_TYPE_GENERIC perhaps if we don't like default

Ah yes, a default domain type, not to be confused with any default 
domain type, including the default default domain type. Just in case 
anyone had forgotten how gleefully fun this is :D

I particularly like the bit where we end up with this construct later:

	switch (hwpt_type) {
	case IOMMU_HWPT_TYPE_DEFAULT:
		/* allocate a domain */
	default:
		/* allocate a different domain */
	}

But of course neither case allocates a *default* domain, because it's 
quite obviously the wrong place to be doing that.

I could go on enjoying myself, but basically yeah, "default" can't be a 
type in itself (at best it would be a meta-type which could be 
requested, such that it resolves to some real type to actually 
allocate), so a good name should reflect what the type functionally 
*means* to the user. IIUC the important distinction is that it's an 
abstract kernel-owned pagetable for the user to indirectly control via 
the API, rather than one it owns and writes directly (and thus has to be 
in a specific agreed format).

Thanks,
Robin.

On 2023/9/22 17:47, Robin Murphy wrote:
> On 2023-09-21 17:44, Jason Gunthorpe wrote:
>> On Thu, Sep 21, 2023 at 08:12:03PM +0800, Baolu Lu wrote:
>>> On 2023/9/21 15:51, Yi Liu wrote:
>>>> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
>>>> index 4a7c5c8fdbb4..3c8660fe9bb1 100644
>>>> --- a/include/uapi/linux/iommufd.h
>>>> +++ b/include/uapi/linux/iommufd.h
>>>> @@ -357,6 +357,14 @@ enum iommufd_hwpt_alloc_flags {
>>>>        IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>>>>    };
>>>> +/**
>>>> + * enum iommu_hwpt_type - IOMMU HWPT Type
>>>> + * @IOMMU_HWPT_TYPE_DEFAULT: default
>>>
>>> How about s/default/vendor agnostic/ ?
>>
>> Please don't use the word vendor :)
>>
>> IOMMU_HWPT_TYPE_GENERIC perhaps if we don't like default
> 
> Ah yes, a default domain type, not to be confused with any default domain 
> type, including the default default domain type. Just in case anyone had 
> forgotten how gleefully fun this is :D
> 
> I particularly like the bit where we end up with this construct later:
> 
>      switch (hwpt_type) {
>      case IOMMU_HWPT_TYPE_DEFAULT:
>          /* allocate a domain */
>      default:
>          /* allocate a different domain */
>      }
> 
> But of course neither case allocates a *default* domain, because it's quite 
> obviously the wrong place to be doing that.
> 
> I could go on enjoying myself, but basically yeah, "default" can't be a 
> type in itself (at best it would be a meta-type which could be requested, 
> such that it resolves to some real type to actually allocate), so a good 
> name should reflect what the type functionally *means* to the user. IIUC 
> the important distinction is that it's an abstract kernel-owned pagetable 
> for the user to indirectly control via the API, rather than one it owns and 
> writes directly (and thus has to be in a specific agreed format).

yes. It is just what the existing domain_alloc_user op does. Here we add
a hwpt_type as the type can be given by user, so we need to define a 
specific type for it.

Perhaps we can also name it as IOMMU_HWPT_TYPE_UNMANAGED to be aligned with
the domain type naming. IOMMU_HWPT_TYPE_GENERIC is also a good choice.
Please feel free let me know your preference.

On 2023/9/21 20:10, Baolu Lu wrote:
> On 2023/9/21 15:51, Yi Liu wrote:
>> +/**
>> + * iommu_copy_user_data - Copy iommu driver specific user space data
>> + * @dst_data: Pointer to an iommu driver specific user data that is 
>> defined in
>> + *            include/uapi/linux/iommufd.h
>> + * @src_data: Pointer to a struct iommu_user_data for user space data info
>> + * @data_len: Length of current user data structure, i.e. sizeof(struct 
>> _dst)
>> + * @min_len: Initial length of user data structure for backward 
>> compatibility.
>> + *           This should be offsetofend using the last member in the 
>> user data
>> + *           struct that was initially added to 
>> include/uapi/linux/iommufd.h
>> + */
>> +static inline int iommu_copy_user_data(void *dst_data,
>> +                       const struct iommu_user_data *src_data,
>> +                       size_t data_len, size_t min_len)
>> +{
>> +    if (WARN_ON(!dst_data || !src_data))
>> +        return -EINVAL;
>> +    if (src_data->len < min_len || data_len < src_data->len)
>> +        return -EINVAL;
>> +    return copy_struct_from_user(dst_data, data_len,
>> +                     src_data->uptr, src_data->len);
>> +}
> 
> I am not sure that I understand the purpose of "min_len" correctly. It
> seems like it would always be equal to data_len?

no, it will not be equal to data_len once there is extension in the
uAPI structure.

> Or, it means the minimal data length that the iommu driver requires?

it is the minimal data length the uAPI requires. min_len is finalized
per the upstream of the first version of the uAPI.

On 2023/9/25 14:22, Yi Liu wrote:
> On 2023/9/21 20:10, Baolu Lu wrote:
>> On 2023/9/21 15:51, Yi Liu wrote:
>>> +/**
>>> + * iommu_copy_user_data - Copy iommu driver specific user space data
>>> + * @dst_data: Pointer to an iommu driver specific user data that is 
>>> defined in
>>> + *            include/uapi/linux/iommufd.h
>>> + * @src_data: Pointer to a struct iommu_user_data for user space 
>>> data info
>>> + * @data_len: Length of current user data structure, i.e. 
>>> sizeof(struct _dst)
>>> + * @min_len: Initial length of user data structure for backward 
>>> compatibility.
>>> + *           This should be offsetofend using the last member in the 
>>> user data
>>> + *           struct that was initially added to 
>>> include/uapi/linux/iommufd.h
>>> + */
>>> +static inline int iommu_copy_user_data(void *dst_data,
>>> +                       const struct iommu_user_data *src_data,
>>> +                       size_t data_len, size_t min_len)
>>> +{
>>> +    if (WARN_ON(!dst_data || !src_data))
>>> +        return -EINVAL;
>>> +    if (src_data->len < min_len || data_len < src_data->len)
>>> +        return -EINVAL;
>>> +    return copy_struct_from_user(dst_data, data_len,
>>> +                     src_data->uptr, src_data->len);
>>> +}
>>
>> I am not sure that I understand the purpose of "min_len" correctly. It
>> seems like it would always be equal to data_len?
> 
> no, it will not be equal to data_len once there is extension in the
> uAPI structure.
> 
>> Or, it means the minimal data length that the iommu driver requires?
> 
> it is the minimal data length the uAPI requires. min_len is finalized
> per the upstream of the first version of the uAPI.

So, it looks like a constant. Perhaps we should document it in the
uapi/iommuf.h and avoid using it as a parameter of a helper function?

Best regards,
baolu

On Mon, Sep 25, 2023 at 02:17:37PM +0800, Yi Liu wrote:

> yes. It is just what the existing domain_alloc_user op does. Here we add
> a hwpt_type as the type can be given by user, so we need to define a
> specific type for it.
> 
> Perhaps we can also name it as IOMMU_HWPT_TYPE_UNMANAGED to be
> aligned with

unmanged is also a weird nonsense name these days. We are slowly
replacing it with paging.

> the domain type naming. IOMMU_HWPT_TYPE_GENERIC is also a good choice.
> Please feel free let me know your preference.

This seems OK to me so far

Jason

On Mon, Sep 25, 2023 at 04:01:55PM +0800, Baolu Lu wrote:
> On 2023/9/25 14:22, Yi Liu wrote:
> > On 2023/9/21 20:10, Baolu Lu wrote:
> > > On 2023/9/21 15:51, Yi Liu wrote:
> > > > +/**
> > > > + * iommu_copy_user_data - Copy iommu driver specific user space data
> > > > + * @dst_data: Pointer to an iommu driver specific user data
> > > > that is defined in
> > > > + *            include/uapi/linux/iommufd.h
> > > > + * @src_data: Pointer to a struct iommu_user_data for user
> > > > space data info
> > > > + * @data_len: Length of current user data structure, i.e.
> > > > sizeof(struct _dst)
> > > > + * @min_len: Initial length of user data structure for backward
> > > > compatibility.
> > > > + *           This should be offsetofend using the last member
> > > > in the user data
> > > > + *           struct that was initially added to
> > > > include/uapi/linux/iommufd.h
> > > > + */
> > > > +static inline int iommu_copy_user_data(void *dst_data,
> > > > +                       const struct iommu_user_data *src_data,
> > > > +                       size_t data_len, size_t min_len)
> > > > +{
> > > > +    if (WARN_ON(!dst_data || !src_data))
> > > > +        return -EINVAL;
> > > > +    if (src_data->len < min_len || data_len < src_data->len)
> > > > +        return -EINVAL;
> > > > +    return copy_struct_from_user(dst_data, data_len,
> > > > +                     src_data->uptr, src_data->len);
> > > > +}
> > > 
> > > I am not sure that I understand the purpose of "min_len" correctly. It
> > > seems like it would always be equal to data_len?
> > 
> > no, it will not be equal to data_len once there is extension in the
> > uAPI structure.
> > 
> > > Or, it means the minimal data length that the iommu driver requires?
> > 
> > it is the minimal data length the uAPI requires. min_len is finalized
> > per the upstream of the first version of the uAPI.
> 
> So, it looks like a constant. Perhaps we should document it in the
> uapi/iommuf.h and avoid using it as a parameter of a helper
> function?

It is per-driver, per-struct, so this is the right way to do it

Jason

On Thu, Sep 21, 2023 at 01:58:19PM -0700, Nicolin Chen wrote:

> Perhaps we could put all min_len defines in uAPI header, like:
> include/uapi/linux/gfs2_ondisk.h:442:#define LH_V1_SIZE (offsetofend(struct gfs2_log_header, lh_hash))
> In this way, drivers won't need to deal with that nor have risks
> of breaking ABI by changing a min_len.

I don't think we need constants, just be sure that every call to 
iommu_copy_user_data() has an offsetof() as the last parameter.

Indeed perhaps you should put it in a macro and force this to happen eg:

 #define iommu_copy_user_data(user_data, from, min_size_member) \
  __iommu_copy_user_data(user_data, from, offsetofend(typeof(*from), min_size_member))

 iommu_copy_user_data(user_data, &data, iotlb);

Jason

On Mon, Sep 25, 2023 at 10:05:06AM -0300, Jason Gunthorpe wrote:
> On Thu, Sep 21, 2023 at 01:58:19PM -0700, Nicolin Chen wrote:
> 
> > Perhaps we could put all min_len defines in uAPI header, like:
> > include/uapi/linux/gfs2_ondisk.h:442:#define LH_V1_SIZE (offsetofend(struct gfs2_log_header, lh_hash))
> > In this way, drivers won't need to deal with that nor have risks
> > of breaking ABI by changing a min_len.
> 
> I don't think we need constants, just be sure that every call to 
> iommu_copy_user_data() has an offsetof() as the last parameter.
> 
> Indeed perhaps you should put it in a macro and force this to happen eg:
> 
>  #define iommu_copy_user_data(user_data, from, min_size_member) \
>   __iommu_copy_user_data(user_data, from, offsetofend(typeof(*from), min_size_member))
> 
>  iommu_copy_user_data(user_data, &data, iotlb);

OK. And always use sizeof(typeof(*from)) in the mcaro for the
current data_len, I assume?

Thanks
Nic

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Friday, September 22, 2023 5:48 PM
> 
> I could go on enjoying myself, but basically yeah, "default" can't be a
> type in itself (at best it would be a meta-type which could be
> requested, such that it resolves to some real type to actually
> allocate), so a good name should reflect what the type functionally
> *means* to the user. IIUC the important distinction is that it's an
> abstract kernel-owned pagetable for the user to indirectly control via
> the API, rather than one it owns and writes directly (and thus has to be
> in a specific agreed format).
> 

IOMMU_HWPT_TYPE_KERNEL then?

IOMMU_HWPT_TYPE_GENERIC also doesn't sound a straight word.

> From: Yi Liu <yi.l.liu@intel.com>
> Sent: Thursday, September 21, 2023 3:51 PM
> +
> +/**
> + * iommu_copy_user_data - Copy iommu driver specific user space data
> + * @dst_data: Pointer to an iommu driver specific user data that is defined
> in
> + *            include/uapi/linux/iommufd.h
> + * @src_data: Pointer to a struct iommu_user_data for user space data info
> + * @data_len: Length of current user data structure, i.e. sizeof(struct _dst)
> + * @min_len: Initial length of user data structure for backward compatibility.
> + *           This should be offsetofend using the last member in the user data
> + *           struct that was initially added to include/uapi/linux/iommufd.h
> + */
> +static inline int iommu_copy_user_data(void *dst_data,
> +				       const struct iommu_user_data *src_data,
> +				       size_t data_len, size_t min_len)

iommu_copy_struct_from_user()?

btw given the confusion raised on how this would be used is it clearer
to move it to the patch together with the 1st user?

On Tue, Sep 26, 2023 at 06:37:55AM +0000, Tian, Kevin wrote:
> > From: Robin Murphy <robin.murphy@arm.com>
> > Sent: Friday, September 22, 2023 5:48 PM
> > 
> > I could go on enjoying myself, but basically yeah, "default" can't be a
> > type in itself (at best it would be a meta-type which could be
> > requested, such that it resolves to some real type to actually
> > allocate), so a good name should reflect what the type functionally
> > *means* to the user. IIUC the important distinction is that it's an
> > abstract kernel-owned pagetable for the user to indirectly control via
> > the API, rather than one it owns and writes directly (and thus has to be
> > in a specific agreed format).
> > 
> 
> IOMMU_HWPT_TYPE_KERNEL then?
> 
> IOMMU_HWPT_TYPE_GENERIC also doesn't sound a straight word.

At the end of the day this enum is the type tag for:

 struct iommu_hwpt_alloc {
 	__u32 size;
 	__u32 pt_id;
 	__u32 out_hwpt_id;
 	__u32 __reserved;
+	__u32 hwpt_type;
+	__u32 data_len;
+	__aligned_u64 data_uptr;
 };

That pointer.

IOMMU_HWPT_ALLOC_DATA_NONE = 0
IOMMU_HWPT_ALLOC_DATA_INTEL_VTD
IOMMU_HWPT_ALLOC_DATA_ARM_SMMUV3

etc?

DATA_NONE requires data_len == 0

Jason

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, September 27, 2023 12:29 AM
> 
> On Tue, Sep 26, 2023 at 06:37:55AM +0000, Tian, Kevin wrote:
> > > From: Robin Murphy <robin.murphy@arm.com>
> > > Sent: Friday, September 22, 2023 5:48 PM
> > >
> > > I could go on enjoying myself, but basically yeah, "default" can't be a
> > > type in itself (at best it would be a meta-type which could be
> > > requested, such that it resolves to some real type to actually
> > > allocate), so a good name should reflect what the type functionally
> > > *means* to the user. IIUC the important distinction is that it's an
> > > abstract kernel-owned pagetable for the user to indirectly control via
> > > the API, rather than one it owns and writes directly (and thus has to be
> > > in a specific agreed format).
> > >
> >
> > IOMMU_HWPT_TYPE_KERNEL then?
> >
> > IOMMU_HWPT_TYPE_GENERIC also doesn't sound a straight word.
> 
> At the end of the day this enum is the type tag for:
> 
>  struct iommu_hwpt_alloc {
>  	__u32 size;
>  	__u32 pt_id;
>  	__u32 out_hwpt_id;
>  	__u32 __reserved;
> +	__u32 hwpt_type;
> +	__u32 data_len;
> +	__aligned_u64 data_uptr;
>  };
> 
> That pointer.
> 
> IOMMU_HWPT_ALLOC_DATA_NONE = 0
> IOMMU_HWPT_ALLOC_DATA_INTEL_VTD
> IOMMU_HWPT_ALLOC_DATA_ARM_SMMUV3
> 
> etc?
> 
> DATA_NONE requires data_len == 0
> 

Looks good. Probably hwpt_type can be also renamed to data_type
to better match this interpretation.

On Thu, Sep 21, 2023 at 12:51:22AM -0700, Yi Liu wrote:
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 660dc1931dc9..12e12e5563e6 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>

Oh we should definately avoid doing that!
  
Maybe this is a good moment to start a new header file exclusively for
iommu drivers and core subsystem to include?

 include/linux/iommu-driver.h

?

Put iommu_copy_user_data() and  struct iommu_user_data in there

Avoid this include in this file.

>  #define IOMMU_READ	(1 << 0)
>  #define IOMMU_WRITE	(1 << 1)
> @@ -227,6 +228,41 @@ struct iommu_iotlb_gather {
>  	bool			queued;
>  };
>  
> +/**
> + * struct iommu_user_data - iommu driver specific user space data info
> + * @uptr: Pointer to the user buffer for copy_from_user()
> + * @len: The length of the user buffer in bytes
> + *
> + * A user space data is an uAPI that is defined in include/uapi/linux/iommufd.h
> + * Both @uptr and @len should be just copied from an iommufd core uAPI structure
> + */
> +struct iommu_user_data {
> +	void __user *uptr;
> +	size_t len;
> +};

Put the "hwpt_type" in here and just call it type

Jason

On 2023/10/12 21:39, Jason Gunthorpe wrote:
> On Thu, Oct 12, 2023 at 05:11:09PM +0800, Yi Liu wrote:
>> On 2023/10/11 00:58, Jason Gunthorpe wrote:
>>> On Thu, Sep 21, 2023 at 12:51:22AM -0700, Yi Liu wrote:
>>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>>> index 660dc1931dc9..12e12e5563e6 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>
>>>
>>> Oh we should definately avoid doing that!
>>> Maybe this is a good moment to start a new header file exclusively for
>>> iommu drivers and core subsystem to include?
>>>
>>>    include/linux/iommu-driver.h
>>>
>>> ?
>>>
>>> Put iommu_copy_user_data() and  struct iommu_user_data in there
>>>
>>> Avoid this include in this file.
>>
>> sure. btw. seems all the user of this API and structure are in the
>> drivers/iommu directory. can we just putting them in
>> drivers/iommu/iommu-priv.h?
> 
> iommu-priv.h should be private to the core iommu code, and we sort of
> extended it to iommufd as well.
> 
> iommu-driver.h would be "private" to the core and all the drivers
> only.
> 
> As include ../.. is often frown on at large scale it is probably
> better to be in include/linux

got it.

> 
>> Just one concern. There are other paths (like cache_invalidate of
>> this series and Nic's set_dev_data) uses this struct as well. I'm
>> a bit worrying if it is good to put type here as type is meaningful
>> for the domain_alloc_user path.
> 
> There is always a type though? I haven't got that far in the series
> yet to see..

not really. Below the users of the struct iommu_user_data in my current
iommufd_nesting branch. Only the domain_alloc_user op has type as there
can be multiple vendor specific alloc data types. Basically, I'm ok to
make the change you suggested, just not sure if it is good to add type
as it is only needed by one path.

	/* Domain allocation and freeing by the iommu driver */
	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
	struct iommu_domain *(*domain_alloc_user)(struct device *dev, u32 flags,
						  enum iommu_hwpt_type hwpt_type,
						  struct iommu_domain *parent,
						  const struct iommu_user_data *user_data);


	int (*set_dev_user_data)(struct device *dev,
				 const struct iommu_user_data *user_data);
	void (*unset_dev_user_data)(struct device *dev);


	int (*cache_invalidate_user)(struct iommu_domain *domain,
				     struct iommu_user_data_array *array,
				     u32 *error_code);

above code snippet is from below file:

https://github.com/yiliu1765/iommufd/blob/iommufd_nesting/include/linux/iommu.h

On 2023/10/12 17:12, Yi Liu wrote:
> On 2023/9/26 14:56, Tian, Kevin wrote:
>>> From: Yi Liu <yi.l.liu@intel.com>
>>> Sent: Thursday, September 21, 2023 3:51 PM
>>> +
>>> +/**
>>> + * iommu_copy_user_data - Copy iommu driver specific user space data
>>> + * @dst_data: Pointer to an iommu driver specific user data that is 
>>> defined
>>> in
>>> + *            include/uapi/linux/iommufd.h
>>> + * @src_data: Pointer to a struct iommu_user_data for user space data info
>>> + * @data_len: Length of current user data structure, i.e. sizeof(struct 
>>> _dst)
>>> + * @min_len: Initial length of user data structure for backward 
>>> compatibility.
>>> + *           This should be offsetofend using the last member in the 
>>> user data
>>> + *           struct that was initially added to 
>>> include/uapi/linux/iommufd.h
>>> + */
>>> +static inline int iommu_copy_user_data(void *dst_data,
>>> +                       const struct iommu_user_data *src_data,
>>> +                       size_t data_len, size_t min_len)
>>
>> iommu_copy_struct_from_user()?
>>
>> btw given the confusion raised on how this would be used is it clearer
>> to move it to the patch together with the 1st user?
> 
> sure. How about your opinion? @Nic.
> 

after a second thinking, the first user of this helper is the patch to
extend mock iommu driver. Is it suitable to introduce a common API together
with selftest code?

https://lore.kernel.org/linux-iommu/20230921075138.124099-14-yi.l.liu@intel.com/

On Thu, Oct 12, 2023 at 05:34:58PM -0700, Nicolin Chen wrote:
> On Tue, Oct 10, 2023 at 01:58:44PM -0300, Jason Gunthorpe wrote:
> > On Thu, Sep 21, 2023 at 12:51:22AM -0700, Yi Liu wrote:
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index 660dc1931dc9..12e12e5563e6 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>
> > 
> > Oh we should definately avoid doing that!
> >   
> > Maybe this is a good moment to start a new header file exclusively for
> > iommu drivers and core subsystem to include?
> > 
> >  include/linux/iommu-driver.h
> > 
> > ?
> > 
> > Put iommu_copy_user_data() and  struct iommu_user_data in there
> > 
> > Avoid this include in this file.
> 
> By looking closer, it seems that we included the uapi header for:
> +	struct iommu_domain *(*domain_alloc_user)(struct device *dev, u32 flags,
> +						  enum iommu_hwpt_data_type data_type,
> +						  struct iommu_domain *parent,
> +						  const struct iommu_user_data *user_data);
> 
> So we could drop the include, and instead add this next to structs:
> +enum iommu_hwpt_data_type;
> 
> Then it's not that necessary to have a new header? We could mark a
> section of "driver exclusively functions" in iommu.h, I think.

Yeah, OK, though I still have a desire to split this header..

(though can you really forward declare enums and then pass it by value?)

Jason

On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:

> not really. Below the users of the struct iommu_user_data in my current
> iommufd_nesting branch. Only the domain_alloc_user op has type as there
> can be multiple vendor specific alloc data types. Basically, I'm ok to
> make the change you suggested, just not sure if it is good to add type
> as it is only needed by one path.

I don't think we should ever have an opaque data blob without a type
tag..

Jason

On Fri, Oct 13, 2023 at 11:04:56AM -0300, Jason Gunthorpe wrote:
> On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:
> 
> > not really. Below the users of the struct iommu_user_data in my current
> > iommufd_nesting branch. Only the domain_alloc_user op has type as there
> > can be multiple vendor specific alloc data types. Basically, I'm ok to
> > make the change you suggested, just not sure if it is good to add type
> > as it is only needed by one path.
> 
> I don't think we should ever have an opaque data blob without a type
> tag..

I can add those "missing" data types, and then a driver will be
responsible for sanitizing the type along with the data_len.

I notice that the enum iommu_hwpt_data_type in the posted patch
is confined to the alloc_user uAPI. Perhaps we should share it
with invalidate too:

/**
 * enum iommu_hwpt_data_type - IOMMU HWPT Data Type
 * @IOMMU_HWPT_DATA_NONE: no data
 * @IOMMU_HWPT_DATA_VTD_S1: Intel VT-d stage-1 page table
 * @IOMMU_HWPT_DATA_ARM_SMMUV3: ARM SMMUv3 Context Descriptor Table
 */
enum iommu_hwpt_data_type {
	IOMMU_HWPT_DATA_NONE,
	IOMMU_HWPT_DATA_VTD_S1,
	IOMMU_HWPT_DATA_ARM_SMMUV3,
};

Though inevitably we'd have to define a separate data group for
things like set_dev_data that is related to idev v.s. hwpt:

// IOMMU_DEV_DATA_TYPE sounds like an IOMMU device, other than a
// passthrough device, so renaming to "_IDEV_" here. And perhaps
// "set_dev_data" could be "set_idev_data" too? Any better name?

/**
 * enum iommu_idev_data_type - Data Type for a Device behind an IOMMU
 * @IOMMU_IDEV_DATA_NONE: no data
 * @IOMMU_IDEV_DATA_ARM_SMMUV3: ARM SMMUv3 specific device data
 */
enum iommu_idev_data_type {
	IOMMU_IDEV_DATA_NONE,
	IOMMU_IDEV_DATA_ARM_SMMUV3,
};

/**
 * struct iommu_idev_data_arm_smmuv3 - ARM SMMUv3 specific device data
 * @sid: The Stream ID that is assigned in the user space
 *
 * The SMMUv3 specific user space data for a device that is behind an SMMU HW.
 * The guest-level user data should be linked to the host-level kernel data,
 * which will be used by user space cache invalidation commands.
 */
struct iommu_idev_data_arm_smmuv3 {
	__u32 sid;
};

/**
 * struct iommu_set_idev_data - ioctl(IOMMU_SET_IDEV_DATA)
 * @size: sizeof(struct iommu_set_idev_data)
 * @dev_id: The device to set an iommu specific device data
 * @data_uptr: User pointer of the device user data
 * @data_len: Length of the device user data
 *
 * The device data must be unset using ioctl(IOMMU_UNSET_IDEV_DATA), before
 * another ioctl(IOMMU_SET_IDEV_DATA) call or before the device itself gets
 * unbind'd from the iommufd context.
 */
struct iommu_set_idev_data {
	__u32 size;
	__u32 dev_id;
	__aligned_u64 data_uptr;
	__u32 data_len;
};

Thanks
Nic

On Fri, Oct 13, 2023 at 07:42:50PM +0800, Yi Liu wrote:
> On 2023/10/12 17:12, Yi Liu wrote:
> > On 2023/9/26 14:56, Tian, Kevin wrote:
> > > > From: Yi Liu <yi.l.liu@intel.com>
> > > > Sent: Thursday, September 21, 2023 3:51 PM
> > > > +
> > > > +/**
> > > > + * iommu_copy_user_data - Copy iommu driver specific user space data
> > > > + * @dst_data: Pointer to an iommu driver specific user data that is
> > > > defined
> > > > in
> > > > + *            include/uapi/linux/iommufd.h
> > > > + * @src_data: Pointer to a struct iommu_user_data for user space data info
> > > > + * @data_len: Length of current user data structure, i.e. sizeof(struct
> > > > _dst)
> > > > + * @min_len: Initial length of user data structure for backward
> > > > compatibility.
> > > > + *           This should be offsetofend using the last member in the
> > > > user data
> > > > + *           struct that was initially added to
> > > > include/uapi/linux/iommufd.h
> > > > + */
> > > > +static inline int iommu_copy_user_data(void *dst_data,
> > > > +                       const struct iommu_user_data *src_data,
> > > > +                       size_t data_len, size_t min_len)
> > > 
> > > iommu_copy_struct_from_user()?
> > > 
> > > btw given the confusion raised on how this would be used is it clearer
> > > to move it to the patch together with the 1st user?
> > 
> > sure. How about your opinion? @Nic.
> > 
> 
> after a second thinking, the first user of this helper is the patch to
> extend mock iommu driver. Is it suitable to introduce a common API together
> with selftest code?
> 
> https://lore.kernel.org/linux-iommu/20230921075138.124099-14-yi.l.liu@intel.com/

I feel no...

But I could separate iommu_copy_struct_from_user and its array
drivitive into an additional patch placed before the selftest
changes, so at least it would be closer to the first callers.

Thanks
Nic

On 2023/10/14 01:56, Nicolin Chen wrote:
> On Fri, Oct 13, 2023 at 11:04:56AM -0300, Jason Gunthorpe wrote:
>> On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:
>>
>>> not really. Below the users of the struct iommu_user_data in my current
>>> iommufd_nesting branch. Only the domain_alloc_user op has type as there
>>> can be multiple vendor specific alloc data types. Basically, I'm ok to
>>> make the change you suggested, just not sure if it is good to add type
>>> as it is only needed by one path.
>>
>> I don't think we should ever have an opaque data blob without a type
>> tag..
> 
> I can add those "missing" data types, and then a driver will be
> responsible for sanitizing the type along with the data_len.
> 
> I notice that the enum iommu_hwpt_data_type in the posted patch
> is confined to the alloc_user uAPI. Perhaps we should share it
> with invalidate too:

invalidation path does not need a type field today as the data
type is vendor specific, vendor driver should know the data type
when calls in.

> 
> /**
>   * enum iommu_hwpt_data_type - IOMMU HWPT Data Type
>   * @IOMMU_HWPT_DATA_NONE: no data
>   * @IOMMU_HWPT_DATA_VTD_S1: Intel VT-d stage-1 page table
>   * @IOMMU_HWPT_DATA_ARM_SMMUV3: ARM SMMUv3 Context Descriptor Table
>   */
> enum iommu_hwpt_data_type {
> 	IOMMU_HWPT_DATA_NONE,
> 	IOMMU_HWPT_DATA_VTD_S1,
> 	IOMMU_HWPT_DATA_ARM_SMMUV3,
> };
> 
> Though inevitably we'd have to define a separate data group for
> things like set_dev_data that is related to idev v.s. hwpt:

yes, the type field is in separate data group per path.

> 
> // IOMMU_DEV_DATA_TYPE sounds like an IOMMU device, other than a
> // passthrough device, so renaming to "_IDEV_" here. And perhaps
> // "set_dev_data" could be "set_idev_data" too? Any better name?
> 
> /**
>   * enum iommu_idev_data_type - Data Type for a Device behind an IOMMU
>   * @IOMMU_IDEV_DATA_NONE: no data
>   * @IOMMU_IDEV_DATA_ARM_SMMUV3: ARM SMMUv3 specific device data
>   */
> enum iommu_idev_data_type {
> 	IOMMU_IDEV_DATA_NONE,
> 	IOMMU_IDEV_DATA_ARM_SMMUV3,
> };
> 
> /**
>   * struct iommu_idev_data_arm_smmuv3 - ARM SMMUv3 specific device data
>   * @sid: The Stream ID that is assigned in the user space
>   *
>   * The SMMUv3 specific user space data for a device that is behind an SMMU HW.
>   * The guest-level user data should be linked to the host-level kernel data,
>   * which will be used by user space cache invalidation commands.
>   */
> struct iommu_idev_data_arm_smmuv3 {
> 	__u32 sid;
> };
> 
> /**
>   * struct iommu_set_idev_data - ioctl(IOMMU_SET_IDEV_DATA)
>   * @size: sizeof(struct iommu_set_idev_data)
>   * @dev_id: The device to set an iommu specific device data
>   * @data_uptr: User pointer of the device user data
>   * @data_len: Length of the device user data
>   *
>   * The device data must be unset using ioctl(IOMMU_UNSET_IDEV_DATA), before
>   * another ioctl(IOMMU_SET_IDEV_DATA) call or before the device itself gets
>   * unbind'd from the iommufd context.
>   */
> struct iommu_set_idev_data {
> 	__u32 size;
> 	__u32 dev_id;
> 	__aligned_u64 data_uptr;
> 	__u32 data_len;
> };
> 
> Thanks
> Nic

On Mon, Oct 16, 2023 at 11:28:15AM +0800, Yi Liu wrote:
> On 2023/10/14 01:56, Nicolin Chen wrote:
> > On Fri, Oct 13, 2023 at 11:04:56AM -0300, Jason Gunthorpe wrote:
> > > On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:
> > > 
> > > > not really. Below the users of the struct iommu_user_data in my current
> > > > iommufd_nesting branch. Only the domain_alloc_user op has type as there
> > > > can be multiple vendor specific alloc data types. Basically, I'm ok to
> > > > make the change you suggested, just not sure if it is good to add type
> > > > as it is only needed by one path.
> > > 
> > > I don't think we should ever have an opaque data blob without a type
> > > tag..
> > 
> > I can add those "missing" data types, and then a driver will be
> > responsible for sanitizing the type along with the data_len.
> > 
> > I notice that the enum iommu_hwpt_data_type in the posted patch
> > is confined to the alloc_user uAPI. Perhaps we should share it
> > with invalidate too:
> 
> invalidation path does not need a type field today as the data
> type is vendor specific, vendor driver should know the data type
> when calls in.

I'm not keen on that, what if a driver needs another type in the
future?  You'd want to make the invalidation data format part of the
domain allocation?

Jason

On Mon, Oct 16, 2023 at 08:54:07AM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 16, 2023 at 11:28:15AM +0800, Yi Liu wrote:
> > On 2023/10/14 01:56, Nicolin Chen wrote:
> > > On Fri, Oct 13, 2023 at 11:04:56AM -0300, Jason Gunthorpe wrote:
> > > > On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:
> > > > 
> > > > > not really. Below the users of the struct iommu_user_data in my current
> > > > > iommufd_nesting branch. Only the domain_alloc_user op has type as there
> > > > > can be multiple vendor specific alloc data types. Basically, I'm ok to
> > > > > make the change you suggested, just not sure if it is good to add type
> > > > > as it is only needed by one path.
> > > > 
> > > > I don't think we should ever have an opaque data blob without a type
> > > > tag..
> > > 
> > > I can add those "missing" data types, and then a driver will be
> > > responsible for sanitizing the type along with the data_len.
> > > 
> > > I notice that the enum iommu_hwpt_data_type in the posted patch
> > > is confined to the alloc_user uAPI. Perhaps we should share it
> > > with invalidate too:
> > 
> > invalidation path does not need a type field today as the data
> > type is vendor specific, vendor driver should know the data type
> > when calls in.
> 
> I'm not keen on that, what if a driver needs another type in the
> future?  You'd want to make the invalidation data format part of the
> domain allocation?

The invalidation data has hwpt_id so it's tied to a hwpt and its 
hwpt->domain. Would it be reasonable to have a different type of
invalidation data for the same type of hwpt?

With this being asked, I added it for our next version. At this
moment, it only does a sanity job:

// API
__iommu_copy_struct_from_user(void *dst_data,
			      const struct iommu_user_data *src_data,
			      unsigned int data_type, size_t data_len,
			      size_t min_len)
{
	if (src_data->type != data_type)
		return -EINVAL;

// Caller
	rc = iommu_copy_struct_from_user(&user_cfg, user_data,
					 IOMMU_HWPT_DATA_SELFTEST, iotlb);
	if (rc)
		return ERR_PTR(rc);

Thanks
Nic

On 2023/10/17 02:17, Nicolin Chen wrote:
> On Mon, Oct 16, 2023 at 08:54:07AM -0300, Jason Gunthorpe wrote:
>> On Mon, Oct 16, 2023 at 11:28:15AM +0800, Yi Liu wrote:
>>> On 2023/10/14 01:56, Nicolin Chen wrote:
>>>> On Fri, Oct 13, 2023 at 11:04:56AM -0300, Jason Gunthorpe wrote:
>>>>> On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:
>>>>>
>>>>>> not really. Below the users of the struct iommu_user_data in my current
>>>>>> iommufd_nesting branch. Only the domain_alloc_user op has type as there
>>>>>> can be multiple vendor specific alloc data types. Basically, I'm ok to
>>>>>> make the change you suggested, just not sure if it is good to add type
>>>>>> as it is only needed by one path.
>>>>>
>>>>> I don't think we should ever have an opaque data blob without a type
>>>>> tag..
>>>>
>>>> I can add those "missing" data types, and then a driver will be
>>>> responsible for sanitizing the type along with the data_len.
>>>>
>>>> I notice that the enum iommu_hwpt_data_type in the posted patch
>>>> is confined to the alloc_user uAPI. Perhaps we should share it
>>>> with invalidate too:
>>>
>>> invalidation path does not need a type field today as the data
>>> type is vendor specific, vendor driver should know the data type
>>> when calls in.
>>
>> I'm not keen on that, what if a driver needs another type in the
>> future?  You'd want to make the invalidation data format part of the
>> domain allocation?
> 
> The invalidation data has hwpt_id so it's tied to a hwpt and its
> hwpt->domain. Would it be reasonable to have a different type of
> invalidation data for the same type of hwpt?

this seems like what Jason asks. A type of hwpt can have two kinds
of invalidation data types. Is it really possible?

> With this being asked, I added it for our next version. At this
> moment, it only does a sanity job:
> 
> // API
> __iommu_copy_struct_from_user(void *dst_data,
> 			      const struct iommu_user_data *src_data,
> 			      unsigned int data_type, size_t data_len,
> 			      size_t min_len)
> {
> 	if (src_data->type != data_type)
> 		return -EINVAL;
> 
> // Caller
> 	rc = iommu_copy_struct_from_user(&user_cfg, user_data,
> 					 IOMMU_HWPT_DATA_SELFTEST, iotlb);
> 	if (rc)
> 		return ERR_PTR(rc);
> 
> Thanks
> Nic

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, October 17, 2023 4:52 PM
> 
> On 2023/10/17 02:17, Nicolin Chen wrote:
> > On Mon, Oct 16, 2023 at 08:54:07AM -0300, Jason Gunthorpe wrote:
> >> On Mon, Oct 16, 2023 at 11:28:15AM +0800, Yi Liu wrote:
> >>> On 2023/10/14 01:56, Nicolin Chen wrote:
> >>>> On Fri, Oct 13, 2023 at 11:04:56AM -0300, Jason Gunthorpe wrote:
> >>>>> On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:
> >>>>>
> >>>>>> not really. Below the users of the struct iommu_user_data in my
> current
> >>>>>> iommufd_nesting branch. Only the domain_alloc_user op has type as
> there
> >>>>>> can be multiple vendor specific alloc data types. Basically, I'm ok to
> >>>>>> make the change you suggested, just not sure if it is good to add type
> >>>>>> as it is only needed by one path.
> >>>>>
> >>>>> I don't think we should ever have an opaque data blob without a type
> >>>>> tag..
> >>>>
> >>>> I can add those "missing" data types, and then a driver will be
> >>>> responsible for sanitizing the type along with the data_len.
> >>>>
> >>>> I notice that the enum iommu_hwpt_data_type in the posted patch
> >>>> is confined to the alloc_user uAPI. Perhaps we should share it
> >>>> with invalidate too:
> >>>
> >>> invalidation path does not need a type field today as the data
> >>> type is vendor specific, vendor driver should know the data type
> >>> when calls in.
> >>
> >> I'm not keen on that, what if a driver needs another type in the
> >> future?  You'd want to make the invalidation data format part of the
> >> domain allocation?
> >
> > The invalidation data has hwpt_id so it's tied to a hwpt and its
> > hwpt->domain. Would it be reasonable to have a different type of
> > invalidation data for the same type of hwpt?
> 
> this seems like what Jason asks. A type of hwpt can have two kinds
> of invalidation data types. Is it really possible?
> 

e.g. vhost-iommu may want its own vendor-agnostic format from
previous discussion...

On Mon, Oct 16, 2023 at 11:17:56AM -0700, Nicolin Chen wrote:
> On Mon, Oct 16, 2023 at 08:54:07AM -0300, Jason Gunthorpe wrote:
> > On Mon, Oct 16, 2023 at 11:28:15AM +0800, Yi Liu wrote:
> > > On 2023/10/14 01:56, Nicolin Chen wrote:
> > > > On Fri, Oct 13, 2023 at 11:04:56AM -0300, Jason Gunthorpe wrote:
> > > > > On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:
> > > > > 
> > > > > > not really. Below the users of the struct iommu_user_data in my current
> > > > > > iommufd_nesting branch. Only the domain_alloc_user op has type as there
> > > > > > can be multiple vendor specific alloc data types. Basically, I'm ok to
> > > > > > make the change you suggested, just not sure if it is good to add type
> > > > > > as it is only needed by one path.
> > > > > 
> > > > > I don't think we should ever have an opaque data blob without a type
> > > > > tag..
> > > > 
> > > > I can add those "missing" data types, and then a driver will be
> > > > responsible for sanitizing the type along with the data_len.
> > > > 
> > > > I notice that the enum iommu_hwpt_data_type in the posted patch
> > > > is confined to the alloc_user uAPI. Perhaps we should share it
> > > > with invalidate too:
> > > 
> > > invalidation path does not need a type field today as the data
> > > type is vendor specific, vendor driver should know the data type
> > > when calls in.
> > 
> > I'm not keen on that, what if a driver needs another type in the
> > future?  You'd want to make the invalidation data format part of the
> > domain allocation?
> 
> The invalidation data has hwpt_id so it's tied to a hwpt and its 
> hwpt->domain. Would it be reasonable to have a different type of
> invalidation data for the same type of hwpt?

Yeah, maybe? Go down the road 10 years and we might have SMMUv3
invalidation format v1 and v2 or something?

Like we don't know what the HW side will do, they might extend the
command queue to have bigger commands and negotiate with the driver if
the bigger/smaller format is used. We've done that in our HW a couple
of times now.

Jason

On Wed, Oct 18, 2023 at 01:37:20PM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 16, 2023 at 11:17:56AM -0700, Nicolin Chen wrote:
> > On Mon, Oct 16, 2023 at 08:54:07AM -0300, Jason Gunthorpe wrote:
> > > On Mon, Oct 16, 2023 at 11:28:15AM +0800, Yi Liu wrote:
> > > > On 2023/10/14 01:56, Nicolin Chen wrote:
> > > > > On Fri, Oct 13, 2023 at 11:04:56AM -0300, Jason Gunthorpe wrote:
> > > > > > On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:
> > > > > > 
> > > > > > > not really. Below the users of the struct iommu_user_data in my current
> > > > > > > iommufd_nesting branch. Only the domain_alloc_user op has type as there
> > > > > > > can be multiple vendor specific alloc data types. Basically, I'm ok to
> > > > > > > make the change you suggested, just not sure if it is good to add type
> > > > > > > as it is only needed by one path.
> > > > > > 
> > > > > > I don't think we should ever have an opaque data blob without a type
> > > > > > tag..
> > > > > 
> > > > > I can add those "missing" data types, and then a driver will be
> > > > > responsible for sanitizing the type along with the data_len.
> > > > > 
> > > > > I notice that the enum iommu_hwpt_data_type in the posted patch
> > > > > is confined to the alloc_user uAPI. Perhaps we should share it
> > > > > with invalidate too:
> > > > 
> > > > invalidation path does not need a type field today as the data
> > > > type is vendor specific, vendor driver should know the data type
> > > > when calls in.
> > > 
> > > I'm not keen on that, what if a driver needs another type in the
> > > future?  You'd want to make the invalidation data format part of the
> > > domain allocation?
> > 
> > The invalidation data has hwpt_id so it's tied to a hwpt and its 
> > hwpt->domain. Would it be reasonable to have a different type of
> > invalidation data for the same type of hwpt?
> 
> Yeah, maybe? Go down the road 10 years and we might have SMMUv3
> invalidation format v1 and v2 or something?
> 
> Like we don't know what the HW side will do, they might extend the
> command queue to have bigger commands and negotiate with the driver if
> the bigger/smaller format is used. We've done that in our HW a couple
> of times now.

I see. We'll have the type. Thanks!