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

[Jan Beulich <jbeulich@suse.com>]

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).

> --- /dev/null
> +++ b/xen/include/xen/iommu-private.h

I don't think private headers belong in this directory. From this patch
alone (including its description) it also doesn't become clear how (and
hence from where) this is intended to be used, which makes it hard to
suggest an alternative.

> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> + /*

Nit: Stray blank.

> + * xen/iommu-private.h
> + *
> + *

Nit: No double empty comment lines please.

> + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
> + * Written by Vikram Garhwal <vikram.garhwal@amd.com>
> + *
> + */
> +#ifndef __XEN_IOMMU_PRIVATE_H__
> +#define __XEN_IOMMU_PRIVATE_H__
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +#include <xen/device_tree.h>

Why (for both of the lines)? All you need is a forward declaration of
struct dt_device_node.

> +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);

Just bool please.

Jan