Re: [XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxxx/1636441347-133850-7-git-send-email-fnu.vikram@xxxxxxxxxx/
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