Re: [PATCH v2 7/8] iommu/amd: Add iommu_ops->domain_alloc_paging support

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,


On 10/4/2024 6:26 PM, Jason Gunthorpe wrote:
> On Fri, Oct 04, 2024 at 05:25:20PM +0530, Vasant Hegde wrote:
> 
>>>> +/*
>>>> + * Allocate domain with default (amd_iommu_pgtable) page table type. Core will
>>>> + * call domain_alloc_user() interface to allocate PASID capable domain.
>>>> + */
>>>> +static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device *dev)
>>>> +{
>>>> +	return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
>>>> +				     dev, 0, amd_iommu_pgtable);
>>>> +}
>>>
>>> I think you should avoid this. The _user one (aka extended) should
>>> handle everthing. It doesn't really make sense anyhow, as the legacy
>>> VFIO path is going to take this branch as it will have 0 flags.
>>
>> I should have been bit more clear in cover letter.
>>
>> I thought eventually we want to remove ops->domain_alloc() interface. Hence I
>> introduced domain_alloc_paging(). I didn't remove amd_iommu_domain_alloc() in
>> this series. I was planning to do it in later.
> 
> We do want to remove domain_alloc
> 
>> If we are fine to keep ops->domain_alloc() then I can just drop this patch.
> 
> But why not just remove it and not add amd_iommu_domain_alloc_paging ?

Because we have __iommu_domain_alloc() which calls domain_alloc_paging /
domain_alloc().

> 
> Your prior patch does this:
> 
> +	if (ops->domain_alloc_paging && !flags)
> +		domain = ops->domain_alloc_paging(dev);
> +	else if (ops->domain_alloc_user)
> +		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> +	else if (ops->domain_alloc && !flags)
> +		domain = ops->domain_alloc(IOMMU_DOMAIN_UNMANAGED);
> +	else
> +		return ERR_PTR(-EOPNOTSUPP);
> 
> So if !ops->domain_alloc_paging && ops->domain_alloc_user it calls
> ops->domain_alloc_user()
> 
> You just need ops->domain_alloc_user(flags == 0) to allocate the
> fasted/best page table with no-PASID support (ie v1 in most cases) and
> everything is fine.

This is fine. I can add it.

> 
> Don't add amd_iommu_domain_alloc_paging(), just add the above
> paragraph of logic to ops->domain_alloc_user()
> 
> The point of all this fixing was to stop using IOMMU_DOMAIN_DMA to
> select a page table format and instead rely on IOMMU_HWPT_ALLOC_PASID
> to make the selection. The core code and VFIO will set
> IOMMU_HWPT_ALLOC_PASID properly for what it wants to do. I expect the
> driver to rely on IOMMU_HWPT_ALLOC_PASID exclusively.

Sure. Will fix it. But as said above, we still need domain_alloc_paging() or
domain_alloc() interface.


-Vasant