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 <vikram.garh...@amd.com>
---
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
domain *d,
break;
if ( d == dom_io )
+ {
+ read_unlock(&dt_host_lock);
return -EINVAL;
+ }
NIT: Same here.
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);
Coding style: Please add a newline.
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 <xen/string.h>
#include <xen/types.h>
#include <xen/list.h>
+#include <xen/rwlock.h>
#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.
+ */
The wording suggests that any update to any node would require to hold
the write lock. However.. it looks like you are only holding the read
when setting is_protected in the SMMU remove callback. Is this intended?
Or maybe you expect is_protected by to protected by dtdevs_lock? If so,
then I think it would be good to spell it out. Possibly on top of
is_protected.
Lastly, there are plenty of place in Xen where the lock is not taken.
They mostly seem to be at boot, so I would mention that for boot only
code, then lock may not be taken.
Lastly, this is a single line comment, so the coding style should be:
/* ... */
+extern rwlock_t dt_host_lock;
+
/**
* Find the interrupt controller
* For the moment we handle only one interrupt controller: the first
Cheers,
--
Julien Grall
