Re: [PATCH v4 11/11] xen/arm: smmuv3: Add support for SMMUv3 driver

[Oleksandr <olekstysh@xxxxxxxxx>]

On 12.01.21 11:41, Rahul Singh wrote:

Hi Rahul


> > >   -static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args 
> > > *args)
> > > +static int arm_smmu_dt_xlate(struct device *dev,
> > > +                               const struct dt_phandle_args *args)
> > >   {
> > > -       return iommu_fwspec_add_ids(dev, args->args, 1);
> > > +       int ret;
> > > +       struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > Please bear in mind I am not familiar with the SMMU, but don't we need to 
> > perform a some kind
> > of sanity check of passed DT IOMMU specifier here?
> I checked the code follow we will never hit the dt_xlate without IOMMU 
> specifier but anyway I will add the sanity check.
By sanity check I meant to make sure that device ID (stream ID) is in 
allowed range (of course, if this is relevant for SMMU).
For example, for IPMMU-VMSA we have a check that device ID (uTLB) is 
less than max uTLB number.

>   
>
> > > +
> > > +static int arm_smmu_iommu_xen_domain_init(struct domain *d)
> > > +{
> > > +       struct arm_smmu_xen_domain *xen_domain;
> > > +
> > > +       xen_domain = xzalloc(struct arm_smmu_xen_domain);
> > > +       if (!xen_domain)
> > > +               return -ENOMEM;
> > > +
> > > +       spin_lock_init(&xen_domain->lock);
> > > +       INIT_LIST_HEAD(&xen_domain->contexts);
> > > +
> > > +       dom_iommu(d)->arch.priv = xen_domain;
> > > +       return 0;
> > > +
> > > +}
> > > +
> > > +static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
> > > +{
> > Both SMMUv2 and IPMMU perform some actions here. Any reason we don't need to do 
> > the same here?
> >
> >      /* Set to false options not supported on ARM. */
> >      if ( iommu_hwdom_inclusive )
> >          printk(XENLOG_WARNING
> >          "map-inclusive dom0-iommu option is not supported on ARM\n");
> >      iommu_hwdom_inclusive = false;
> >      if ( iommu_hwdom_reserved == 1 )
> >          printk(XENLOG_WARNING
> >          "map-reserved dom0-iommu option is not supported on ARM\n");
> >      iommu_hwdom_reserved = 0;
> >
> >      arch_iommu_hwdom_init(d);
> I will add the above code for SMMUv3 also.
Great.

I was thinking about it, this is the third IOMMU driver on Arm which has 
to disable the _same_ unsupported options, probably this code wants to 
be folded in arch_iommu_hwdom_init() to avoid duplication?
--
Regards,

Oleksandr Tyshchenko