Re: [XEN][PATCH v7 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.06.2023 11:19, Jan Beulich wrote:
> On 02.06.2023 02:48, Vikram Garhwal wrote:
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -18,6 +18,7 @@
>>  #include <xen/device_tree.h>
>>  #include <xen/guest_access.h>
>>  #include <xen/iommu.h>
>> +#include <xen/iommu-private.h>
>>  #include <xen/lib.h>
>>  #include <xen/sched.h>
>>  #include <xsm/xsm.h>
>> @@ -83,16 +84,14 @@ fail:
>>      return rc;
>>  }
>>  
>> -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
>> +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
>>  {
>>      bool_t assigned = 0;
>>  
>>      if ( !dt_device_is_protected(dev) )
>>          return 0;
>>  
>> -    spin_lock(&dtdevs_lock);
>>      assigned = !list_empty(&dev->domain_list);
>> -    spin_unlock(&dtdevs_lock);
>>  
>>      return assigned;
>>  }
>> @@ -213,27 +212,40 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, 
>> struct domain *d,
>>          if ( (d && d->is_dying) || domctl->u.assign_device.flags )
>>              break;
>>  
>> +        spin_lock(&dtdevs_lock);
>> +
>>          ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>>                                      domctl->u.assign_device.u.dt.size,
>>                                      &dev);
>>          if ( ret )
>> +        {
>> +            spin_unlock(&dtdevs_lock);
>>              break;
>> +        }
>>  
>>          ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
>>          if ( ret )
>> +        {
>> +            spin_unlock(&dtdevs_lock);
>>              break;
>> +        }
>>  
>>          if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
>>          {
>> -            if ( iommu_dt_device_is_assigned(dev) )
>> +
>> +            if ( iommu_dt_device_is_assigned_locked(dev) )
>>              {
>>                  printk(XENLOG_G_ERR "%s already assigned.\n",
>>                         dt_node_full_name(dev));
>>                  ret = -EINVAL;
>>              }
>> +
>> +            spin_unlock(&dtdevs_lock);
>>              break;
>>          }
>>  
>> +        spin_unlock(&dtdevs_lock);
>> +
>>          if ( d == dom_io )
>>              return -EINVAL;
>>  
> 
> I'm having trouble seeing how this patch can be correct: How do present
> callers of iommu_dt_device_is_assigned() continue to build? I can only
> guess that a new wrapper of this name is introduced in an earlier or
> later patch, but thus breaking bisectability (either here or, if the
> wrapper is added in an earlier patch, there).

Oh, I'm sorry - looks like I overlooked that the 2nd hunk alters the
(sole) caller. Somehow I was under the (wrong) impression that both
hunks fiddle with the same function.

Jan