Re: [RFC v2 2/2] arm,smmu: add support for generic DT bindings

[Julien Grall <julien@xxxxxxx>]

Hi Brian,

On 22/07/2020 05:00, Brian Woods wrote:
> Restructure some of the code and add supporting functions for adding
> generic device tree (DT) binding support.  
It feels to me you want to split the patch in two:
   1) Restructure the code
   2) Add support for DT bindings

> This will allow for using
> current Linux device trees with just modifying the chosen field to
> enable Xen.
So what happen if the legacy binding and generic bindings co-exist. 
Which one will be used?
> Signed-off-by: Brian Woods <brian.woods@xxxxxxxxxx>
> ---
>
> Just realized that I'm fairly sure I need to do some work on the SMRs.
> Other than that though, I think things should be okayish.
The SMMU code in Xen is pretty awful (I know I adapted it for Xen). It 
would be hard to make it worse :).
> v1 -> v2
>      - Corrected how reading of DT is done with generic bindings
>
>
>   xen/drivers/passthrough/arm/smmu.c    | 102 +++++++++++++++++++++++++---------
>   xen/drivers/passthrough/device_tree.c |  17 +-----
>   2 files changed, 78 insertions(+), 41 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index 7a5c6cd..25c090a 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);
> @@ -772,56 +774,104 @@ 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)
> +static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
> +                                        struct device *dev,
> +                                        struct iommu_fwspec *fwspec)
>   {
> -       int i, ret = 0;
> +       int i;
>         struct arm_smmu_master *master;
> +       struct device_node *dev_node = dev_get_dev_node(dev);
>   
> -	master = find_smmu_master(smmu, masterspec->np);
> +       master = find_smmu_master(smmu, dev_node);
>         if (master) {
>                 dev_err(dev,
>                         "rejecting multiple registrations for master device 
> %s\n",
> -                       masterspec->np->name);
> +                       dev_node->name);
>                 return -EBUSY;
>         }
>   
>   	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
>         if (!master)
>                 return -ENOMEM;
> -       master->of_node = masterspec->np;
>   
> -	ret = iommu_fwspec_init(&master->of_node->dev, smmu->dev);
> -       if (ret) {
> -               kfree(master);
> -               return ret;
> -       }
> -       master->cfg.fwspec = dev_iommu_fwspec_get(&master->of_node->dev);
> -
> -       /* adding the ids here */
> -       ret = iommu_fwspec_add_ids(&masterspec->np->dev,
> -                                  masterspec->args,
> -                                  masterspec->args_count);
> -       if (ret)
> -               return ret;
> +       master->of_node = dev_node;
> +       master->cfg.fwspec = fwspec;
>   
>   	/* Xen: Let Xen know that the device is protected by an SMMU */
> -       dt_device_set_protected(masterspec->np);
> +       dt_device_set_protected(dev_node);
>   
>   	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
> -               for (i = 0; i < master->cfg.fwspec->num_ids; ++i) {
> -                       if (masterspec->args[i] >= smmu->num_mapping_groups) {
> +               for (i = 0; i < fwspec->num_ids; ++i) {
> +                       if (fwspec->ids[i] >= smmu->num_mapping_groups) {
>                                 dev_err(dev,
>                                         "stream ID for master device %s greater than 
> maximum allowed (%d)\n",
> -                                       masterspec->np->name, 
> smmu->num_mapping_groups);
> +                                       dev_node->name, 
> smmu->num_mapping_groups);
>                                 return -ERANGE;
>                         }
>                 }
>         }
> +
>         return insert_smmu_master(smmu, master);
>   }
>   
> +static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
> +{
> +       struct arm_smmu_device *smmu;
> +       struct iommu_fwspec *fwspec;
> +
> +       fwspec = dev_iommu_fwspec_get(dev);
> +       if (fwspec == NULL)
> +               return -ENXIO;
> +
> +       smmu = (struct arm_smmu_device *) find_smmu(fwspec->iommu_dev)
Please don't use explicit cast to remove a const. If you need 
find_smmu() to return a non-const value, then you should drop the const 
from the return function.

> +       if (smmu == NULL)
> +               return -ENXIO;
> +
> +       return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
This feels a bit odd to me to call a "legacy" function from a "generic" 
call. How about remove "legacy" from the function name?
> +}
> +
> +static int arm_smmu_dt_xlate_generic(struct device *dev,
> +                                   const struct of_phandle_args *spec)
Please use dt_phandle_args to stay consistent with the naming and the 
fact the code is mostly Xen specific (though derived from Linux).
>   
> -static __init const struct arm_smmu_device *find_smmu(const struct device *dev)
> +static const struct arm_smmu_device *find_smmu(const struct device *dev)
>   {
>         struct arm_smmu_device *smmu;
>         bool found = false;
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index acf6b62..dd9cf65 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -158,22 +158,7 @@ int iommu_add_dt_device(struct dt_device_node *np)
>            * these callback implemented.
>            */
>           if ( !ops->add_device || !ops->dt_xlate )
> -        {
> -            /*
> -             * Some Device Trees may expose both legacy SMMU and generic
> -             * IOMMU bindings together. However, the SMMU driver is only
> -             * supporting the former and will protect them during the
> -             * initialization. So we need to skip them and not return
> -             * error here.
> -             *
> -             * XXX: This can be dropped when the SMMU is able to deal
> -             * with generic bindings.
> -             */
> -            if ( dt_device_is_protected(np) )
> -                return 0;
> -            else
> -                return -EINVAL;
> -        }
I would add a comment in the commit message explaining the hack in 
iommu_add_dt_device() can be removed.
> +            return -EINVAL;
>   
>           if ( !dt_device_is_available(iommu_spec.np) )
>               break;
Cheers,


--
Julien Grall