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 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..1f9cfccf95 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.1On 01/09/2023 06:59, 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> > --- > 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..1f9cfccf95 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)); This looks not right (I know Julien suggested this). The second part will be checked only if state > active i.e. suspend/resume. I think this wants to be: ASSERT(system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock)); so that once the state is >= active, we require dt_host_lock to be locked. ~Michal
On Mon, 4 Sep 2023, Michal Orzel wrote: > On 01/09/2023 06:59, 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> > > --- > > 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..1f9cfccf95 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)); > This looks not right (I know Julien suggested this). The second part will be checked only if state > active i.e. suspend/resume. > I think this wants to be: > ASSERT(system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock)); > so that once the state is >= active, we require dt_host_lock to be locked. Well spotted!
Hi, On Mon, Sep 04, 2023 at 01:09:52PM +0200, Michal Orzel wrote: > > > On 01/09/2023 06:59, 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> > > --- > > 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..1f9cfccf95 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)); > This looks not right (I know Julien suggested this). The second part will be checked only if state > active i.e. suspend/resume. > I think this wants to be: > ASSERT(system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock)); > so that once the state is >= active, we require dt_host_lock to be locked. I rechecked this, you are right! Will update it to check the lock only for >= active. > > ~Michal
© 2016 - 2026 Red Hat, Inc.