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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 02/06/2023 01: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.
Can you explain if you are trying to resolve an existing bug, or this is 
something that will be necessary in a follow-up patch?
> 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)
> +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
>   {
Please add an ASSERT() checking the lock is taken.

>       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);
> +
'dtdevs_lock' was intended to protect modification related to any IOMMU 
change. But here...
>           ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>                                       domctl->u.assign_device.u.dt.size,
>                                       &dev);
... you also include "dt_find_node_by_gpath". Can you explain why and 
add a comment on top of 'dtdevs_lock' to explain what it is intended use?
>           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 */
> + /*
> + * xen/iommu-private.h
> + *
> + *
> + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
> + * Written by Vikram Garhwal <vikram.garhwal@xxxxxxx>
> + *
> + */
> +#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__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
--
Julien Grall