| Message ID | 20230602004824.20731-10-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
On 02/06/2023 02:48, 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. Would be nice to explain the need of putting a prototype into a private header. > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > > --- > Changes from v6: > Created a private header and moved iommu_dt_device_is_assigned() to header. > --- > xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++---- > xen/include/xen/iommu-private.h | 27 +++++++++++++++++++++++++++ > 2 files changed, 43 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..52e370db01 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,14 @@ fail: > return rc; > } > > -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev) I would write a comment that this function expects to be called with dtdevs_lock locked. > +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev) > { > bool_t assigned = 0; > > 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 +212,40 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, > if ( (d && d->is_dying) || domctl->u.assign_device.flags ) > break; > > + 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..5615decaff > --- /dev/null > +++ b/xen/include/xen/iommu-private.h > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + /* incorrect indentation (<< 1) > + * xen/iommu-private.h > + * > + * > + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved. > + * Written by Vikram Garhwal <vikram.garhwal@amd.com> I'm not sure if placing the copyright is appropriate, given a single prototype for a function not really implemented by you with just spinlocks removed. But I might be wrong. > + * > + */ > +#ifndef __XEN_IOMMU_PRIVATE_H__ > +#define __XEN_IOMMU_PRIVATE_H__ > + > +#ifdef CONFIG_HAS_DEVICE_TREE > +#include <xen/device_tree.h> > +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev); > +#endif > + > +#endif /* __XEN_IOMMU_PRIVATE_H__ */ empty line here please > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ ~Michal
On 02.06.2023 02:48, Vikram Garhwal wrote: > --- 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,14 @@ 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; > > 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 +212,40 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, > if ( (d && d->is_dying) || domctl->u.assign_device.flags ) > break; > > + 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; > I'm having trouble seeing how this patch can be correct: How do present callers of iommu_dt_device_is_assigned() continue to build? I can only guess that a new wrapper of this name is introduced in an earlier or later patch, but thus breaking bisectability (either here or, if the wrapper is added in an earlier patch, there). > --- /dev/null > +++ b/xen/include/xen/iommu-private.h I don't think private headers belong in this directory. From this patch alone (including its description) it also doesn't become clear how (and hence from where) this is intended to be used, which makes it hard to suggest an alternative. > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + /* Nit: Stray blank. > + * xen/iommu-private.h > + * > + * Nit: No double empty comment lines please. > + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved. > + * Written by Vikram Garhwal <vikram.garhwal@amd.com> > + * > + */ > +#ifndef __XEN_IOMMU_PRIVATE_H__ > +#define __XEN_IOMMU_PRIVATE_H__ > + > +#ifdef CONFIG_HAS_DEVICE_TREE > +#include <xen/device_tree.h> Why (for both of the lines)? All you need is a forward declaration of struct dt_device_node. > +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev); Just bool please. Jan
On 02.06.2023 11:19, Jan Beulich wrote: > On 02.06.2023 02:48, Vikram Garhwal wrote: >> --- 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,14 @@ 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; >> >> 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 +212,40 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, >> if ( (d && d->is_dying) || domctl->u.assign_device.flags ) >> break; >> >> + 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; >> > > I'm having trouble seeing how this patch can be correct: How do present > callers of iommu_dt_device_is_assigned() continue to build? I can only > guess that a new wrapper of this name is introduced in an earlier or > later patch, but thus breaking bisectability (either here or, if the > wrapper is added in an earlier patch, there). Oh, I'm sorry - looks like I overlooked that the 2nd hunk alters the (sole) caller. Somehow I was under the (wrong) impression that both hunks fiddle with the same function. Jan
Hi, On 02/06/2023 01:48, 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. Can you explain if you are trying to resolve an existing bug, or this is something that will be necessary in a follow-up patch? > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > > --- > Changes from v6: > Created a private header and moved iommu_dt_device_is_assigned() to header. > --- > xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++---- > xen/include/xen/iommu-private.h | 27 +++++++++++++++++++++++++++ > 2 files changed, 43 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..52e370db01 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,14 @@ 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) > { Please add an ASSERT() checking the lock is taken. > bool_t assigned = 0; > > 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 +212,40 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, > if ( (d && d->is_dying) || domctl->u.assign_device.flags ) > break; > > + spin_lock(&dtdevs_lock); > + 'dtdevs_lock' was intended to protect modification related to any IOMMU change. But here... > ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path, > domctl->u.assign_device.u.dt.size, > &dev); ... you also include "dt_find_node_by_gpath". Can you explain why and add a comment on top of 'dtdevs_lock' to explain what it is intended use? > 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..5615decaff > --- /dev/null > +++ b/xen/include/xen/iommu-private.h > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + /* > + * xen/iommu-private.h > + * > + * > + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved. > + * Written by Vikram Garhwal <vikram.garhwal@amd.com> > + * > + */ > +#ifndef __XEN_IOMMU_PRIVATE_H__ > +#define __XEN_IOMMU_PRIVATE_H__ > + > +#ifdef CONFIG_HAS_DEVICE_TREE > +#include <xen/device_tree.h> > +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: > + */
Hi, On 02/06/2023 08:45, Michal Orzel wrote: > > On 02/06/2023 02:48, Vikram Garhwal wrote: >> diff --git a/xen/include/xen/iommu-private.h b/xen/include/xen/iommu-private.h >> new file mode 100644 >> index 0000000000..5615decaff >> --- /dev/null >> +++ b/xen/include/xen/iommu-private.h >> @@ -0,0 +1,27 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> + /* > incorrect indentation (<< 1) > >> + * xen/iommu-private.h >> + * >> + * >> + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved. >> + * Written by Vikram Garhwal <vikram.garhwal@amd.com> > I'm not sure if placing the copyright is appropriate, given a single prototype for a function > not really implemented by you with just spinlocks removed. But I might be wrong. I agree. If you want to add a copyright then it should be the one from the original author. But in this case, I don't think we should add an copyright as there is nothing really interesting in the header. Cheers,
Hi, On 6/5/23 12:22 PM, Julien Grall wrote: > Hi, > > On 02/06/2023 08:45, Michal Orzel wrote: >> >> On 02/06/2023 02:48, Vikram Garhwal wrote: >>> diff --git a/xen/include/xen/iommu-private.h >>> b/xen/include/xen/iommu-private.h >>> new file mode 100644 >>> index 0000000000..5615decaff >>> --- /dev/null >>> +++ b/xen/include/xen/iommu-private.h >>> @@ -0,0 +1,27 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> + /* >> incorrect indentation (<< 1) >> >>> + * xen/iommu-private.h >>> + * >>> + * >>> + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights >>> Reserved. >>> + * Written by Vikram Garhwal <vikram.garhwal@amd.com> >> I'm not sure if placing the copyright is appropriate, given a single >> prototype for a function >> not really implemented by you with just spinlocks removed. But I >> might be wrong. > > I agree. If you want to add a copyright then it should be the one from > the original author. > > But in this case, I don't think we should add an copyright as there is > nothing really interesting in the header. Thanks for pointing this out and explaining the process. Will remove in next version. > > Cheers, >
On Mon, Jun 05, 2023 at 08:19:38PM +0100, Julien Grall wrote: > Hi, > > On 02/06/2023 01:48, 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. > > Can you explain if you are trying to resolve an existing bug, or this is > something that will be necessary in a follow-up patch? Updated for v8. > > > > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > > > > --- > > Changes from v6: > > Created a private header and moved iommu_dt_device_is_assigned() to header. > > --- > > xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++---- > > xen/include/xen/iommu-private.h | 27 +++++++++++++++++++++++++++ > > 2 files changed, 43 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..52e370db01 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,14 @@ 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) > > { > > Please add an ASSERT() checking the lock is taken. > Done in v8. > > bool_t assigned = 0; > > 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 +212,40 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, > > if ( (d && d->is_dying) || domctl->u.assign_device.flags ) > > break; > > + spin_lock(&dtdevs_lock); > > + > > 'dtdevs_lock' was intended to protect modification related to any IOMMU > change. But here... > > > ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path, > > domctl->u.assign_device.u.dt.size, > > &dev); > > ... you also include "dt_find_node_by_gpath". Can you explain why and add a > comment on top of 'dtdevs_lock' to explain what it is intended use? I have added a comment in v8. There was a comment in v3: "ensure that the "dev" doesn't disappear between the time we look it up". So, i moved the lock here and for dt_host the lock is added in follow-up patch: "common/device_tree: Add rwlock for dt_host". So, this all will happen with dtdevs_lock and dt_host_lock. > > > 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..5615decaff > > --- /dev/null > > +++ b/xen/include/xen/iommu-private.h > > @@ -0,0 +1,27 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > + /* > > + * xen/iommu-private.h > > + * > > + * > > + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved. > > + * Written by Vikram Garhwal <vikram.garhwal@amd.com> > > + * > > + */ > > +#ifndef __XEN_IOMMU_PRIVATE_H__ > > +#define __XEN_IOMMU_PRIVATE_H__ > > + > > +#ifdef CONFIG_HAS_DEVICE_TREE > > +#include <xen/device_tree.h> > > +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: > > + */ > > -- > Julien Grall
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index 1c32d7b50c..52e370db01 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,14 @@ 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; 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 +212,40 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, if ( (d && d->is_dying) || domctl->u.assign_device.flags ) break; + 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..5615decaff --- /dev/null +++ b/xen/include/xen/iommu-private.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + /* + * xen/iommu-private.h + * + * + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved. + * Written by Vikram Garhwal <vikram.garhwal@amd.com> + * + */ +#ifndef __XEN_IOMMU_PRIVATE_H__ +#define __XEN_IOMMU_PRIVATE_H__ + +#ifdef CONFIG_HAS_DEVICE_TREE +#include <xen/device_tree.h> +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. Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> --- Changes from v6: Created a private header and moved iommu_dt_device_is_assigned() to header. --- xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++---- xen/include/xen/iommu-private.h | 27 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 xen/include/xen/iommu-private.h