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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 02/06/2023 02:48, Vikram Garhwal wrote:
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> Remove static type so this can also be used by SMMU drivers to check if the
> device is being used before removing.
> 
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign.

Would be nice to explain the need of putting a prototype into a private header.

> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> 
> ---
> Changes from v6:
>     Created a private header and moved iommu_dt_device_is_assigned() to 
> header.
> ---
>  xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++----
>  xen/include/xen/iommu-private.h       | 27 +++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 4 deletions(-)
>  create mode 100644 xen/include/xen/iommu-private.h
> 
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index 1c32d7b50c..52e370db01 100644
> --- 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)
I would write a comment that this function expects to be called with 
dtdevs_lock locked.

> +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;
>  
> diff --git a/xen/include/xen/iommu-private.h b/xen/include/xen/iommu-private.h
> new file mode 100644
> index 0000000000..5615decaff
> --- /dev/null
> +++ b/xen/include/xen/iommu-private.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> + /*
incorrect indentation (<< 1)

> + * xen/iommu-private.h
> + *
> + *
> + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
> + * Written by Vikram Garhwal <vikram.garhwal@xxxxxxx>
I'm not sure if placing the copyright is appropriate, given a single prototype 
for a function
not really implemented by you with just spinlocks removed. But I might be wrong.

> + *
> + */
> +#ifndef __XEN_IOMMU_PRIVATE_H__
> +#define __XEN_IOMMU_PRIVATE_H__
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +#include <xen/device_tree.h>
> +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);
> +#endif
> +
> +#endif /* __XEN_IOMMU_PRIVATE_H__ */
empty line here please

> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

~Michal