| Message ID | 20230817003947.3849-10-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
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 <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 ... 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
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 <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 > > ... 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
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 <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 >> >> ... 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,
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 <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 >> >> ... 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
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 <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 > > > > > > ... 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.vikram@xilinx.com/1636441347-133850-7-git-send-email-fnu.vikram@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
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 <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 >>>> >>>> ... 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.vikram@xilinx.com/1636441347-133850-7-git-send-email-fnu.vikram@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,
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: + */
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