Re: [PATCH v5 02/12] iommufd: Refactor __fault_domain_replace_dev() to be a wrapper of iommu_replace_group_handle()

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

On 2024/11/5 16:03, Baolu Lu wrote:
> On 2024/11/5 16:01, Yi Liu wrote:
>> On 2024/11/5 13:06, Baolu Lu wrote:
>>> On 11/4/24 21:25, Yi Liu wrote:
>>>> There is a wrapper of iommu_attach_group_handle(), so making a wrapper for
>>>> iommu_replace_group_handle() for further code refactor. No functional 
>>>> change
>>>> intended.
>>>
>>> This patch is not a simple, non-functional refactoring. It allocates
>>> attach_handle for all devices in domain attach/replace interfaces,
>>> regardless of whether the domain is iopf-capable. Therefore, the commit
>>> message should be rephrased to accurately reflect the patch's purpose
>>> and rationale.
>>
>> This patch splits the __fault_domain_replace_dev() a lot, the else branch 
>> of the below code was lifted to the iommufd_fault_domain_replace_dev().
>> While the new __fault_domain_replace_dev() will only be called when the
>> hwpt->fault is valid. So the iommu_attach_handle is still allocated only
>> for the iopf-capable path. When the hwpt->fault is invalid, the
>> iommufd_fault_domain_replace_dev() calls iommu_replace_group_handle() with
>> a null iommu_attach_handle. What you described is done in the patch 04 of
>> this series. 🙂
>>
>> -    if (hwpt->fault) {
>> -        handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>> -        if (!handle)
>> -            return -ENOMEM;
>> -
>> -        handle->idev = idev;
>> -        ret = iommu_replace_group_handle(idev->igroup->group,
>> -                         hwpt->domain, &handle->handle);
>> -    } else {
>> -        ret = iommu_replace_group_handle(idev->igroup->group,
>> -                         hwpt->domain, NULL);
>> -    }
> 
> Okay, I overlooked that part.
> 
> Below change caused me to think that attach handle is always allocated
> in this patch no matter ...

aha, because the caller of this chunk would check hwpt->fault first.

> 
> -    if (hwpt->fault) {
> -        handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> -        if (!handle)
> -            return -ENOMEM;
> -
> -        handle->idev = idev;
> -        ret = iommu_replace_group_handle(idev->igroup->group,
> -                         hwpt->domain, &handle->handle);
> -    } else {
> -        ret = iommu_replace_group_handle(idev->igroup->group,
> -                         hwpt->domain, NULL);
> -    }
> +    handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> +    if (!handle)
> +        return -ENOMEM;
> 
> If no functional change, please just ignore this comment.

yep, no functional change is intended in this patch. But do let me know if
you find one. :)

-- 
Regards,
Yi Liu