Re: [Xen-devel] [PATCH 1/7] xen/arm: SMMU: Implement the add_device callback in SMMU

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 4 Jul 2017, Wei Chen wrote:
> Hi Stefano,
> 
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> > Sent: 2017年7月4日 5:58
> > 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 1/7] xen/arm: SMMU: Implement the add_device
> > callback in SMMU
> > 
> > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > This add_device callback function is taking care of adding a device
> > > to SMMU and make sure it is fully prepare to be used by the SMMU
> > > afterwards.
> > >
> > > In previous code, we don't implement the add_device callback in
> > > iommu_ops for ARM SMMU. We placed the work of add_device to
> > > assign_device callback. The function assign_device should not care
> > > about adding the device to an iommu_group. It might not even be
> > > able to decide how to do that. In this patch, we move this work
> > > back to add_device callback.
> > >
> > > This add_device callback is only called while we are handling all
> > > devices for constructing the Domain0.
> > >
> > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > ---
> > >  xen/drivers/passthrough/arm/smmu.c | 34 
> > > +++++++++++++++++++++++-----------
> > >  1 file changed, 23 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > b/xen/drivers/passthrough/arm/smmu.c
> > > index 74c09b0..2efa52d 100644
> > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > @@ -2591,6 +2591,26 @@ static void arm_smmu_destroy_iommu_domain(struct
> > iommu_domain *domain)
> > >   xfree(domain);
> > >  }
> > >
> > > +static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> > > +{
> > > + if (dt_device_is_protected(dev->of_node)) {
> > > +         if (!dev->archdata.iommu) {
> > > +                 dev->archdata.iommu = xzalloc(struct 
> > > arm_smmu_xen_device);
> > > +                 if (!dev->archdata.iommu)
> > > +                         return -ENOMEM;
> > > +         }
> > > +
> > > +         if (!dev_iommu_group(dev))
> > > +                 return arm_smmu_add_device(dev);
> > > + }
> > > +
> > > + /*
> > > +  * Return 0 if the device is not protected to follow the behavior
> > > +  * of PCI add device.
> > 
> > What does this comment mean?
> > 
> 
> While I was looking at the iommu_add_device which is used by PCI counterpart.
> I found it will always return 0 even for device that are not protected by an 
> IOMMU.
> I would much prefer to keep platform devices have similar behavior as PCI 
> devices.
> 
> So while I was implementing iommu_add_dt_device and arm_smmu_xen_add_device, I
> returned 0 if the device is not protected by IOMMU.

I understand now. Please rephrase to:

  "Return 0 (not an error) if the device is not protected by an SMMU, to
  match the behavior of iommu_add_device."


> > 
> > > +  */
> > > + return 0;
> > > +}
> > > +
> > >  static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
> > >                          struct device *dev, u32 flag)
> > >  {
> > > @@ -2600,17 +2620,8 @@ static int arm_smmu_assign_dev(struct domain *d, u8
> > devfn,
> > >
> > >   xen_domain = dom_iommu(d)->arch.priv;
> > >
> > > - if (!dev->archdata.iommu) {
> > > -         dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> > > -         if (!dev->archdata.iommu)
> > > -                 return -ENOMEM;
> > > - }
> > > -
> > > - if (!dev_iommu_group(dev)) {
> > > -         ret = arm_smmu_add_device(dev);
> > > -         if (ret)
> > > -                 return ret;
> > > - }
> > > + if (!dev_iommu_group(dev))
> > > +     return -ENODEV;
> > >
> > >   spin_lock(&xen_domain->lock);
> > >
> > > @@ -2784,6 +2795,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
> > >      .teardown = arm_smmu_iommu_domain_teardown,
> > >      .iotlb_flush = arm_smmu_iotlb_flush,
> > >      .iotlb_flush_all = arm_smmu_iotlb_flush_all,
> > > +    .add_device = arm_smmu_xen_add_device,
> > >      .assign_device = arm_smmu_assign_dev,
> > >      .reassign_device = arm_smmu_reassign_dev,
> > >      .map_page = arm_smmu_map_page,
> > > --
> > > 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