| Message ID | 20230819002850.32349-15-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
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.garhwal@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,
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 <vikram.garhwal@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. 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; > > 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. > Changed all above coding styles. > > 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. > Yes, dtdevs_lock will be held to avoid concurrent calls to SMMU remove. > 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. Updated. > > 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
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 <vikram.garhwal@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. > 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) > > > { > > > 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. > > > Changed all above coding styles. > > > 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. > > > Yes, dtdevs_lock will be held to avoid concurrent calls to SMMU remove. > > 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. > Updated. > > > > 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
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 <vikram.garhwal@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. >> 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,
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 <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. + */ +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 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 <vikram.garhwal@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(+)