Re: [XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
Hi, On 21/08/2023 20:46, Vikram Garhwal wrote: Hi Julien On Fri, Aug 18, 2023 at 09:35:02PM +0100, Julien Grall wrote: Hi Vikram, On 18/08/2023 20:52, Vikram Garhwal wrote: Hi Jan On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote: On 17.08.2023 02:39, Vikram Garhwal wrote: --- /dev/null +++ b/xen/include/xen/iommu-private.h I don't think private headers should live in include/xen/. Judging from only the patches I was Cc-ed on, ... Thank you for suggestion. Do you where can i place it then? Please see another comment down regarding who might be using this function. @@ -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 + +/* + * 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 ... I don't even see the need for the declaration, as the function is used only from the file also defining it. But of course if there is a use elsewhere (in Arm-only code, as is suggested by the description here), then the header (under a suitable name) wants to live under drivers/passthrough/ (and of course be included only from anywhere in that sub-tree). This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is added in 12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops). AFAICT, the caller of this function (iommu_remove_dt_device()) will already check if the device was assigned and bail out if that's the case. So why do we need to check it again in the SMMU driver? This was comment from you in v2: "Even if the IOMMU subsystem check it, it would be good that the SMMU driver also check the device is not currently used before removing it. If it is, then we should return -EBUSY." Link:https://patchew.org/Xen/1636441347-133850-1-git-send-email-fnu.vik...@xilinx.com/1636441347-133850-7-git-send-email-fnu.vik...@xilinx.com/ A lot of water flown under the bridge since that discussion. If you want to check the device is not attached in the SMMU driver, then you should use the internal SMMU state rather than the generic state. You can look at arm_smmu_attach_dev() for an example how to do it. Cheers, -- Julien Grall
Re: [XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
Hi Julien On Fri, Aug 18, 2023 at 09:35:02PM +0100, Julien Grall wrote: > Hi Vikram, > > On 18/08/2023 20:52, Vikram Garhwal wrote: > > Hi Jan > > On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote: > > > On 17.08.2023 02:39, Vikram Garhwal wrote: > > > > --- /dev/null > > > > +++ b/xen/include/xen/iommu-private.h > > > > > > I don't think private headers should live in include/xen/. Judging from > > > only > > > the patches I was Cc-ed on, ... > > Thank you for suggestion. Do you where can i place it then? > > Please see another comment down regarding who might be using this function. > > > > > > > @@ -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 > > > > + > > > > +/* > > > > + * 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 > > > > > > ... I don't even see the need for the declaration, as the function is used > > > only from the file also defining it. But of course if there is a use > > > elsewhere (in Arm-only code, as is suggested by the description here), > > > then > > > the header (under a suitable name) wants to live under > > > drivers/passthrough/ > > > (and of course be included only from anywhere in that sub-tree). > > > > > This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is > > added in > > 12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops). > > AFAICT, the caller of this function (iommu_remove_dt_device()) will already > check if the device was assigned and bail out if that's the case. > > > So why do we need to check it again in the SMMU driver? > This was comment from you in v2: "Even if the IOMMU subsystem check it, it would be good that the SMMU driver also check the device is not currently used before removing it. If it is, then we should return -EBUSY." Link:https://patchew.org/Xen/1636441347-133850-1-git-send-email-fnu.vik...@xilinx.com/1636441347-133850-7-git-send-email-fnu.vik...@xilinx.com/ And there was similar comment from Michal in v5. That's why this was kept. Regards, Vikram > And if you really need to then you most likely want to check the internal > state of the SMMU driver rather than the generic state. > > Cheers, > > -- > Julien Grall
Re: [XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
On 18.08.2023 21:52, Vikram Garhwal wrote: > On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote: >> On 17.08.2023 02:39, Vikram Garhwal wrote: >>> --- /dev/null >>> +++ b/xen/include/xen/iommu-private.h >> >> I don't think private headers should live in include/xen/. Judging from only >> the patches I was Cc-ed on, ... > Thank you for suggestion. Do you where can i place it then? Irrespective of Julien's reply (potentially rendering this moot), see ... > Please see another comment down regarding who might be using this function. >> >>> @@ -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 >>> + >>> +/* >>> + * 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 >> >> ... I don't even see the need for the declaration, as the function is used >> only from the file also defining it. But of course if there is a use >> elsewhere (in Arm-only code, as is suggested by the description here), then >> the header (under a suitable name) wants to live under drivers/passthrough/ ... my earlier reply. Jan >> (and of course be included only from anywhere in that sub-tree). >> > This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is added > in > 12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops). > > I will make sure to cc you for all the patches in v9 series. I plan to send > it today. > >> Jan
Re: [XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
Hi Vikram, On 18/08/2023 20:52, Vikram Garhwal wrote: Hi Jan On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote: On 17.08.2023 02:39, Vikram Garhwal wrote: --- /dev/null +++ b/xen/include/xen/iommu-private.h I don't think private headers should live in include/xen/. Judging from only the patches I was Cc-ed on, ... Thank you for suggestion. Do you where can i place it then? Please see another comment down regarding who might be using this function. @@ -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 + +/* + * 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 ... I don't even see the need for the declaration, as the function is used only from the file also defining it. But of course if there is a use elsewhere (in Arm-only code, as is suggested by the description here), then the header (under a suitable name) wants to live under drivers/passthrough/ (and of course be included only from anywhere in that sub-tree). This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is added in 12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops). AFAICT, the caller of this function (iommu_remove_dt_device()) will already check if the device was assigned and bail out if that's the case. So why do we need to check it again in the SMMU driver? And if you really need to then you most likely want to check the internal state of the SMMU driver rather than the generic state. Cheers, -- Julien Grall
Re: [XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
Hi Jan On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote: > On 17.08.2023 02:39, Vikram Garhwal wrote: > > --- /dev/null > > +++ b/xen/include/xen/iommu-private.h > > I don't think private headers should live in include/xen/. Judging from only > the patches I was Cc-ed on, ... Thank you for suggestion. Do you where can i place it then? Please see another comment down regarding who might be using this function. > > > @@ -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 > > + > > +/* > > + * 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 > > ... I don't even see the need for the declaration, as the function is used > only from the file also defining it. But of course if there is a use > elsewhere (in Arm-only code, as is suggested by the description here), then > the header (under a suitable name) wants to live under drivers/passthrough/ > (and of course be included only from anywhere in that sub-tree). > This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is added in 12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops). I will make sure to cc you for all the patches in v9 series. I plan to send it today. > Jan
Re: [XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
On 17.08.2023 02:39, Vikram Garhwal wrote: > --- /dev/null > +++ b/xen/include/xen/iommu-private.h I don't think private headers should live in include/xen/. Judging from only the patches I was Cc-ed on, ... > @@ -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 > + > +/* > + * 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 ... I don't even see the need for the declaration, as the function is used only from the file also defining it. But of course if there is a use elsewhere (in Arm-only code, as is suggested by the description here), then the header (under a suitable name) wants to live under drivers/passthrough/ (and of course be included only from anywhere in that sub-tree). Jan
[XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
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
---
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
#include
#include
+#include
#include
#include
#include
@@ -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(_lock));
+
if ( !dt_device_is_protected(dev) )
return 0;
-spin_lock(_lock);
assigned = !list_empty(>domain_list);
-spin_unlock(_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(_lock);
+
ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
domctl->u.assign_device.u.dt.size,
);
if ( ret )
+{
+spin_unlock(_lock);
break;
+}
ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
if ( ret )
+{
+spin_unlock(_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(_lock);
break;
}
+spin_unlock(_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 00..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
+
+/*
+ * 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:
+ */
--
2.17.1
