RE: [XEN][PATCH v6 14/19] common/device_tree: Add rwlock for dt_host

[Henry Wang]

Hi Vikram,

> -Original Message-
> Subject: [XEN][PATCH v6 14/19] common/device_tree: Add rwlock for dt_host
> 
>  Dynamic programming ops will modify the dt_host and there might be other
>  function which are browsing the dt_host at the same time. To avoid the race
>  conditions, adding rwlock for browsing the dt_host during runtime.

While now I understand why you use rwlock instead of spinlock in this patch
since you explained it in replying my comment in v5 (Thanks!). I would still
suggest that you can add that kind of explanation in the commit message to
make the commit message clear to everyone that reading this patch.

> 
> Signed-off-by: Vikram Garhwal 
> ---
>  xen/common/device_tree.c  |  4 
>  xen/drivers/passthrough/device_tree.c | 18 ++
>  xen/include/xen/device_tree.h |  6 ++
>  3 files changed, 28 insertions(+)
> 

[...]

>  ret = iommu_add_dt_device(dev);
>  if ( ret < 0 )
> @@ -310,6 +321,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl,
> struct domain *d,
>  printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign
> \"%s\""
> " to dom%u failed (%d)\n",
> dt_node_full_name(dev), d->domain_id, ret);
> +
> +read_unlock(&dt_host->lock);

Since you added "read_unlock(&dt_host->lock);" before the final return,
i.e. "return ret", I don't think you need to add "read_unlock(&dt_host->lock);"
here before the break. Or am I missing something?

>  break;
> 
>  case XEN_DOMCTL_deassign_device:
> @@ -328,11 +341,15 @@ int iommu_do_dt_domctl(struct xen_domctl
> *domctl, struct domain *d,
>  break;
> 
>  ret = xsm_deassign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
> +

Nit: Unnecessary blank line addition here.

Kind regards,
Henry

[XEN][PATCH v6 14/19] common/device_tree: Add rwlock for dt_host

[Vikram Garhwal]

 Dynamic programming ops will modify the dt_host and there might be other
 function which are browsing the dt_host at the same time. To avoid the race
 conditions, adding rwlock for browsing the dt_host during runtime.

Signed-off-by: Vikram Garhwal 
---
 xen/common/device_tree.c  |  4 
 xen/drivers/passthrough/device_tree.c | 18 ++
 xen/include/xen/device_tree.h |  6 ++
 3 files changed, 28 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 426a809f42..48cb68bcd9 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -2109,7 +2109,11 @@ int unflatten_device_tree(const void *fdt, struct 
dt_device_node **mynodes)
 
 dt_dprintk(" <- unflatten_device_tree()\n");
 
+/* Init r/w lock for host device tree. */
+rwlock_init(&dt_host->lock);
+
 return 0;
+
 }
 
 static void dt_alias_add(struct dt_alias_prop *ap,
diff --git a/xen/drivers/passthrough/device_tree.c 
b/xen/drivers/passthrough/device_tree.c
index 46f9080c8f..e3be8e3f91 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -111,6 +111,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);
@@ -118,10 +120,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;
 }
 
@@ -245,6 +251,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 +302,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct 
domain *d,
 spin_unlock(&dtdevs_lock);
 
 if ( d == dom_io )
+{
+read_unlock(&dt_host->lock);
 return -EINVAL;
+}
 
 ret = iommu_add_dt_device(dev);
 if ( ret < 0 )
@@ -310,6 +321,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct 
domain *d,
 printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign \"%s\""
" to dom%u failed (%d)\n",
dt_node_full_name(dev), d->domain_id, ret);
+
+read_unlock(&dt_host->lock);
 break;
 
 case XEN_DOMCTL_deassign_device:
@@ -328,11 +341,15 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct 
domain *d,
 break;
 
 ret = xsm_deassign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
+
 if ( ret )
 break;
 
 if ( d == dom_io )
+{
+read_unlock(&dt_host->lock);
 return -EINVAL;
+}
 
 ret = iommu_deassign_dt_device(d, dev);
 
@@ -347,5 +364,6 @@ 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 d6366d3dac..e616dd7e9c 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define DEVICE_TREE_MAX_DEPTH 16
 
@@ -106,6 +107,11 @@ struct dt_device_node {
 struct list_head domain_list;
 
 struct device dev;
+
+/*
+ * Lock that protects r/w updates to unflattened device tree i.e. dt_host.
+ */
+rwlock_t lock;
 };
 
 #define dt_to_dev(dt_node)  (&(dt_node)->dev)
-- 
2.17.1