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

[Yi Liu <yi.l.liu@intel.com>]

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

-- 
Regards,
Yi Liu