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.garh...@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
