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.garh...@amd.com> > > Reviewed-by: Michal Orzel <michal.or...@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!
