Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request

[Julien Grall <julien.grall@xxxxxxx>]

Hi Oleksandr,

On 02/08/2019 17:39, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> This patch adds minimal required support to General IOMMU framework
> to be able to handle a case when IOMMU driver requesting deferred
> probing for a device.
>
> In order not to pull Linux's error code (-EPROBE_DEFER) to Xen
> we have chosen -EAGAIN to be used for indicating that device
> probing is deferred.
>
> This is needed for the upcoming IPMMU driver which may request
> deferred probing depending on what device will be probed the first
> (there is some dependency between these devices, Root device must be
> registered before Cache devices. If not the case, driver will deny
> further Cache device probes until Root device is registered).
> As we can't guarantee a fixed pre-defined order for the device nodes
> in DT, we need to be ready for the situation where devices being
> probed in "any" order.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> ---
>   xen/common/device_tree.c            |  1 +
>   xen/drivers/passthrough/arm/iommu.c | 35 ++++++++++++++++++++++++++++++++++-
>   xen/include/asm-arm/device.h        |  6 +++++-
>   xen/include/xen/device_tree.h       |  1 +
>   4 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index e107c6f..6f37448 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1774,6 +1774,7 @@ static unsigned long __init unflatten_dt_node(const void 
> *fdt,
>           /* By default the device is not protected */
>           np->is_protected = false;
>           INIT_LIST_HEAD(&np->domain_list);
> +        INIT_LIST_HEAD(&np->deferred_probe);
I am not entirely happy to add a new list_head field per node just for the 
benefits of boot code. Could we re-use domain_list (with a comment in the code 
and appropriate ASSERT)?
>   
>           if ( new_format )
>           {
> diff --git a/xen/drivers/passthrough/arm/iommu.c 
> b/xen/drivers/passthrough/arm/iommu.c
> index 2135233..3195919 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -20,6 +20,12 @@
>   #include <xen/device_tree.h>
>   #include <asm/device.h>
>   
> +/*
> + * Used to keep track of devices for which driver requested deferred probing
> + * (returns -EAGAIN).
> + */
> +static LIST_HEAD(deferred_probe_list);
This wants to be in init section as this is only used at boot.

> +
>   static const struct iommu_ops *iommu_ops;
>   
>   const struct iommu_ops *iommu_get_ops(void)
> @@ -42,7 +48,7 @@ void __init iommu_set_ops(const struct iommu_ops *ops)
>   
>   int __init iommu_hardware_setup(void)
>   {
> -    struct dt_device_node *np;
> +    struct dt_device_node *np, *tmp;
>       int rc;
>       unsigned int num_iommus = 0;
>   
> @@ -51,6 +57,33 @@ int __init iommu_hardware_setup(void)
>           rc = device_init(np, DEVICE_IOMMU, NULL);
>           if ( !rc )
>               num_iommus++;
> +        else if (rc == -EAGAIN)
> +            /*
> +             * Driver requested deferred probing, so add this device to
> +             * the deferred list for further processing.
> +             */
> +            list_add(&np->deferred_probe, &deferred_probe_list);
> +    }
> +
> +    /*
> +     * Process devices in the deferred list if at least one successfully
> +     * probed device is present.
> +     */
I think this can turn into an infinite loop if all device in deferred_probe_list 
still return -EDEFER_PROBE and num_iommus is a non-zero.
A better condition would be to check that at least one IOMMU is added at each 
loop. If not, then we should bail with an error because it likely means 
something is buggy.
> +    while ( !list_empty(&deferred_probe_list) && num_iommus )
> +    {
> +        list_for_each_entry_safe ( np, tmp, &deferred_probe_list,
> +                                   deferred_probe )
> +        {
> +            rc = device_init(np, DEVICE_IOMMU, NULL);
> +            if ( !rc )
> +                num_iommus++;
> +            if ( rc != -EAGAIN )
> +                /*
> +                 * Driver didn't request deferred probing, so remove this 
> device
> +                 * from the deferred list.
> +                 */
> +                list_del_init(&np->deferred_probe);
> +        }
>       }
>   
>       return ( num_iommus > 0 ) ? 0 : -ENODEV;
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 63a0f36..ee1c3bc 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -44,7 +44,11 @@ struct device_desc {
>       enum device_class class;
>       /* List of devices supported by this driver */
>       const struct dt_device_match *dt_match;
> -    /* Device initialization */
> +    /*
> +     * Device initialization.
> +     *
> +     * -EAGAIN is used to indicate that device probing is deferred.
> +     */
>       int (*init)(struct dt_device_node *dev, const void *data);
>   };
>   
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 8315629..71b0e47 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -93,6 +93,7 @@ struct dt_device_node {
>       /* IOMMU specific fields */
>       bool is_protected;
>       struct list_head domain_list;
> +    struct list_head deferred_probe;
>   
>       struct device dev;
>   };
Cheers,

--
Julien Grall

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