> -----Original Message-----
> From: Joerg Roedel <j...@8bytes.org>
> Sent: Friday, May 15, 2020 8:46 AM
> To: Lu Baolu <baolu...@linux.intel.com>
> Cc: Prakhya, Sai Praneeth <sai.praneeth.prak...@intel.com>;
> iommu@lists.linux-foundation.org
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
>
> On Fri, May 15, 2020 at 08:55:42PM +0800, Lu Baolu wrote:
> > It seems that we can do like this:
> >
> > [1] mutex_lock(&group->lock)
> > [2] for_each_group_device(device_lock())
> > [3] if (for_each_group_device(!device_is_bound()))
> > change_default_domain()
> > [4] for_each_group_device_reverse(device_unlock())
> > [5] mutex_unlock(&group->lock)
>> A possible problem exists at step 2 when another thread is trying to lock
>> devices in the reverse order at the same time. -> By Allen
Makes sense.. this could happen and we could prevent this if the iommu_group
has only one device. So, going with Joerg's suggestion, if we support dynamic
switching of default-domain of a group with only one device, I think we
shouldn't hit this issue.
> The problem here is that I am pretty sure we also have:
>
> device_lock() /* from device/driver core code */
> -> bus_notifier()
> -> iommu_bus_notifier()
> -> ...
> -> mutex_lock(&group->lock)
>
> Which would cause lock-inversion with the above code.
Thanks for this call chain, Joerg. It makes sense to me how lock inversion
could happen
iommu_bus_notifier()
-> iommu_release_device()
-> ops->release_device() (Eg: intel_iommu_release_device())
-> iommu_group_remove_device()
-> mutex_lock()
But, I added print statements to iommu_bus_notifier() and
iommu_group_remove_device() and noticed that iommu_group_remove_device() wasn't
being called upon modprobe -r <driver_name> and iommu_probe_device() isn't
called upon modprobe <driver_name> because
if (action == BUS_NOTIFY_ADD_DEVICE) {
int ret;
ret = iommu_probe_device(dev);
return (ret) ? NOTIFY_DONE : NOTIFY_OK;
} else if (action == BUS_NOTIFY_REMOVED_DEVICE) {
iommu_release_device(dev);
return NOTIFY_OK;
}
"action" was != BUS_NOTIFY_ADD_DEVICE || BUS_NOTIFY_REMOVED_DEVICE upon
modprobe. So (after booting [1]), I am wondering when would action be == to one
of the above so that these iommu functions get called.
And,
switch (action) {
case BUS_NOTIFY_BIND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_BIND_DRIVER;
break;
case BUS_NOTIFY_BOUND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_BOUND_DRIVER;
break;
case BUS_NOTIFY_UNBIND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_UNBIND_DRIVER;
break;
case BUS_NOTIFY_UNBOUND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER;
break;
}
if (group_action)
blocking_notifier_call_chain(&group->notifier,
group_action, dev);
I also noticed that vfio is the only one that registers for group notifiers and
from a first look it wasn't very obvious if vfio is trying to get group->mutex.
[1] I am interested in after booting because dynamic switching of iommu_group
default-domain is supported only through sysfs.
Regards,
Sai
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
