Re: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

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

Hi Jacob,

On 9/30/2024 11:25 PM, Jacob Pan wrote:
> Hi Vasant,
> 
> On Thu, 26 Sep 2024 15:47:48 +0530
> Vasant Hegde <vasant.hegde@amd.com> wrote:
> 
>> Hi Jacob,
>>
>>
>> On 9/13/2024 9:32 AM, Jacob Pan wrote:
>>> Hi Vasant,
>>>
>>> On Wed, 11 Sep 2024 10:19:04 +0000
>>> Vasant Hegde <vasant.hegde@amd.com> wrote:
>>>   
>>>> @@ -1962,27 +1978,7 @@ static struct iommu_domain
>>>> *__iommu_domain_alloc(const struct iommu_ops *ops, if (!domain)
>>>>  		return ERR_PTR(-ENOMEM);
>>>>  
>>>> -	domain->type = type;
>>>> -	domain->owner = ops;
>>>> -	/*
>>>> -	 * If not already set, assume all sizes by default; the
>>>> driver
>>>> -	 * may override this later
>>>> -	 */
>>>> -	if (!domain->pgsize_bitmap)
>>>> -		domain->pgsize_bitmap = ops->pgsize_bitmap;
>>>> -
>>>> -	if (!domain->ops)
>>>> -		domain->ops = ops->default_domain_ops;
>>>> -
>>>> -	if (iommu_is_dma_domain(domain)) {
>>>> -		int rc;
>>>> -
>>>> -		rc = iommu_get_dma_cookie(domain);
>>>> -		if (rc) {
>>>> -			iommu_domain_free(domain);
>>>> -			return ERR_PTR(rc);
>>>> -		}
>>>> -	}
>>>> +	iommu_domain_init(domain, type, ops);
>>>>  	return domain;
>>>>  }
>>>>  
>>>> @@ -2965,6 +2961,14 @@ static int iommu_setup_default_domain(struct
>>>> iommu_group *group, if (group->default_domain == dom)
>>>>  		return 0;
>>>>  
>>>> +	if (iommu_is_dma_domain(dom)) {
>>>> +		ret = iommu_get_dma_cookie(dom);
>>>> +		if (ret) {
>>>> +			iommu_domain_free(dom);
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +  
>>> nit, if a user changes default domain types via sysfs and the
>>> current domain is already DMA API type, i.e. DMA to DMA-FQ, I think
>>> it causes an extra round of iommu_get/put_dma_cookie() compared
>>> with doing this in allocation time.  
>>
>> DMA -> DMA_FQ will just initialize flush queue and exit. It will not
>> enter iommu_setup_default_domain(). In case of DMA_FQ -> DMA it will
>> do new domain allocation. Am I missing something here?
> You are right. No extra get/put_dma_cookie there.
> My other concern is that this change made domain alloc/free asymmetrical
> in terms of get/put_dma_cookie.

That's correct. And this should be fine right? These are internal functions.

-Vasant

> 
> Currently, we have ftrace like this when I do:
> echo DMA > /sys/bus/pci/devices/0000\:38\:00.0/iommu_group/type
> 
>     iommu_setup_default_domain() {
>       __iommu_domain_alloc() {
>         intel_iommu_domain_alloc();
>         iommu_get_dma_cookie();
>       }
>       iommu_domain_free() {
>          iommu_put_dma_cookie();
>         intel_iommu_domain_free();
>        }
> 
> With this change, it will be:
>     iommu_setup_default_domain() {
>         iommu_get_dma_cookie();
> 	__iommu_domain_alloc() {
>         intel_iommu_domain_alloc();
>       }
>       iommu_domain_free() {
>          iommu_put_dma_cookie();
>         intel_iommu_domain_free();
>        }
>