Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

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

On 2024/5/8 20:25, Jason Gunthorpe wrote:
> On Wed, May 08, 2024 at 02:10:05PM +0800, Yi Liu wrote:
>> On 2024/5/7 23:18, Jason Gunthorpe wrote:
>>> On Tue, May 07, 2024 at 10:28:34AM +0800, Yi Liu wrote:
>>>>>> We still need something to do before we can safely remove this check.
>>>>>> All the domain allocation interfaces should eventually have the device
>>>>>> pointer as the input, and all domain attributions could be initialized
>>>>>> during domain allocation. In the attach paths, it should return -EINVAL
>>>>>> directly if the domain is not compatible with the iommu for the device.
>>>>>
>>>>> Yes, and this is already true for PASID.
>>>>
>>>> I'm not quite get why it is already true for PASID. I think Baolu's remark
>>>> is general to domains attached to either RID or PASID.
>>>>
>>>>> I feel we could reasonably insist that domanis used with PASID are
>>>>> allocated with a non-NULL dev.
>>>>
>>>> Any special reason for this disclaim?
>>>
>>> If it makes the driver easier, why not?
>>
>> yep.
>>
>>> PASID is special since PASID is barely used, we could insist that
>>> new PASID users also use the new domian_alloc API.
>>
>> Ok. I have one doubt on how to make it in iommufd core. Shall the iommufd
>> core call ops->domain_alloc_paging() directly or still call
>> ops->domain_alloc_user() while ops->domain_alloc_user() flows into the
>> paging domain allocation with non-null dev?
> 
> I mostly figured we'd need a new iommu_domain_alloc_dev() sort of
> thing? VFIO should be changed over too.

Would this new iommu-domain_alloc_dev() have flags and user_data input?
As below code snippet, the existing iommufd core uses domain_alloc_user
op to allocate the s2 domain (paging domain), and will fall back to
iommu_domain_alloc() only if the domain_alloc_user op does not exist. The
typical reason is to use domain_alloc_user op is to allocate a paging
domain with NESTED_PARENT flag. I suppose the new iommu_domain_alloc_dev()
shall allow allocating s2 domain with NESTED_PARENT as well. right?


struct iommufd_hwpt_paging *
iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
			  struct iommufd_device *idev, u32 flags,
			  bool immediate_attach,
			  const struct iommu_user_data *user_data)
{

	...
	if (ops->domain_alloc_user) {
		hwpt->domain = ops->domain_alloc_user(idev->dev, flags, NULL,
						      user_data);
		if (IS_ERR(hwpt->domain)) {
			rc = PTR_ERR(hwpt->domain);
			hwpt->domain = NULL;
			goto out_abort;
		}
		hwpt->domain->owner = ops;
	} else {
		hwpt->domain = iommu_domain_alloc(idev->dev->bus);
		if (!hwpt->domain) {
			rc = -ENOMEM;
			goto out_abort;
		}
	}
	...
}

-- 
Regards,
Yi Liu