| Message ID | 20230502233650.20121-15-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
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 <vikram.garhwal@amd.com> > --- > 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
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 <xen/string.h> #include <xen/types.h> #include <xen/list.h> +#include <xen/rwlock.h> #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)
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 <vikram.garhwal@amd.com> --- 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(+)