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 <[email protected]>
> > Reviewed-by: Michal Orzel <[email protected]>
> > ---
> > 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
