diff mbox series

[XEN,v9,09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

Message ID 20230819002850.32349-10-vikram.garhwal@amd.com (mailing list archive)
State Superseded
Headers show
Series dynamic node programming using overlay dtbo | expand

Commit Message

Vikram Garhwal Aug. 19, 2023, 12:28 a.m. UTC 
Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
Remove static type so this can also be used by SMMU drivers to check if the
device is being used before removing.

Moving spin_lock to caller was done to prevent the concurrent access to
iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
patches in this series introduces node add/remove feature.

Also, caller is required hold the correct lock so moved the function prototype
to a private header.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

---
Changes from v7:
    Update commit message.
    Add ASSERT().
---
---
 xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
 xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)
 create mode 100644 xen/include/xen/iommu-private.h

Comments

Jan Beulich Aug. 21, 2023, 6:53 a.m. UTC | #1 
On 19.08.2023 02:28, Vikram Garhwal wrote:
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> Remove static type so this can also be used by SMMU drivers to check if the
> device is being used before removing.
> 
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> patches in this series introduces node add/remove feature.
> 
> Also, caller is required hold the correct lock so moved the function prototype
> to a private header.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> ---
> Changes from v7:
>     Update commit message.
>     Add ASSERT().
> ---
> ---
>  xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
>  xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 4 deletions(-)
>  create mode 100644 xen/include/xen/iommu-private.h

