Re: [XEN][PATCH v11 14/20] common/device_tree: Add rwlock for dt_host

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

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 <vikram.garhwal@xxxxxxx>
> > Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> > ---
> > 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