| Message ID | 20230906011631.30310-15-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
On Tue, 5 Sep 2023, 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> > Reviewed-by: Michal Orzel <michal.orzel@amd.com> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes from v11: > Update check state >= active for checking dt_host_lock to be locked > Changes from v10: > Add ASSERT for iommu_assign_dt_device() and iommu_add_dt_device(). > Changes from v9: > Update commit message and fix indentation. > Add ASSERT() for iommu_deassign_dt_device() and iommu_remove_dt_device(). > 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 | 28 +++++++++++++++++++++++++-- > xen/include/xen/device_tree.h | 7 +++++++ > 3 files changed, 34 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 80f6efc606..075fb25a37 100644 > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -31,6 +31,8 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev) > int rc = -EBUSY; > struct domain_iommu *hd = dom_iommu(d); > > + ASSERT(system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock)); > + > if ( !is_iommu_enabled(d) ) > return -EINVAL; > > @@ -62,6 +64,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; > > @@ -113,6 +117,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); > @@ -120,10 +126,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; > } > > @@ -133,6 +143,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; > > @@ -177,6 +189,8 @@ int iommu_add_dt_device(struct dt_device_node *np) > struct device *dev = dt_to_dev(np); > int rc = 1, index = 0; > > + ASSERT(system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock)); > + > if ( !iommu_enabled ) > return 1; > > @@ -249,6 +263,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: > @@ -289,7 +305,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, > } > > if ( d == dom_io ) > - return -EINVAL; > + { > + ret = -EINVAL; > + break; > + } > > ret = iommu_add_dt_device(dev); > if ( ret < 0 ) > @@ -327,7 +346,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); > > @@ -342,5 +364,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 >
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 80f6efc606..075fb25a37 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -31,6 +31,8 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev) int rc = -EBUSY; struct domain_iommu *hd = dom_iommu(d); + ASSERT(system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock)); + if ( !is_iommu_enabled(d) ) return -EINVAL; @@ -62,6 +64,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; @@ -113,6 +117,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); @@ -120,10 +126,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; } @@ -133,6 +143,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; @@ -177,6 +189,8 @@ int iommu_add_dt_device(struct dt_device_node *np) struct device *dev = dt_to_dev(np); int rc = 1, index = 0; + ASSERT(system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock)); + if ( !iommu_enabled ) return 1; @@ -249,6 +263,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: @@ -289,7 +305,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, } if ( d == dom_io ) - return -EINVAL; + { + ret = -EINVAL; + break; + } ret = iommu_add_dt_device(dev); if ( ret < 0 ) @@ -327,7 +346,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); @@ -342,5 +364,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
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> Reviewed-by: Michal Orzel <michal.orzel@amd.com> --- Changes from v11: Update check state >= active for checking dt_host_lock to be locked Changes from v10: Add ASSERT for iommu_assign_dt_device() and iommu_add_dt_device(). Changes from v9: Update commit message and fix indentation. Add ASSERT() for iommu_deassign_dt_device() and iommu_remove_dt_device(). 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 | 28 +++++++++++++++++++++++++-- xen/include/xen/device_tree.h | 7 +++++++ 3 files changed, 34 insertions(+), 2 deletions(-)