I find it odd for new versions to be sent when discussion on the prior
version hasn't settled yet, and when you then also don't incorporate
the feedback given. I also find it odd to now be Cc-ed on the entire
series. Imo instead of that patches which aren't self-explanatory /
self-consistent simply need their descriptions improved (in the case
here to mention what further use of a function is intended). Yet
better would be to add declarations (and remove static) only at the
point that's actually needed. This then eliminates the risk of
subsequent changes in response to feedback (like Julien's here)
resulting in the declaration no longer being needed, thus leading to
a Misra violation (besides the general tidiness aspect).

Jan
Vikram Garhwal Aug. 21, 2023, 7:41 p.m. UTC | #2 
Hi Jen,
On Mon, Aug 21, 2023 at 08:53:38AM +0200, Jan Beulich wrote:
> On 19.08.2023 02:28, Vikram Garhwal wrote:
> > Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> > Remove static type so this can also be used by SMMU drivers to check if the
> > device is being used before removing.
> > 
> > Moving spin_lock to caller was done to prevent the concurrent access to
> > iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> > patches in this series introduces node add/remove feature.
> > 
> > Also, caller is required hold the correct lock so moved the function prototype
> > to a private header.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > 
> > ---
> > Changes from v7:
> >     Update commit message.
> >     Add ASSERT().
> > ---
> > ---
> >  xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
> >  xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 4 deletions(-)
> >  create mode 100644 xen/include/xen/iommu-private.h
> 
> I find it odd for new versions to be sent when discussion on the prior
> version hasn't settled yet, and when you then also don't incorporate
> the feedback given. I also find it odd to now be Cc-ed on the entire
> series. Imo instead of that patches which aren't self-explanatory /
> self-consistent simply need their descriptions improved (in the case
> here to mention what further use of a function is intended). Yet
> better would be to add declarations (and remove static) only at the
> point that's actually needed. This then eliminates the risk of
> subsequent changes in response to feedback (like Julien's here)
> resulting in the declaration no longer being needed, thus leading to
> a Misra violation (besides the general tidiness aspect).
> 
Reason behind sending new version: Patch 15/19 is largest patch in terms of change
and there was a long discussion with Julien and Stefano regarding a big change.
Last week(after v8) we agreed on change and Stefano and Julien were okay to send
v9 so it will be easier to review with that change.

Regarding cc-ing you to whole series, there was following comment on v8 09/1
"I don't think private headers should live in include/xen/. Judging from only
the patches I was Cc-ed on." I thought you wanted to see the whole full series.
Looks like i misunderstood the comment and Will remove the from cc-list in next
version.

The function itself will be needed in further patches in this series.
I am okay with keeping static and other things same here to avoid MISRA violation
and change wherever they are called first time.

Regarding Julien's comment i am reply back in same thread on why this was
not taken in account.
> Jan
Julien Grall Aug. 22, 2023, 7:43 p.m. UTC | #3 
Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> Remove static type so this can also be used by SMMU drivers to check if the
> device is being used before removing.

I have commented on v8. But I will comment here to make easier to keep 
track of comment.

I don't think iommu_dt_device_is_assigned_locked() should be called from 
the SMMU. If you want to check a device is assigned then it would be 
best to use the internal state of the SMMU.

This has two benefits:
   * This avoids what I view as a layer of violation
   * This confirmed that the internal state match with what we expect

> 
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> patches in this series introduces node add/remove feature.
> 
> Also, caller is required hold the correct lock so moved the function prototype
> to a private header.

I don't understand how requiring the caller to hold the correct lock 
means you need to move the protype in a private header. Can you clarify?

> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> ---
> Changes from v7:
>      Update commit message.
>      Add ASSERT().
> ---
> ---
>   xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
>   xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
>   2 files changed, 50 insertions(+), 4 deletions(-)
>   create mode 100644 xen/include/xen/iommu-private.h
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 1c32d7b50c..5796ee1f93 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -18,6 +18,7 @@
>   #include <xen/device_tree.h>
>   #include <xen/guest_access.h>
>   #include <xen/iommu.h>
> +#include <xen/iommu-private.h>
>   #include <xen/lib.h>
>   #include <xen/sched.h>
>   #include <xsm/xsm.h>
> @@ -83,16 +84,16 @@ fail:
>       return rc;
>   }
>   
> -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
>   {
>       bool_t assigned = 0;
>   
> +    ASSERT(spin_is_locked(&dtdevs_lock));
> +
>       if ( !dt_device_is_protected(dev) )
>           return 0;
>   
> -    spin_lock(&dtdevs_lock);
>       assigned = !list_empty(&dev->domain_list);
> -    spin_unlock(&dtdevs_lock);
>   
>       return assigned;
>   }
> @@ -213,27 +214,44 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>           if ( (d && d->is_dying) || domctl->u.assign_device.flags )
>               break;
>   
> +        /*
> +         * To ensure that the dev doesn't disappear between the time we look it
> +         * up with dt_find_node_by_gpath() and we check the assignment later.
> +         */
> +        spin_lock(&dtdevs_lock);

This change doesn't look to be explained in the commit message. But 
looking at the code after this series is applied, you justified the 
addition of read_lock(&dt_host_lock) to protect access to the 
device-tree. This will be held longer than dtdevs_lock. So is it 
actually necessary to move the locking earlier?

> +
>           ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>                                       domctl->u.assign_device.u.dt.size,
>                                       &dev);
>           if ( ret )
> +        {
> +            spin_unlock(&dtdevs_lock);
>               break;
> +        }
>   
>           ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
>           if ( ret )
> +        {
> +            spin_unlock(&dtdevs_lock);
>               break;
> +        }
>   
>           if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
>           {
> -            if ( iommu_dt_device_is_assigned(dev) )
> +
> +            if ( iommu_dt_device_is_assigned_locked(dev) )
>               {
>                   printk(XENLOG_G_ERR "%s already assigned.\n",
>                          dt_node_full_name(dev));
>                   ret = -EINVAL;
>               }
> +
> +            spin_unlock(&dtdevs_lock);
>               break;
>           }
>   
> +        spin_unlock(&dtdevs_lock);
> +
>           if ( d == dom_io )
>               return -EINVAL;
>   
> diff --git a/xen/include/xen/iommu-private.h b/xen/include/xen/iommu-private.h
> new file mode 100644
> index 0000000000..bb5c94e7a6
> --- /dev/null
> +++ b/xen/include/xen/iommu-private.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * xen/iommu-private.h
> + */
> +#ifndef __XEN_IOMMU_PRIVATE_H__
> +#define __XEN_IOMMU_PRIVATE_H__
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +#include <xen/device_tree.h>
> +
> +/*
> + * Checks if dt_device_node is assigned to a domain or not. This function
> + * expects to be called with dtdevs_lock acquired by caller.
> + */
> +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);
> +#endif
> +
> +#endif /* __XEN_IOMMU_PRIVATE_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Cheers,
Vikram Garhwal Aug. 25, 2023, 5:09 a.m. UTC | #4 
Hi Julien,
On Tue, Aug 22, 2023 at 08:43:27PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> > Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> > Remove static type so this can also be used by SMMU drivers to check if the
> > device is being used before removing.
> 
> I have commented on v8. But I will comment here to make easier to keep track
> of comment.
> 
> I don't think iommu_dt_device_is_assigned_locked() should be called from the
> SMMU. If you want to check a device is assigned then it would be best to use
> the internal state of the SMMU.
> 
> This has two benefits:
>   * This avoids what I view as a layer of violation
>   * This confirmed that the internal state match with what we expect
> 
> > 
> > Moving spin_lock to caller was done to prevent the concurrent access to
> > iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> > patches in this series introduces node add/remove feature.
> > 
> > Also, caller is required hold the correct lock so moved the function prototype
> > to a private header.
> 
> I don't understand how requiring the caller to hold the correct lock means
> you need to move the protype in a private header. Can you clarify?
> 
With removal of check in smmu.c, this header is no longer required.
will remove private header too.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > 
> > ---
> > Changes from v7:
> >      Update commit message.
> >      Add ASSERT().
> > ---
> > ---
> >   xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
> >   xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
> >   2 files changed, 50 insertions(+), 4 deletions(-)
> >   create mode 100644 xen/include/xen/iommu-private.h
> > 
> > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> > index 1c32d7b50c..5796ee1f93 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -18,6 +18,7 @@
> >   #include <xen/device_tree.h>
> >   #include <xen/guest_access.h>
> >   #include <xen/iommu.h>
> > +#include <xen/iommu-private.h>
> >   #include <xen/lib.h>
> >   #include <xen/sched.h>
> >   #include <xsm/xsm.h>
> > @@ -83,16 +84,16 @@ fail:
> >       return rc;
> >   }
> > -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> > +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
> >   {
> >       bool_t assigned = 0;
> > +    ASSERT(spin_is_locked(&dtdevs_lock));
> > +
> >       if ( !dt_device_is_protected(dev) )
> >           return 0;
> > -    spin_lock(&dtdevs_lock);
> >       assigned = !list_empty(&dev->domain_list);
> > -    spin_unlock(&dtdevs_lock);
> >       return assigned;
> >   }
> > @@ -213,27 +214,44 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> >           if ( (d && d->is_dying) || domctl->u.assign_device.flags )
> >               break;
> > +        /*
> > +         * To ensure that the dev doesn't disappear between the time we look it
> > +         * up with dt_find_node_by_gpath() and we check the assignment later.
> > +         */
> > +        spin_lock(&dtdevs_lock);
> 
> This change doesn't look to be explained in the commit message. But looking
> at the code after this series is applied, you justified the addition of
> read_lock(&dt_host_lock) to protect access to the device-tree. This will be
> held longer than dtdevs_lock. So is it actually necessary to move the
> locking earlier?
I re-looked at the implementation and actually, dt_host_lock will protect the
dt related changes. I will move it down before iommu_dt_device_is_assigned_lock()
call.
> 
> > +
> >           ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
> >                                       domctl->u.assign_device.u.dt.size,
> >                                       &dev);
> >           if ( ret )
> > +        {
> > +            spin_unlock(&dtdevs_lock);
> >               break;
> > +        }
> >           ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
> >           if ( ret )
> > +        {
> > +            spin_unlock(&dtdevs_lock);
> >               break;
> > +        }
> >           if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
> >           {
> > -            if ( iommu_dt_device_is_assigned(dev) )
> > +
> > +            if ( iommu_dt_device_is_assigned_locked(dev) )
> >               {
> >                   printk(XENLOG_G_ERR "%s already assigned.\n",
> >                          dt_node_full_name(dev));
> >                   ret = -EINVAL;
> >               }
> > +
> > +            spin_unlock(&dtdevs_lock);
> >               break;
> >           }
> > +        spin_unlock(&dtdevs_lock);
> > +
> >           if ( d == dom_io )
> >               return -EINVAL;
> > diff --git a/xen/include/xen/iommu-private.h b/xen/include/xen/iommu-private.h
> > new file mode 100644
> > index 0000000000..bb5c94e7a6
> > --- /dev/null
> > +++ b/xen/include/xen/iommu-private.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * xen/iommu-private.h
> > + */
> > +#ifndef __XEN_IOMMU_PRIVATE_H__
> > +#define __XEN_IOMMU_PRIVATE_H__
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +#include <xen/device_tree.h>
> > +
> > +/*
> > + * Checks if dt_device_node is assigned to a domain or not. This function
> > + * expects to be called with dtdevs_lock acquired by caller.
> > + */
> > +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);
> > +#endif
> > +
> > +#endif /* __XEN_IOMMU_PRIVATE_H__ */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> 
> Cheers,
> 
> -- 
> Julien Grall
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 1c32d7b50c..5796ee1f93 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -18,6 +18,7 @@ 
 #include <xen/device_tree.h>
 #include <xen/guest_access.h>
 #include <xen/iommu.h>
