| Message ID | 20230411191636.26926-13-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 v5 12/17] 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. For clarity, could you please add a little bit more details to explain why you chose rwlock instead of normal spinlock? > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > --- > xen/common/device_tree.c | 3 +++ > xen/drivers/passthrough/device_tree.c | 39 +++++++++++++++++++++++++++ > xen/include/xen/device_tree.h | 6 +++++ > 3 files changed, 48 insertions(+) > > if ( ret ) > + { > 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); > + } I am not sure if it is necessary to add "{" and "}" here. > + > + read_unlock(&dt_host->lock); > break; > > case XEN_DOMCTL_deassign_device: > @@ -322,25 +345,41 @@ int iommu_do_dt_domctl(struct xen_domctl > *domctl, struct domain *d, > if ( domctl->u.assign_device.flags ) > break; > > + read_lock(&dt_host->lock); > + > ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path, > domctl->u.assign_device.u.dt.size, > &dev); > if ( ret ) > + { > + read_unlock(&dt_host->lock); I think instead of adding "read_unlock" in every break and return path, you can... > break; > + } > > ret = xsm_deassign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev)); > + > if ( ret ) > + { > + read_unlock(&dt_host->lock); > break; > + } > > if ( d == dom_io ) > + { > + read_unlock(&dt_host->lock); > return -EINVAL; ...do something like: ret = -EINVAL; break; here, and then add one single "read_unlock" before the "return ret;" in the bottom of the function? > + } > > ret = iommu_deassign_dt_device(d, dev); > > if ( ret ) > + { > 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); > + } Same here. I am not sure if it is necessary to add "{" and "}". Kind regards, Henry
Hi Henry, On 4/13/23 7:09 PM, Henry Wang wrote: > Hi Vikram, > >> -----Original Message----- >> Subject: [XEN][PATCH v5 12/17] 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. > For clarity, could you please add a little bit more details to explain why you chose > rwlock instead of normal spinlock? rwlock is just to protect someone reading the dt_host while dynamic programming is modifying the dt_host. Initial suggestion was from Julien about adding rwlock here. For now, dynamic programming is the dt_host writer in Xen during run time. All other iommu passthrough function was just readers during run-time. So, it was better to go for r/w locks here as spinlock won't be able to difference between read and r/w access. For next version, I will add a comment in commit message. >> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >> --- >> xen/common/device_tree.c | 3 +++ >> xen/drivers/passthrough/device_tree.c | 39 +++++++++++++++++++++++++++ >> xen/include/xen/device_tree.h | 6 +++++ >> 3 files changed, 48 insertions(+) >> >> if ( ret ) >> + { >> 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); >> + } > I am not sure if it is necessary to add "{" and "}" here. You are right. Will remove it in next version. > >> + >> + read_unlock(&dt_host->lock); >> break; >> >> case XEN_DOMCTL_deassign_device: >> @@ -322,25 +345,41 @@ int iommu_do_dt_domctl(struct xen_domctl >> *domctl, struct domain *d, >> if ( domctl->u.assign_device.flags ) >> break; >> >> + read_lock(&dt_host->lock); >> + >> ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path, >> domctl->u.assign_device.u.dt.size, >> &dev); >> if ( ret ) >> + { >> + read_unlock(&dt_host->lock); > I think instead of adding "read_unlock" in every break and return path, > you can... > >> break; >> + } >> >> ret = xsm_deassign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev)); >> + >> if ( ret ) >> + { >> + read_unlock(&dt_host->lock); >> break; >> + } >> >> if ( d == dom_io ) >> + { >> + read_unlock(&dt_host->lock); >> return -EINVAL; > ...do something like: > > ret = -EINVAL; > break; > > here, and then add one single "read_unlock" before the "return ret;" > in the bottom of the function? Will do. > >> + } >> >> ret = iommu_deassign_dt_device(d, dev); >> >> if ( ret ) >> + { >> 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); >> + } > Same here. I am not sure if it is necessary to add "{" and "}". > Kind regards, > Henry
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 507b4ac5b6..b330e6a013 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -2098,6 +2098,9 @@ void unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes) *allnextp = NULL; dt_dprintk(" <- unflatten_device_tree()\n"); + + /* Init r/w lock for host device tree. */ + rwlock_init(&dt_host->lock); } 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 a77a217f3d..74f994b9da 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -112,6 +112,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); @@ -119,10 +121,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; } @@ -259,11 +265,15 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, spin_lock(&dtdevs_lock); + read_lock(&dt_host->lock); + ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path, domctl->u.assign_device.u.dt.size, &dev); if ( ret ) { + read_unlock(&dt_host->lock); + spin_unlock(&dtdevs_lock); break; @@ -272,6 +282,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev)); if ( ret ) { + read_unlock(&dt_host->lock); + spin_unlock(&dtdevs_lock); break; @@ -287,6 +299,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, ret = -EINVAL; } + read_unlock(&dt_host->lock); + spin_unlock(&dtdevs_lock); break; @@ -295,22 +309,31 @@ 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 ) { printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n", dt_node_full_name(dev)); + + read_unlock(&dt_host->lock); break; } ret = iommu_assign_dt_device(d, dev); if ( ret ) + { 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: @@ -322,25 +345,41 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, if ( domctl->u.assign_device.flags ) break; + read_lock(&dt_host->lock); + ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path, domctl->u.assign_device.u.dt.size, &dev); if ( ret ) + { + read_unlock(&dt_host->lock); break; + } ret = xsm_deassign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev)); + if ( ret ) + { + read_unlock(&dt_host->lock); break; + } if ( d == dom_io ) + { + read_unlock(&dt_host->lock); return -EINVAL; + } ret = iommu_deassign_dt_device(d, dev); if ( ret ) + { 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; default: diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index 998f972ebc..b7272bbccc 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 | 3 +++ xen/drivers/passthrough/device_tree.c | 39 +++++++++++++++++++++++++++ xen/include/xen/device_tree.h | 6 +++++ 3 files changed, 48 insertions(+)