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

[Wei Chen <Wei.Chen@xxxxxxx>]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> Sent: 2017年7月6日 1:58
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxx;
> 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 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."
> 

Ok, I will do it in next version.

> 
> > >
> > > > +        */
> > > > +       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