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

[Wei Chen <Wei.Chen@arm.com>]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月6日 1:58
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xen.org;
> Steve Capper <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> <Julien.Grall@arm.com>; nd <nd@arm.com>
> 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@kernel.org]
> > > Sent: 2017年7月4日 5:58
> > > To: Wei Chen <Wei.Chen@arm.com>
> > > Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> > > <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> > > <Julien.Grall@arm.com>; nd <nd@arm.com>
> > > 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@arm.com>
> > > > ---
> > > >  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@lists.xen.org
> > > > https://lists.xen.org/xen-devel
> > > >
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel