> 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.
So, given the probability of these functions failing here are very low, I 
think, why not bite the bullet and say, add code to handle these error cases if 
we see that these functions are failing frequently? Otherwise the error 
handling code is just a dead code.

> cleaning
> up all new resources allocated in this function, putting a error message to 
> tell
> the user why it fails and returning an error code.
> 
> > i.e. group's domain/default_domain are already updated to new domain and
> assume there are 'n' devices in the group and this failed for 'k'th device, I 
> wasn't
> sure how I could roll back the changes made for k-1 devices.
> 
> A successful attach could be checked by (group->domain ==
> group->default_domain).

No.. because I have manually set group->domain == group->default_domain = 
new_domain (did this because iommu_group_add_device() and 
iommu_group_create_direct_mappings() needs them)
So, probably we might need some other way to check successful attach.

> > So, I thought probably just alert the user that there was an error while
> changing default domain type and try updating for other devices in the group
> (hopefully other devices might succeed). Also,*generally*  we shouldn't see 
> any
> errors here because all these devices were already in the same group earlier 
> (we
> aren’t adding/removing new devices to the group). We are just changing default
> domain type and we already made sure that device could be in the requested
> default domain type.
> >
> >>> +
> >>> +         ret = prv_dom->ops->add_device(dev);
> >>> +         if (ret)
> >>> +                 dev_err(dev, "Error adding to iommu: %d\n", ret);
> >> Ditto.
> >>
> >>> + }
> >>> +
> >>> + iommu_group_put(group);
> >>> + iommu_domain_free(prv_dom);
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int is_driver_bound(struct device *dev, void *not_used) {
> >>> + int ret = 0;
> >>> +
> >>> + device_lock(dev);
> >>> + if (device_is_bound(dev))
> >>> +         ret = 1;
> >>> + device_unlock(dev);
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static ssize_t iommu_group_store_type(struct iommu_group *group,
> >>> +                               const char *buf, size_t count) {
> >>> + int ret = 0, req_type = 0, req_auto = 0;
> >>> + struct iommu_domain *prv_dom;
> >>> + struct group_device *grp_dev;
> >>> + const struct iommu_ops *ops;
> >>> + struct device *dev;
> >>> +
> >>> + if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> >>> +         return -EACCES;
> >>> +
> >>> + if (WARN_ON(!group))
> >>> +         return -EINVAL;
> >>> +
> >>> + if (sysfs_streq(buf, "identity"))
> >>> +         req_type = IOMMU_DOMAIN_IDENTITY;
> >>> + else if (sysfs_streq(buf, "DMA"))
> >>> +         req_type = IOMMU_DOMAIN_DMA;
> >>> + else if (sysfs_streq(buf, "auto"))
> >>> +         req_auto = 1;
> >>> + else
> >>> +         return -EINVAL;
> >>> +
> >>> + /*
> >>> +  * Check if any device in the group still has a driver binded to it.
> >>> +  * This might race with device driver probing code and unfortunately
> >>> +  * there is no clean way out of that either, locking all devices in the
> >>> +  * group and then do the re-attach will introduce a lock-inversion with
> >>> +  * group->mutex - Joerg.
> >> Do you mean that we can't do below?
> >>
> >> mutex_lock(&group->mutex);
> >> for_each_group_device()
> >>    device_lock(dev);
> >>
> >> /* Default domain switch */
> >> for_each_group_device()
> >>    device_unlock()
> >> mutex_unlock(&group->mutex)
> > I think, Joerg talks about two issues here 1. is_driver_bound()
> > takes/releases device_lock() which could race with device driver probing 
> > code
> i.e. in an other terminal user could try to unload/load the module.
> 
> So you need to device_lock() all devices during the whole process.
> (I assume that load/unload device driver requiring this lock as well.
> Didn't check it in code. Please correct me if I misunderstood it.)

Ok.. I will check this.

> > 2. We cannot do as you suggested above because the functions (one is
> mentioned below) that switch default domain need to take
> iommu_group_mutex lock. So, taking the mutex and then calling iommu
> functions that switch default domain will dead lock.
> 
> You probably need to move the mutex_lock() in that function out to the caller?
> And only assert the lock has been held there.

I mean, there are many functions that are taking iommu_group_mutex_lock and 
they are being called as part of changing default domain.
So, we would then need to change all those functions and let callers of those 
functions know that now it's their responsibility to take the lock and call 
these functions.
Some of those functions are:

iommu_group_add_device
iommu_group_remove_device
iommu_group_for_each_dev

So, rather than that, I think it's better to delegate taking/releasing lock to 
individual functions than giving that responsibility to callers because then 
some callers might forget to take/release lock.

> >
> >>> +  */
> >>> + if (iommu_group_for_each_dev(group, NULL, is_driver_bound)) {
> >>> +         pr_err("Active drivers exist for devices in the group\n");
> >>> +         return -EBUSY;
> >>> + }
> >>> +
> >>> + mutex_lock(&group->mutex);
> >>> + prv_dom = group->default_domain;
> >>> + if (!prv_dom || !prv_dom->ops || !prv_dom->ops-
> >>> dev_def_domain_type) {
> >>> +         pr_err("'dev_def_domain_type' call back isn't registered\n");
> >>> +         ret = -EPERM;
> >>> +         goto out;
> >>> + }
> >>> +
> >>> + /*
> >>> +  * Check if any user level driver (that doesn't use kernel driver like
> >>> +  * VFIO) is directly using the group. VFIO use case is detected by
> >>> +  * is_driver_bound() check above
> >>> +  */
> >>> + if (prv_dom != group->domain) {
> >>> +         pr_err("Group assigned to user level for direct access\n");
> >>> +         ret = -EBUSY;
> >>> +         goto out;
> >>> + }
> >>> +
> >>> + /*
> >>> +  * If user has requested "auto", kernel has to decide the default domain
> >>> +  * type of a group. Hence, find out individual preferences of a device.
> >>> +  */
> >>> + ops = prv_dom->ops;
> >>> + if (req_auto) {
> >>> +         int dma_devs = 0, idt_devs = 0, dev_def_dom;
> >>> +
> >>> +         list_for_each_entry(grp_dev, &group->devices, list) {
> >>> +                 dev = grp_dev->dev;
> >>> +                 dev_def_dom = ops->dev_def_domain_type(dev);
> >>> +                 if (dev_def_dom == IOMMU_DOMAIN_IDENTITY)
> >>> +                         idt_devs++;
> >>> +                 if (dev_def_dom == IOMMU_DOMAIN_DMA)
> >>> +                         dma_devs++;
> >>> +         }
> >>> +
> >>> +         /*
> >>> +          * Default domain type of a group is undefined if some devices
> >>> +          * in the group should be in dma domain while other devices
> >>> +          * should be in identity domain
> >>> +          */
> >>> +         if (idt_devs && dma_devs) {
> >>> +                 pr_err("Some devices need identity domain while other
> >> need dma domain\n");
> >>> +                 ret = -EPERM;
> >>> +                 goto out;
> >>> +         }
> >>> +
> >>> +         /*
> >>> +          * Default domain type of a group is identity if
> >>> +          * 1. All the devices in the group need to be in identity domain
> >>> +          * 2. Some devices should be in identity domain while other
> >>> +          *    devices could be in either of dma or identity domain
> >>> +          */
> >>> +         if (idt_devs && !dma_devs)
> >>> +                 req_type = IOMMU_DOMAIN_IDENTITY;
> >>> +
> >>> +         /*
> >>> +          * Default domain type of a group is dma if
> >>> +          * 1. All the devices in the group need to be in dma domain
> >>> +          * 2. Some devices should be in dma domain while other devices
> >>> +          *    could be in either of dma or identity domain
> >>> +          * 3. All the devices could be in either of the domains (namely
> >>> +          *    dma and identity)
> >>> +          */
> >>> +         if (!idt_devs)
> >>> +                 req_type = IOMMU_DOMAIN_DMA;
> >> Actually, There are four combinations:
> >>
> >>                    idt_devs==0             idt_devs!=0
> >> dma_devs == 0              iommu_def_domain_type   identity
> >> dma_devs != 0              DMA                     unsupported
> > Yes.. you are right. All these four combinations are presently handled in 
> > the
> code.
> 
> At least I didn't see the iommu_def_domain_type is used if both idt_devs and
> dma_devs are both equal to 0. :-)

Sorry! I didn't get what you meant by "iommu_def_domain_type", so, could you 
please explain that?

When idt_devs == 0 and dma_devs == 0, it means that all the devices in the 
group could be in either of the domain. Hence, I default to DMA domain which is 
covered by this code.
                if (!idt_devs)
                        req_type = IOMMU_DOMAIN_DMA;

> > The comment above talks about combinations that lead us to select DMA
> domain.
> >
> > The cases you mentioned above are handled as:
> > 1. dma_devs == 0 && idt_devs == 0 -> In code, req_type =
> IOMMU_DOMAIN_DMA; because this means*all*  the devices in the group can
> be in either of identity domain or DMA domain, hence select DMA domain over
> identity domain.
> > 2. dma_devs != 0 && idt_devs == 0 -> In code, req_type =
> IOMMU_DOMAIN_DMA; because this means some devices in the
> group*should*  be in DMA domain and other devices in the group could be in
> either of identity domain or DMA domain.
> > 3. dma_devs == 0 && idt_devs != 0 -> In code, req_type =
> IOMMU_DOMAIN_IDENTITY; because this means some devices in the
> group*should*  be in identity domain and other devices in the group could be 
> in
> either of identity domain or DMA domain.
> > 4. dma_devs != 0 && idt_devs != 0 -> In code, return error, because this 
> > means
> some devices in the group*should*  be in identity domain and other devices in
> the group*should*  be in DMA domain. This combination is not expected. This
> situation shouldn't arise because this would have failed during boot itself
> because these kind of devices aren't grouped together in the first place.

Regards,
Sai
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Reply via email to