Re: [PATCH V2 3/5] iommu: Add support to change default domain of an iommu_group

[Lu Baolu <baolu.lu@linux.intel.com>]

On 2020/2/24 15:57, Prakhya, Sai Praneeth wrote:
>> On 2020/2/24 15:03, Prakhya, Sai Praneeth wrote:
>>>> On 2020/2/24 11:20, Prakhya, Sai Praneeth wrote:
>>>>>>> +	list_for_each_entry_safe(grp_dev, temp, &group->devices, list) {
>>>>>>> +		struct device *dev;
>>>>>>> +
>>>>>>> +		dev = grp_dev->dev;
>>>>>>> +		iommu_release_device(dev);
>>>>>>> +
>>>>>>> +		ret = iommu_group_add_device(group, dev);
>>>>>>> +		if (ret)
>>>>>>> +			dev_err(dev, "Failed to add to iommu group %d: %d\n",
>>>>>>> +				group->id, ret);
>>>>>> Need to handle this error case.
>>>>> I wasn't sure on how to handle the error ☹
>>>> Just roll back to the state before calling this function and return
>>>> an appropriate error value.
>>>>
>>>> The likely behavior is detaching the new domains from all devices (if
>>>> it has already attached), attaching the old domains to all devices in
>>>> the group,
>>> And while handling this error case, there is a possibility that attaching to old
>> domain could fail.. so, we might need to handle that error case as well. If we
>> plan to handle error case, since we have removed devices from group above,
>> adding them back could fail too.. that would lead into handling error case for an
>> error case.
>>
>> We can assume that the old domain should always be attached back.
>> Otherwise, there must be some bugs in the vendor iommu driver.
>>
>> It must be able to role back, otherwise users have to reboot the system in order
>> to use the devices in the group. This is not acceptable in the production kernel.
> I agree that we should be able to roll back but I am afraid that the error handling code could become complex than the usual code that gets to run. For example, assume there are 'n' devices in the group, 'k' of them are successfully processed (i.e. default domain type has been changed) and if k+1 fails while changing default domain type, to roll back state of k devices, we need to maintain a list of processed devices so that we now roll back state for devices in this list. The present code does not have any list because it's processing sequentially, it takes a device from the group, changes it domain and moves to other device and hence does not require a list.
> 
> All said, I could give this a try and see how complex the code could turn out to be. I hope I am wrong (i.e. turns out implementing error handling is simple).
> 

I think something like below should work.

static int iommu_group_do_attach_device(struct device *dev, void *data)
{
         struct iommu_domain *domain = data;

         return __iommu_attach_device(domain, dev);
}

static int __iommu_attach_group(struct iommu_domain *domain,
                                 struct iommu_group *group)
{
         int ret;

         if (group->default_domain && group->domain != 
group->default_domain)
                 return -EBUSY;

         ret = __iommu_group_for_each_dev(group, domain,
                                          iommu_group_do_attach_device);
         if (ret == 0)
                 group->domain = domain;

         return ret;
}

The vendor iommu driver should always deprecate the old domain if it
exists. Add a comment there.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu