Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

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@xxxxxxx>
> > 
> > ---
> > 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