+#include <xen/iommu-private.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xsm/xsm.h>
@@ -83,16 +84,16 @@  fail:
     return rc;
 }
 
-static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
+bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
 {
     bool_t assigned = 0;
 
+    ASSERT(spin_is_locked(&dtdevs_lock));
+
     if ( !dt_device_is_protected(dev) )
         return 0;
 
-    spin_lock(&dtdevs_lock);
     assigned = !list_empty(&dev->domain_list);
-    spin_unlock(&dtdevs_lock);
 
     return assigned;
 }
@@ -213,27 +214,44 @@  int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
         if ( (d && d->is_dying) || domctl->u.assign_device.flags )
             break;
 
+        /*
+         * To ensure that the dev doesn't disappear between the time we look it
+         * up with dt_find_node_by_gpath() and we check the assignment later.
+         */
+        spin_lock(&dtdevs_lock);
+
         ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
                                     domctl->u.assign_device.u.dt.size,
                                     &dev);
         if ( ret )
+        {
+            spin_unlock(&dtdevs_lock);
             break;
+        }
 
         ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
         if ( ret )
+        {
+            spin_unlock(&dtdevs_lock);
             break;
+        }
 
         if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
         {
-            if ( iommu_dt_device_is_assigned(dev) )
+
+            if ( iommu_dt_device_is_assigned_locked(dev) )
             {
                 printk(XENLOG_G_ERR "%s already assigned.\n",
                        dt_node_full_name(dev));
                 ret = -EINVAL;
             }
+
+            spin_unlock(&dtdevs_lock);
             break;
         }
 
+        spin_unlock(&dtdevs_lock);
+
         if ( d == dom_io )
             return -EINVAL;
 
diff --git a/xen/include/xen/iommu-private.h b/xen/include/xen/iommu-private.h
new file mode 100644
index 0000000000..bb5c94e7a6
--- /dev/null
+++ b/xen/include/xen/iommu-private.h
@@ -0,0 +1,28 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/iommu-private.h
+ */
+#ifndef __XEN_IOMMU_PRIVATE_H__
+#define __XEN_IOMMU_PRIVATE_H__
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+#include <xen/device_tree.h>
+
+/*
+ * Checks if dt_device_node is assigned to a domain or not. This function
+ * expects to be called with dtdevs_lock acquired by caller.
+ */
+bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);
+#endif
+
+#endif /* __XEN_IOMMU_PRIVATE_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */