Re: [XEN][PATCH v10 14/20] common/device_tree: Add rwlock for dt_host

[Vikram Garhwal <vikram.garhwal@amd.com>]

On Fri, Aug 25, 2023 at 01:02:16AM -0700, Vikram Garhwal wrote:
> Dynamic programming ops will modify the dt_host and there might be other
> functions which are browsing the dt_host at the same time. To avoid the race
> conditions, adding rwlock for browsing the dt_host during runtime. dt_host
> writer will be added in the follow-up patch for device tree overlay
> functionalities."
> 
> Reason behind adding rwlock instead of spinlock:
>    For now, dynamic programming is the sole modifier of dt_host in Xen during
>        run time. All other access functions like iommu_release_dt_device() are
>        just reading the dt_host during run-time. So, there is a need to protect
>        others from browsing the dt_host while dynamic programming is modifying
>        it. rwlock is better suitable for this task as spinlock won't be able to
>        differentiate between read and write access.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> ---
> Changes from v9:
>     Update commit message and fix indentation.
>     Add ASSERT() for iommu_deassign_dt_device() and iommu_remove_dt_device().

Copy-pasting Julien's comment here for keeping comments with latest version:
"We also need to add ASSERT(system_state <= SYS_STATE_active || check lock);
in iommu_add_dt_device() and iommu_assign_dt_device()."

I will make the changes in v11.
Regards,
Vikram

>     Fix code styles.
>     Remove rwlock_init in unflatten_device_tree() and do DEFINE_RWLOCK in
>         device-tree.c
> Changes from v7:
>     Keep one lock for dt_host instead of lock for each node under dt_host.
> ---
> ---
>  xen/common/device_tree.c              |  1 +
>  xen/drivers/passthrough/device_tree.c | 24 ++++++++++++++++++++++--
>  xen/include/xen/device_tree.h         |  7 +++++++
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f38f51ec0b..b1c2952951 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -31,6 +31,7 @@ dt_irq_xlate_func dt_irq_xlate;
>  struct dt_device_node *dt_host;
>  /* Interrupt controller node*/
>  const struct dt_device_node *dt_interrupt_controller;
> +DEFINE_RWLOCK(dt_host_lock);
>  
>  /**
>   * struct dt_alias_prop - Alias property in 'aliases' node
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 3fad65fb69..b81dab5a48 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -62,6 +62,8 @@ int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev)
>      const struct domain_iommu *hd = dom_iommu(d);
>      int rc;
>  
> +    ASSERT(rw_is_locked(&dt_host_lock));
> +
>      if ( !is_iommu_enabled(d) )
>          return -EINVAL;
>  
> @@ -114,6 +116,8 @@ int iommu_release_dt_devices(struct domain *d)
>      if ( !is_iommu_enabled(d) )
>          return 0;
>  
> +    read_lock(&dt_host_lock);
> +
>      list_for_each_entry_safe(dev, _dev, &hd->dt_devices, domain_list)
>      {
>          rc = iommu_deassign_dt_device(d, dev);
> @@ -121,10 +125,14 @@ int iommu_release_dt_devices(struct domain *d)
>          {
>              dprintk(XENLOG_ERR, "Failed to deassign %s in domain %u\n",
>                      dt_node_full_name(dev), d->domain_id);
> +            read_unlock(&dt_host_lock);
> +
>              return rc;
>          }
>      }
>  
> +    read_unlock(&dt_host_lock);
> +
>      return 0;
>  }
>  
> @@ -134,6 +142,8 @@ int iommu_remove_dt_device(struct dt_device_node *np)
>      struct device *dev = dt_to_dev(np);
>      int rc;
>  
> +    ASSERT(rw_is_locked(&dt_host_lock));
> +
>      if ( !iommu_enabled )
>          return 1;
>  
> @@ -251,6 +261,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>      int ret;
>      struct dt_device_node *dev;
>  
> +    read_lock(&dt_host_lock);
> +
>      switch ( domctl->cmd )
>      {
>      case XEN_DOMCTL_assign_device:
> @@ -294,7 +306,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>          spin_unlock(&dtdevs_lock);
>  
>          if ( d == dom_io )
> -            return -EINVAL;
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
>  
>          ret = iommu_add_dt_device(dev);
>          if ( ret < 0 )
> @@ -332,7 +347,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>              break;
>  
>          if ( d == dom_io )
> -            return -EINVAL;
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
>  
>          ret = iommu_deassign_dt_device(d, dev);
>  
> @@ -347,5 +365,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>          break;
>      }
>  
> +    read_unlock(&dt_host_lock);
> +
>      return ret;
>  }
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 44d315c8ba..a262bba2ed 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -18,6 +18,7 @@
>  #include <xen/string.h>
>  #include <xen/types.h>
>  #include <xen/list.h>
> +#include <xen/rwlock.h>
>  
>  #define DEVICE_TREE_MAX_DEPTH 16
>  
> @@ -218,6 +219,12 @@ extern struct dt_device_node *dt_host;
>   */
>  extern const struct dt_device_node *dt_interrupt_controller;
>  
> +/*
> + * Lock that protects r/w updates to unflattened device tree i.e. dt_host during
> + * runtime. Lock may not be taken for boot only code.
> + */
> +extern rwlock_t dt_host_lock;
> +
>  /**
>   * Find the interrupt controller
>   * For the moment we handle only one interrupt controller: the first
> -- 
> 2.17.1
> 
>