Re: [Xen-devel] [RFC 2/2] smmu: add support for generic DT bindings

[Julien Grall <julien@xxxxxxx>]

Hi Brian,

Thank you for the patch.

On 10/01/2020 01:26, Brian Woods wrote:
> Restructure some of the code and add supporting functions for adding
> generic device tree (DT) binding support.  The normal add_device and
> dt_xlate functions are wrappers of the legacy functions due to legacy
> calls needing more arguments because the find_smmu can't a smmu that
> isn't initialized.
>
> Signed-off-by: Brian Woods <brian.woods@xxxxxxxxxx>
> ---
> RFC especially on:
>     - Checks for the: arm_smmu_dt_add_device* and arm_smmu_dt_xlate*
>       functions.
>
>   xen/drivers/passthrough/arm/smmu.c    | 118 +++++++++++++++++++++++++---------
>   xen/drivers/passthrough/device_tree.c |  17 +----
>   2 files changed, 87 insertions(+), 48 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index c5db5be..08787cd 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -251,6 +251,8 @@ struct iommu_group
>         atomic_t ref;
>   };
>   
> +static const struct arm_smmu_device *find_smmu(const struct device *dev);
> +
>   static struct iommu_group *iommu_group_alloc(void)
>   {
>         struct iommu_group *group = xzalloc(struct iommu_group);
> @@ -775,64 +777,114 @@ static int insert_smmu_master(struct arm_smmu_device 
> *smmu,
>         return 0;
>   }
>   
> -static int register_smmu_master(struct arm_smmu_device *smmu,
> -                               struct device *dev,
> -                               struct of_phandle_args *masterspec)
> +/*
> + * Since smmu isn't done initializing before this is run in the legacy
> + * case, create a function where that's passed and then have the generic
> + * function just be a simple wrapper.
> + */
> +static int arm_smmu_dt_xlate_legacy(struct device *dev,
> +                                   const struct of_phandle_args *spec,
> +                                   struct iommu_fwspec *fwspec)
> +{
> +       if ((spec->args_count + fwspec->num_ids) > MAX_MASTER_STREAMIDS) {
> +               dev_err(dev,
> +                       "reached maximum number (%d) of stream IDs for master device 
> %s\n",
> +                       MAX_MASTER_STREAMIDS, spec->np->name);
> +               return -ENOSPC;
> +       }
> +
> +       /* adding the ids here */
> +       return iommu_fwspec_add_ids(dev,
> +                                   spec->args,
> +                                   spec->args_count);
> +}
> +
> +static int arm_smmu_dt_xlate(struct device *dev,
> +                            const struct dt_phandle_args *spec)
> +{
> +       return arm_smmu_dt_xlate_legacy(dev,
> +                                       spec,
> +                                       dev_iommu_fwspec_get(dev));
> +}
The legacy and generic bindings are fundamentally different.

In the case of the legacy binding, a specifier will contains multiple 
streamids. But for the generic bindings, the interpretation of the 
specifier will depend on the number of arguments.
If you want to specify multiple streamID, you would either have to use 
one specifier per streamID or use stream matching. You also have an 
additional property to take care of (see "stream-match-mask").
Please have a look at the bindings in Linux ([1], [2]) for more details. 
I would also recommend to have a look at the SMMU driver in Linux as 
well [3].
I would expect this to change the way the patch is structure. So I am 
not going to review the rest. Although, let me know if you want me to 
look at a particular bits.
Cheers,


[1] Documentation/devicetree/bindings/iommu/iommu.txt
[2] Documentation/devicetree/bindings/iommu/arm,smmu.txt
[3] drivers/iommu/arm-smmu.c

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel