Re: [Xen-devel] [PATCH V3 7/8] iommu/arm: Introduce iommu_add_dt_device API

[Julien Grall <julien.grall@arm.com>]

Hi Oleksandr,

On 8/20/19 7:09 PM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> This patch adds new iommu_add_dt_device API for adding DT device
> to the IOMMU using generic IOMMU DT bindings [1] and previously
> added "iommu_fwspec" support and "add_device/of_xlate" callbacks.
> 
> New function does the following:
> - Parse the DT bindings according to the specification
> - Provide DT IOMMU specifier which describes the IOMMU master
>    interfaces of that device (device IDs, etc) to the driver
> - Add master device to the IOMMU if latter is present and available
> 
> The additional benefit here is to avoid to go through the whole DT
> multiple times in IOMMU driver trying to locate master devices which
> belong to each IOMMU device being probed.

So the commit title/message describes the new function 
iommu_add_dt_device, but not the main important thing i.e. "Why is it 
called when building dom0".

While I agree the new function is the big part of the function what 
matter is we need to register device using the generic IOMMU bindings 
before assigning the device to a domain. The split is to keep separate 
"add" and "assign". The later can be called from dom0.

> 
> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> ---
> Changes V2 -> V3:
>      - clarified patch description
>      - clarified comments in code
>      - modified to provide DT IOMMU specifier to the driver
>        using "of_xlate" callback
>      - documented function usage
>      - modified to return an error if ops is not present/implemented,
>      - added ability to return a possitive value to indicate
>        that device doesn't need to be protected
>      - removed check for the "iommu" property presence
>        in the common code
>      - included <asm/iommu_fwspec.h> directly
> ---
>   xen/arch/arm/domain_build.c         | 11 ++++++++
>   xen/drivers/passthrough/arm/iommu.c | 55 +++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/iommu.h         | 11 ++++++++
>   3 files changed, 77 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index e79d4e2..159ea6a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1240,6 +1240,7 @@ static int __init map_device_children(struct domain *d,
>   
>   /*
>    * For a given device node:
> + *  - Try to call iommu_add_dt_device to protect the device by an IOMMU
>    *  - Give permission to the guest to manage IRQ and MMIO range
>    *  - Retrieve the IRQ configuration (i.e edge/level) from device tree
>    * When the device is not marked for guest passthrough:
> @@ -1257,6 +1258,16 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
>       u64 addr, size;
>       bool need_mapping = !dt_device_for_passthrough(dev);
>   
> +    dt_dprintk("%s add to iommu\n", dt_node_full_name(dev));

This message is slightly confusing. You are not adding the device, you 
are trying to. So how about "Check if %s is behind an IOMMU and add it".

> +
> +    res = iommu_add_dt_device(dev);
> +    if ( res < 0 )
> +    {
> +        printk(XENLOG_ERR "Failed to add %s to the IOMMU\n",
> +               dt_node_full_name(dev));
> +        return res;
> +    }
> +
>       nirq = dt_number_of_irq(dev);
>       naddr = dt_number_of_address(dev);
>   
> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index 72a30e0..47e4bc6 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -20,6 +20,7 @@
>   #include <xen/lib.h>
>   
>   #include <asm/device.h>
> +#include <asm/iommu_fwspec.h>
>   
>   /*
>    * Deferred probe list is used to keep track of devices for which driver
> @@ -139,3 +140,57 @@ int arch_iommu_populate_page_table(struct domain *d)
>   void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>   {
>   }
> +
> +int __init 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;
> +
> +    if ( !iommu_enabled )
> +        return 1;
> +
> +    if ( !ops || !ops->add_device || !ops->of_xlate )

The SMMU does not implement of_xlate(). It is actually only mandatory if 
you are using the generic bindings. So I would only check ops->of_xlate 
if "iommus" exists.

> +        return -EINVAL;
> +
> +    if ( dev_iommu_fwspec_get(dev) )
> +        return -EEXIST; > +
> +    /* According Documentation/devicetree/bindings/iommu/iommu.txt from Linux */

s/According/According to/

> +    while ( !dt_parse_phandle_with_args(np, "iommus", "#iommu-cells",
> +                                        index, &iommu_spec) )
> +    {
> +        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's responsibility is to decide how to interpret them.

NIT: "The driver is responsible to decide...".

> +         * It should also initialize/verify that device.

What do you mean? of_xlate should mostly translate the specifier to fwspec.

> +         */
> +        rc = ops->of_xlate(dev, &iommu_spec);
> +        if ( rc )
> +            break;
> +
> +        index++;
> +    }
> +
> +    /*
> +     * Add master device to the IOMMU if latter is present and available.
> +     * The driver's responsibility is to check whether that device
> +     * was initialized/verified before and mark that device as protected.

I don't understand the last sentence. For me, "device" refers to what's 
pointed by "dev". But the IOMMU driver is not responsible for 
initializing the device.

> +     */
> +    if ( !rc )
> +        rc = ops->add_device(0, dev);
> +
> +    if ( rc < 0 )
> +        iommu_fwspec_free(dev);
> +
> +    return rc;
> +}
> diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
> index 20d865e..e75359c 100644
> --- a/xen/include/asm-arm/iommu.h
> +++ b/xen/include/asm-arm/iommu.h
> @@ -26,6 +26,17 @@ struct arch_iommu
>   const struct iommu_ops *iommu_get_ops(void);
>   void iommu_set_ops(const struct iommu_ops *ops);
>   
> +/*
> + * Helper to add master device to the IOMMU using generic IOMMU DT bindings.
> + *
> + * Return values:
> + *  0 : device is protected by an IOMMU
> + * <0 : device is not protected by an IOMMU, but must be (error condition)
> + * >0 : device doesn't need to be protected by an IOMMU
> + *      (IOMMU is not enabled/present or device is not connected to it).
> + */
> +int iommu_add_dt_device(struct dt_device_node *np);
> +
>   /* mapping helpers */
>   int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
>                                       unsigned int flags,
> 

Cheers,

-- 
Julien Grall

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