Re: [PATCH v4 3/7] iommu/arm: Add iommu_dt_xlate()

[Julien Grall <julien@xxxxxxx>]

Hi Stewart,

On 07/06/2023 04:02, Stewart Hildebrand wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> Move code for processing DT IOMMU specifier to a separate helper.
> This helper will be re-used for adding PCI devices by the subsequent
> patches as we will need exact the same actions for processing
> DT PCI-IOMMU specifier.
>
> While at it introduce NO_IOMMU to avoid magic "1".
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> ---
> v3->v4:
> * make dt_phandle_args *iommu_spec const
> * move !ops->add_device check to helper
>
> v2->v3:
> * no change
>
> v1->v2:
> * no change
>
> downstream->v1:
> * trivial rebase
> * s/dt_iommu_xlate/iommu_dt_xlate/
>
> (cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from
>   the downstream branch poc/pci-passthrough from
>   https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
> ---
>   xen/drivers/passthrough/device_tree.c | 47 ++++++++++++++++++---------
>   1 file changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index c60e78eaf556..ff9e66ebf92a 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -127,15 +127,42 @@ int iommu_release_dt_devices(struct domain *d)
>       return 0;
>   }
>   
> +/* This correlation must not be altered */
Please expand why.

> +#define NO_IOMMU    1
Given that the value is returned, should not this be moved to an header 
and used by the callers?
> +
> +static int iommu_dt_xlate(struct device *dev,
> +                          const struct dt_phandle_args *iommu_spec)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    int rc;
> +
> +    if ( !ops->dt_xlate )
> +        return -EINVAL;
> +
> +    if ( !dt_device_is_available(iommu_spec->np) )
> +        return NO_IOMMU;
> +
> +    rc = iommu_fwspec_init(dev, &iommu_spec->np->dev);
> +    if ( rc )
> +        return rc;
> +
> +    /*
> +     * Provide DT IOMMU specifier which describes the IOMMU master
> +     * interfaces of that device (device IDs, etc) to the driver.
> +     * The driver is responsible to decide how to interpret them.
> +     */
> +    return ops->dt_xlate(dev, iommu_spec);
> +}
> +
>   int iommu_add_dt_device(struct dt_device_node *np)
>   {
>       const struct iommu_ops *ops = iommu_get_ops();
>       struct dt_phandle_args iommu_spec;
>       struct device *dev = dt_to_dev(np);
> -    int rc = 1, index = 0;
> +    int rc = NO_IOMMU, index = 0;
>   
>       if ( !iommu_enabled )
> -        return 1;
> +        return NO_IOMMU;
>   
>       if ( !ops )
>           return -EINVAL;
> @@ -163,22 +190,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
>            * The driver which supports generic IOMMU DT bindings must have
>            * these callback implemented.
The 's' was missing to callback. But now, I think you want to replace 
'these' with 'this' as you move the second check.
>            */
> -        if ( !ops->add_device || !ops->dt_xlate )
> +        if ( !ops->add_device )
>               return -EINVAL;
>   
> -        if ( !dt_device_is_available(iommu_spec.np) )
> -            break;
> -
> -        rc = iommu_fwspec_init(dev, &iommu_spec.np->dev);
> -        if ( rc )
> -            break;
> -
> -        /*
> -         * Provide DT IOMMU specifier which describes the IOMMU master
> -         * interfaces of that device (device IDs, etc) to the driver.
> -         * The driver is responsible to decide how to interpret them.
> -         */
> -        rc = ops->dt_xlate(dev, &iommu_spec);
> +        rc = iommu_dt_xlate(dev, &iommu_spec);
>           if ( rc )
>               break;
>   
Cheers,

--
Julien Grall