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 16:12, Lu Baolu wrote:
> 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.
>

By the way, this is the expected behavior. Please check
__iommu_detach_group().

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