[XEN][PATCH v9 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. dt_host
writer will be added in the follow-up patch titled "xen/arm: Implement device
tree node addition 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
---
Changes from v7:
Keep one lock for dt_host instead of lock for each node under dt_host.
---
---
xen/common/device_tree.c | 5 +
xen/drivers/passthrough/device_tree.c | 15 +++
xen/include/xen/device_tree.h | 6 ++
3 files changed, 26 insertions(+)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 0f10037745..6b934fe036 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;
+rwlock_t dt_host_lock;
/**
* struct dt_alias_prop - Alias property in 'aliases' node
@@ -2137,7 +2138,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 4cb32dc0b3..31815d2b60 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -114,6 +114,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);
@@ -121,10 +123,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;
}
@@ -251,6 +257,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:
@@ -304,7 +312,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 )
@@ -342,7 +353,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct
domain *d,
break;
if ( d == dom_io )
+{
+read_unlock(&dt_host_lock);
return -EINVAL;
+}
ret = iommu_deassign_dt_device(d, dev);
@@ -357,5 +371,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 e507658b23..8191f30197 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
@@ -216,6 +217,11 @@ 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.
+ */
+extern rwlock_t dt_host_lock;
+
/**
* Find the interrupt controller
* For the moment we handle only one interrupt controller: the first
--
2.17.1
Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host
Hi Vikram,
On 19/08/2023 01:28, Vikram Garhwal wrote:
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
Typo: I think you want to write 'functions'
conditions, adding rwlock for browsing the dt_host during runtime. dt_host
writer will be added in the follow-up patch titled "xen/arm: Implement device
tree node addition functionalities."
I would prefer if we avoid mention the name of the follow-up commit.
This will reduce the risk that the name of the commit is incorrect if we
decide to commit this patch before the rest of the series is ready.
Also, the commit message seems to be indented. Was it intended?
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.
The indentation looks odd here as well.
Signed-off-by: Vikram Garhwal
---
Changes from v7:
Keep one lock for dt_host instead of lock for each node under dt_host.
---
---
xen/common/device_tree.c | 5 +
xen/drivers/passthrough/device_tree.c | 15 +++
xen/include/xen/device_tree.h | 6 ++
3 files changed, 26 insertions(+)
I am not sue where to put the comment. I noticed that you didn't touch
iommu_remove_dt_device() and iommu_add_dt_device(). Does this mean the
caller is expected to held the lock? If so, then this should be
documented and an ASSERT() should be added.
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 0f10037745..6b934fe036 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;
+rwlock_t dt_host_lock;
/**
* struct dt_alias_prop - Alias property in 'aliases' node
@@ -2137,7 +2138,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);
Calling rwlock_init() from unflattent_device_tree() seems to be
incorrect as it would lead to re-initialize the lock every time we are
create a new DT overlay.
Instead you want to replace the definition of dt_host_lock with:
DEFINE_RWLOCK(dt_host_lock)
+
return 0;
+
Spurious change?
}
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 4cb32dc0b3..31815d2b60 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -114,6 +114,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);
So iommu_deassign_dt_device() is now called with the read lock. I am
assuming the intention is all the caller will need to fist held the
lock. If so, then I think this would require an ASSERT() in
iommu_deassign_dt_device() and a comment on top of the function because
it is exported.
I am guessing that iommu_assign_dt_device() is in the same situation.
@@ -121,10 +123,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);
Coding style: Usually we add the newline before the return. So I would
switch around the two lines.
return rc;
}
}
+read_unlock(&dt_host_lock);
+
return 0;
}
@@ -251,6 +257,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:
@@ -304,7 +312,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;
NIT: Rather than adding the unlock here, you could use:
rc = -EINVAL;
break;
+}
ret = iommu_add_dt_device(dev);
if ( ret < 0 )
@@ -342,7 +353,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct
Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host
Hi Julien,
On Wed, Aug 23, 2023 at 11:06:59PM +0100, Julien Grall wrote:
> Hi Vikram,
>
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> > 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
>
> Typo: I think you want to write 'functions'
>
> > conditions, adding rwlock for browsing the dt_host during runtime. dt_host
> > writer will be added in the follow-up patch titled "xen/arm: Implement
> > device
> > tree node addition functionalities."
>
> I would prefer if we avoid mention the name of the follow-up commit. This
> will reduce the risk that the name of the commit is incorrect if we decide
> to commit this patch before the rest of the series is ready.
>
> Also, the commit message seems to be indented. Was it intended?
>
> >
> > 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.
>
> The indentation looks odd here as well.
>
Changed above comments in v10.
> >
> > Signed-off-by: Vikram Garhwal
> >
> > ---
> > Changes from v7:
> > Keep one lock for dt_host instead of lock for each node under dt_host.
> > ---
> > ---
> > xen/common/device_tree.c | 5 +
> > xen/drivers/passthrough/device_tree.c | 15 +++
> > xen/include/xen/device_tree.h | 6 ++
> > 3 files changed, 26 insertions(+)
>
> I am not sue where to put the comment. I noticed that you didn't touch
> iommu_remove_dt_device() and iommu_add_dt_device(). Does this mean the
> caller is expected to held the lock? If so, then this should be documented
> and an ASSERT() should be added.
Added ASSERT in iommu_(add,remove,assign and deassign)_dt_device(),
>
> >
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 0f10037745..6b934fe036 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;
> > +rwlock_t dt_host_lock;
> > /**
> >* struct dt_alias_prop - Alias property in 'aliases' node
> > @@ -2137,7 +2138,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);
>
> Calling rwlock_init() from unflattent_device_tree() seems to be incorrect as
> it would lead to re-initialize the lock every time we are create a new DT
> overlay.
>
> Instead you want to replace the definition of dt_host_lock with:
>
> DEFINE_RWLOCK(dt_host_lock)
>
Changed this. DEFINE_RWLOCK is added to device-tree.c and this is removed.
> > +
> > return 0;
> > +
>
> Spurious change?
>
> > }
> > 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 4cb32dc0b3..31815d2b60 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -114,6 +114,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);
>
> So iommu_deassign_dt_device() is now called with the read lock. I am
> assuming the intention is all the caller will need to fist held the lock. If
> so, then I think this would require an ASSERT() in
> iommu_deassign_dt_device() and a comment on top of the function because it
> is exported.
>
> I am guessing that iommu_assign_dt_device() is in the same situation.
>
>
> > @@ -121,10 +123,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);
>
> Coding style: Usually we add the newline before the return. So I would
> switch around the two lines.
>
> > return rc;
> > }
> > }
> > +read_unlock(&dt_host_lock);
> > +
> > return 0;
> > }
> > @@ -251,6 +257,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl,
> > struct domain *d,
> > int ret;
> > s
Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host
Hi,
On Thu, Aug 24, 2023 at 11:22:00PM -0700, Vikram Garhwal wrote:
> Hi Julien,
> On Wed, Aug 23, 2023 at 11:06:59PM +0100, Julien Grall wrote:
> > Hi Vikram,
> >
> > On 19/08/2023 01:28, Vikram Garhwal wrote:
> > > 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
> >
> > Typo: I think you want to write 'functions'
> >
> > > conditions, adding rwlock for browsing the dt_host during runtime.
> > > dt_host
> > > writer will be added in the follow-up patch titled "xen/arm: Implement
> > > device
> > > tree node addition functionalities."
> >
> > I would prefer if we avoid mention the name of the follow-up commit. This
> > will reduce the risk that the name of the commit is incorrect if we decide
> > to commit this patch before the rest of the series is ready.
> >
> > Also, the commit message seems to be indented. Was it intended?
> >
> > >
> > > 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.
> >
> > The indentation looks odd here as well.
> >
> Changed above comments in v10.
> > >
> > > Signed-off-by: Vikram Garhwal
> > >
> > > ---
> > > Changes from v7:
> > > Keep one lock for dt_host instead of lock for each node under
> > > dt_host.
> > > ---
> > > ---
> > > xen/common/device_tree.c | 5 +
> > > xen/drivers/passthrough/device_tree.c | 15 +++
> > > xen/include/xen/device_tree.h | 6 ++
> > > 3 files changed, 26 insertions(+)
> >
> > I am not sue where to put the comment. I noticed that you didn't touch
> > iommu_remove_dt_device() and iommu_add_dt_device(). Does this mean the
> > caller is expected to held the lock? If so, then this should be documented
> > and an ASSERT() should be added.
> Added ASSERT in iommu_(add,remove,assign and deassign)_dt_device(),
iommu_add_ and iommu_assign_ are called at boot time. Also, only other callers
are handle_device via overlays and iommu_do_dt_domctl() which will hold the
dt_host_lock. Will look into it more but for now sending v10 with ASSER in these
two functions.
> >
> > >
> > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > > index 0f10037745..6b934fe036 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;
> > > +rwlock_t dt_host_lock;
> > > /**
> > >* struct dt_alias_prop - Alias property in 'aliases' node
> > > @@ -2137,7 +2138,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);
> >
> > Calling rwlock_init() from unflattent_device_tree() seems to be incorrect as
> > it would lead to re-initialize the lock every time we are create a new DT
> > overlay.
> >
> > Instead you want to replace the definition of dt_host_lock with:
> >
> > DEFINE_RWLOCK(dt_host_lock)
> >
> Changed this. DEFINE_RWLOCK is added to device-tree.c and this is removed.
> > > +
> > > return 0;
> > > +
> >
> > Spurious change?
> >
> > > }
> > > 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 4cb32dc0b3..31815d2b60 100644
> > > --- a/xen/drivers/passthrough/device_tree.c
> > > +++ b/xen/drivers/passthrough/device_tree.c
> > > @@ -114,6 +114,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);
> >
> > So iommu_deassign_dt_device() is now called with the read lock. I am
> > assuming the intention is all the caller will need to fist held the lock. If
> > so, then I think this would require an ASSERT() in
> > iommu_deassign_dt_device() and a comment on top of the function because it
> > is exported.
> >
> > I am guessing that iommu_assign_dt_device() is in the same situation.
> >
> >
> > > @@ -121,10 +123,14 @@ int iommu_release_dt_devices(struct domain *d)
> > >
Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host
Hi, On 25/08/2023 08:52, Vikram Garhwal wrote: Hi, On Thu, Aug 24, 2023 at 11:22:00PM -0700, Vikram Garhwal wrote: Hi Julien, On Wed, Aug 23, 2023 at 11:06:59PM +0100, Julien Grall wrote: Hi Vikram, On 19/08/2023 01:28, Vikram Garhwal wrote: 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 Typo: I think you want to write 'functions' conditions, adding rwlock for browsing the dt_host during runtime. dt_host writer will be added in the follow-up patch titled "xen/arm: Implement device tree node addition functionalities." I would prefer if we avoid mention the name of the follow-up commit. This will reduce the risk that the name of the commit is incorrect if we decide to commit this patch before the rest of the series is ready. Also, the commit message seems to be indented. Was it intended? 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. The indentation looks odd here as well. Changed above comments in v10. Signed-off-by: Vikram Garhwal --- Changes from v7: Keep one lock for dt_host instead of lock for each node under dt_host. --- --- xen/common/device_tree.c | 5 + xen/drivers/passthrough/device_tree.c | 15 +++ xen/include/xen/device_tree.h | 6 ++ 3 files changed, 26 insertions(+) I am not sue where to put the comment. I noticed that you didn't touch iommu_remove_dt_device() and iommu_add_dt_device(). Does this mean the caller is expected to held the lock? If so, then this should be documented and an ASSERT() should be added. Added ASSERT in iommu_(add,remove,assign and deassign)_dt_device(), iommu_add_ and iommu_assign_ are called at boot time. Also, only other callers are handle_device via overlays and iommu_do_dt_domctl() which will hold the dt_host_lock. The goal of the ASSERT() is to confirm that this holds true today and in the future. Will look into it more but for now sending v10 with ASSER in these two functions. You could have at least written a comment on top... Regarding the boot function, I would consider to take the lock there too. Otherwise, you could use: ASSERT(system_state <= SYS_STATE_active || check lock); Cheers, -- Julien Grall
