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

[Julien Grall <julien@xxxxxxx>]

On 15/01/2021 12:38, Rahul Singh wrote:
> Hello Oleksandr,
>
> > On 12 Jan 2021, at 8:59 pm, Oleksandr <olekstysh@xxxxxxxxx> wrote:
> >
> >
> > 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.
> Sorry I misunderstood your previous comments. Yes SMMUv3 driver is performing 
> the sanity check for Stream Id before configuring the hardware in function 
> arm_smmu_sid_in_range().
> > >   
> > >
> > > > > +
> > > > > +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?
> Yes I also agree with you to avoid duplication we can move the come code to the 
> function arch_iommu_hwdom_init().
> I will submit the patch(not part of this series)  if everyone is ok to move the 
> common code to arch_iommu_hwdom_init().
+1 on a follow-up.

Cheers,

--
Julien Grall