[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Vikram Garhwal <vikram.garhwal@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Michal Orzel <michal.orzel@xxxxxxx>
  • Date: Fri, 2 Jun 2023 09:45:17 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=/T0hDm9iVFs+fVU4clQnFY0EpKmEM1s4nlNkyk4Nk+A=; b=KR2q/YRdL3YuVK/9e9GpE0njXPXOQKSNVzECiIcm6KGfq8gi/tXHdtW6h1Jx8qMsJpmn+h8ecYxvSCqBEH5WY1osH6c8yrJI5e2E6gT/LlGGmyywfRgREgwHC3kBorxrP/5c8cKdbvOKSoEiIpq9AIHfqNwosilMFuP5gdpTnKwF/Fupc1oImiYqMwlL4a/Ot9Df1gwUNugPWsoijetlJqKmy+T7kr6efTqEF344kz2sqQ79R5v+Y5HOIhRSpiaSLmGiasTziitFW3xir8dkFN7oLb5NgSc25iXHi2i0Aokp7U8iMDByviLYztxYVJ2DyjubxGW1ZmTJg4XQl4QHOQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=SirzQ/im7Aovl7Nb68+ks55pqwcHM3KyF9k85GcpR+MrP28rEX+gLY+R6RBeauP0u5Ri+JC1Hj0/cXiML7djC7r7M1vP9JW0X63fZrf0oxIsVa+dPtU5hZoOrJ/Q+zd6UEbBGoR2d6kh0gAVF3TbkffWUhbS6JFvl4b2S9xbG/vKK4jagL3kH9P2rfCSJB6ap8UaONThvn2mkP+J/KBWOvsDfIux+c7h6lMYrqE3JLAubFHmtM/rtsdD1zvC7kGTT/KuIbn6eTHJ0MKsxSyKMmAwVoGl3JssLnvN/Kfk8OdLpbaBw5DPLtLhXCMo6DOWzvGPSaeW0Gq+zPUtuKrbOw==
  • Cc: <sstabellini@xxxxxxxxxx>, <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Fri, 02 Jun 2023 07:45:44 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.