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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 25/08/2023 10:02, Vikram Garhwal wrote:
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> 
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> patches in this series introduces node add/remove feature.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> 
> ---
> Changes from v9:
>     Make iommu_dt_device_is_assigned_locked() static and delete header.
>     Move dtdevs_lock before iommu_dt_device_is_assigned_locked().
> Changes from v7:
>     Update commit message.
>     Add ASSERT().
> ---
> ---
>  xen/drivers/passthrough/device_tree.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index 1c32d7b50c..5d84c07b50 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -83,16 +83,17 @@ fail:
>      return rc;
>  }
>  
> -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> +static bool_t
> +iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
This does not apply cleanly due to recent change from bool_t to bool. Please 
rebase for v11 (the function
should then fit in a single line I think).

>  {
>      bool_t assigned = 0;
>  
> +    ASSERT(spin_is_locked(&dtdevs_lock));
> +
>      if ( !dt_device_is_protected(dev) )
>          return 0;
>  
> -    spin_lock(&dtdevs_lock);
>      assigned = !list_empty(&dev->domain_list);
> -    spin_unlock(&dtdevs_lock);
>  
>      return assigned;
>  }
> @@ -223,17 +224,24 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, 
> struct domain *d,
>          if ( ret )
>              break;
>  
> +        spin_lock(&dtdevs_lock);
Why is this lock placed here instead of ...
> +
>          if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
>          {
> -            if ( iommu_dt_device_is_assigned(dev) )
> +
... here, right before iommu_dt_device_is_assigned_locked()?
> +            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);
You could then remove this one.

With the remarks addressed:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal