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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

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().

Regards,
Rahul
 
> 
> -- 
> Regards,
> 
> Oleksandr Tyshchenko
>