Re: [Xen-devel] [PATCH 2/7] xen/arm: SMMU: Introduce a helper to add DT device to SMMU

[Wei Chen <Wei.Chen@xxxxxxx>]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> Sent: 2017年7月6日 2:02
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>;
> Julien Grall <Julien.Grall@xxxxxxx>; Steve Capper <Steve.Capper@xxxxxxx>; nd
> <nd@xxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH 2/7] xen/arm: SMMU: Introduce a helper to add
> DT device to SMMU
> 
> On Tue, 4 Jul 2017, Wei Chen wrote:
> > Hi Stefano,
> >
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> > > Sent: 2017年7月4日 6:03
> > > To: Wei Chen <Wei.Chen@xxxxxxx>
> > > Cc: xen-devel@xxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; Steve Capper
> > > <Steve.Capper@xxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>; Julien Grall
> > > <Julien.Grall@xxxxxxx>; nd <nd@xxxxxxx>
> > > Subject: Re: [Xen-devel] [PATCH 2/7] xen/arm: SMMU: Introduce a helper to
> add
> > > DT device to SMMU
> > >
> > > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > > In current code, we only have the iommu_add_device to add PCI device
> > > > to IOMMU. But for ARM SMMU, we don't have a separate helper to add
> > > > platform device with device tree to SMMU. This work was included in
> > > > the iommu_assign_dt_device. But sometimes, we just want to add device
> > > > to SMMU to do some preparation for further use. In this case, we can't
> > > > call iommu_assign_dt_device.
> > > >
> > > > In previous patch, we have implement the add_device callback for SMMU,
> > > > so we can separate this work from assign_device now.
> > > >
> > > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > > ---
> > > >  xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++++++
> > > >  xen/include/xen/iommu.h               |  1 +
> > > >  2 files changed, 21 insertions(+)
> > > >
> > > > diff --git a/xen/drivers/passthrough/device_tree.c
> > > b/xen/drivers/passthrough/device_tree.c
> > > > index 99ed49e..a8f403a 100644
> > > > --- a/xen/drivers/passthrough/device_tree.c
> > > > +++ b/xen/drivers/passthrough/device_tree.c
> > > > @@ -24,6 +24,26 @@
> > > >
> > > >  static spinlock_t dtdevs_lock = SPIN_LOCK_UNLOCKED;
> > > >
> > > > +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev)
> > > > +{
> > > > +    int rc;
> > > > +
> > > > +    struct domain_iommu *hd = dom_iommu(d);
> > > > +
> > > > +    if ( !iommu_enabled || !hd->platform_ops ||
> > > > +         !hd->platform_ops->add_device )
> > > > +        return 0;
> > >
> > > Shouldn't we also have:
> > >
> > >   if ( !dt_device_is_protected(dev) )
> > >         return 0;
> > >
> > > ?
> > >
> >
> > When we're using the legacy binding, the master IDs will be registered to
> SMMU and
> > the protected flag of relevant master devices will be set to true
> (dt_device_is_protected
> > will return true). But for generic IOMMU bindings, before we call ops-
> >add_device,
> > we didn't register the master device's master id to SMMU and hadn't set the
> protected
> > flag, The dt_device_is_protected will always return false.
> >
> > In this case, we can't call dt_device_is_protected(dev) here.
> 
> I get it. Please add an in-code comment here to explain the situation.
> 

Ok, I will do it.

> 
> > >
> > > > +    spin_lock(&dtdevs_lock);
> > > > +
> > > > +    /* The devfn field doesn't matter to DT device. */
> > > > +    rc = hd->platform_ops->add_device(0, dt_to_dev(dev));
> > > > +
> > > > +    spin_unlock(&dtdevs_lock);
> > > > +
> > > > +    return rc;
> > > > +}
> > > > +
> > > >  int iommu_assign_dt_device(struct domain *d, struct dt_device_node 
> > > > *dev)
> > > >  {
> > > >      int rc = -EBUSY;
> > > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > > > index 5803e3f..ec03faa 100644
> > > > --- a/xen/include/xen/iommu.h
> > > > +++ b/xen/include/xen/iommu.h
> > > > @@ -132,6 +132,7 @@ void iommu_read_msi_from_ire(struct msi_desc
> *msi_desc,
> > > struct msi_msg *msg);
> > > >  #ifdef CONFIG_HAS_DEVICE_TREE
> > > >  #include <xen/device_tree.h>
> > > >
> > > > +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev);
> > > >  int iommu_assign_dt_device(struct domain *d, struct dt_device_node
> *dev);
> > > >  int iommu_deassign_dt_device(struct domain *d, struct dt_device_node
> *dev);
> > > >  int iommu_dt_domain_init(struct domain *d);
> > > > --
> > > > 2.7.4
> > > >
> > > >
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@xxxxxxxxxxxxx
> > > > https://lists.xen.org/xen-devel
> > > >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > https://lists.xen.org/xen-devel
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel