| Message ID | 20230921075138.124099-2-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommufd: Add nesting infrastructure | expand |
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? 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 2023/9/27 09:08, Tian, Kevin wrote: >> 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. ack.
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/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? > >> #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 I assume this is a change related to the above comment to avoid including uapi/linux/iommufd.h. right? 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.
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.
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 > 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.. Jason
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. > > #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 Ah, then domain_alloc_user can omit the enum iommu_hwpt_data_type. Thanks! Nic
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 2023/10/17 17:28, Tian, Kevin wrote: >> 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... ok. So in future, if there is vendor-agnostic format cache invalidation data structure, then each existing hwpt types would have two cache invalidation types. is it? So it is required to make the invalidation data format part of the domain allocation. Perhaps we can add it later? Regards, Yi Liu
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!
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 491bcde1ff96..4ffc939a71f0 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4075,7 +4075,10 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type) } static struct iommu_domain * -intel_iommu_domain_alloc_user(struct device *dev, u32 flags) +intel_iommu_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) { struct iommu_domain *domain; struct intel_iommu *iommu; diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index 26a8a818ffa3..1d7378a6cbb3 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -96,7 +96,9 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, hwpt->ioas = ioas; if (ops->domain_alloc_user) { - hwpt->domain = ops->domain_alloc_user(idev->dev, flags); + hwpt->domain = ops->domain_alloc_user(idev->dev, flags, + IOMMU_HWPT_TYPE_DEFAULT, + NULL, NULL); if (IS_ERR(hwpt->domain)) { rc = PTR_ERR(hwpt->domain); hwpt->domain = NULL; diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index b54cbfb1862b..2205a552e570 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -171,7 +171,10 @@ static struct iommu_domain *mock_domain_alloc(unsigned int iommu_domain_type) } static struct iommu_domain * -mock_domain_alloc_user(struct device *dev, u32 flags) +mock_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) { struct iommu_domain *domain; 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> #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; +}; + +/** + * 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); +} + /** * struct iommu_ops - iommu ops and capabilities * @capable: check capability @@ -237,11 +273,17 @@ struct iommu_iotlb_gather { * @domain_alloc: allocate iommu domain * @domain_alloc_user: Allocate an iommu domain corresponding to the input * parameters like flags defined as enum iommufd_ioas_map_flags + * and the @hwpt_type that is defined as enum iommu_hwpt_type * in include/uapi/linux/iommufd.h. Different from the * domain_alloc op, it requires iommu driver to fully * initialize a new domain including the generic iommu_domain - * struct. Upon success, a domain is returned. Upon failure, - * ERR_PTR must be returned. + * struct. + * Upon success, if the @user_data is valid and the @parent + * points to a kernel-managed domain, the new domain must be + * IOMMU_DOMAIN_NESTED type; otherwise, the @parent must be + * NULL while the @user_data can be optionally provided, the + * new domain must be IOMMU_DOMAIN_UNMANAGED type. + * Upon failure, ERR_PTR must be returned. * @probe_device: Add device to iommu driver handling * @release_device: Remove device from iommu driver handling * @probe_finalize: Do final setup work after the device is added to an IOMMU @@ -274,7 +316,10 @@ struct iommu_ops { /* 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); + 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); struct iommu_device *(*probe_device)(struct device *dev); void (*release_device)(struct device *dev); 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 + */ +enum iommu_hwpt_type { + IOMMU_HWPT_TYPE_DEFAULT, +}; + /** * struct iommu_hwpt_alloc - ioctl(IOMMU_HWPT_ALLOC) * @size: sizeof(struct iommu_hwpt_alloc)