diff mbox series

[V4,2/8] iommu/arm: Add ability to handle deferred probing request

Message ID 1568388917-7287-3-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series iommu/arm: Add Renesas IPMMU-VMSA support + Linux's iommu_fwspec | expand

Commit Message

Oleksandr Tyshchenko Sept. 13, 2019, 3:35 p.m. UTC 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

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@epam.com>
CC: Julien Grall <julien.grall@arm.com>

---
Changes V3 -> V4:
    - moved changes related to the "headers ordering"
      to a separate patch
    - added explaination why domain_list is re-used
    - removed the unnecessary pair of outermost parentheses
      when checking the list_empty

Changes V2 -> V3:
    - removed deferred_probe field from struct dt_device_node,
      re-used domain_list instead
    - documented domain_list usage
    - added ASSERT to check that np->domain_list is empty
      before re-using it
    - put deferred_probe_list to init section
    - used more strict logic regarding processing devices in
      the deferred list
    - added more comments to code
    - put headers in alphabetical order
---
 xen/drivers/passthrough/arm/iommu.c | 56 +++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/device.h        |  6 +++-
 xen/include/xen/device_tree.h       |  7 +++++
 3 files changed, 66 insertions(+), 3 deletions(-)

Comments

Julien Grall Sept. 19, 2019, 9:44 a.m. UTC | #1 
Hi Oleksandr,

On 13/09/2019 16:35, Oleksandr Tyshchenko wrote:
> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index f219de9..555acfc 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>
>   
> +/*
> + * Deferred probe list is used to keep track of devices for which driver
> + * requested deferred probing (returned -EAGAIN).
> + */
> +static __initdata LIST_HEAD(deferred_probe_list);
> +
>   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,21 @@ int __init iommu_hardware_setup(void)
>           rc = device_init(np, DEVICE_IOMMU, NULL);
>           if ( !rc )
>               num_iommus++;
> +        else if ( rc == -EAGAIN )
> +        {
> +            /*
> +             * We expect nobody uses device's domain_list at such early stage,

NIT: s/We expect nobody uses/Nobody should use/

> +             * so we can re-use it to link the device in the deferred list to
> +             * avoid introducing extra list_head field in struct dt_device_node.
> +             */
> +            ASSERT(list_empty(&np->domain_list));

[...]

>   void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
> 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 9a7a8f2..3702e9b 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -92,6 +92,13 @@ struct dt_device_node {
>   
>       /* IOMMU specific fields */
>       bool is_protected;
> +    /*
> +     * The main purpose of this list node is to link the structure in the list

s/node//?

> +     * of devices assigned to domain.
> +     *
> +     * Boot code (iommu_hardware_setup) re-uses this list to link the structure
> +     * in the list of devices for which driver requested deferred probing.
> +     */
>       struct list_head domain_list;
>   
>       struct device dev;
> 

With the two above addressed and pending the patch it depends on [1] is acked:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg01924.html
Oleksandr Tyshchenko Sept. 19, 2019, 9:57 a.m. UTC | #2 
On 19.09.19 12:44, Julien Grall wrote:
> Hi Oleksandr,

Hi, Julien.


>
> On 13/09/2019 16:35, Oleksandr Tyshchenko wrote:
>> diff --git a/xen/drivers/passthrough/arm/iommu.c 
>> b/xen/drivers/passthrough/arm/iommu.c
>> index f219de9..555acfc 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>
>>   +/*
>> + * Deferred probe list is used to keep track of devices for which 
>> driver
>> + * requested deferred probing (returned -EAGAIN).
>> + */
>> +static __initdata LIST_HEAD(deferred_probe_list);
>> +
>>   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,21 @@ int __init iommu_hardware_setup(void)
>>           rc = device_init(np, DEVICE_IOMMU, NULL);
>>           if ( !rc )
>>               num_iommus++;
>> +        else if ( rc == -EAGAIN )
>> +        {
>> +            /*
>> +             * We expect nobody uses device's domain_list at such 
>> early stage,
>
> NIT: s/We expect nobody uses/Nobody should use/

ok


>
>> +             * so we can re-use it to link the device in the 
>> deferred list to
>> +             * avoid introducing extra list_head field in struct 
>> dt_device_node.
>> +             */
>> +            ASSERT(list_empty(&np->domain_list));
>
> [...]
>
>>   void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct 
>> domain *d)
>> 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 9a7a8f2..3702e9b 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -92,6 +92,13 @@ struct dt_device_node {
>>         /* IOMMU specific fields */
>>       bool is_protected;
>> +    /*
>> +     * The main purpose of this list node is to link the structure 
>> in the list
>
> s/node//?

ok


>
>> +     * of devices assigned to domain.
>> +     *
>> +     * Boot code (iommu_hardware_setup) re-uses this list to link 
>> the structure
>> +     * in the list of devices for which driver requested deferred 
>> probing.
>> +     */
>>       struct list_head domain_list;
>>         struct device dev;
>>
>
> With the two above addressed and pending the patch it depends on [1] 
> is acked:
>
> Reviewed-by: Julien Grall <julien.grall@arm.com>

Thank you.


Can I do anything to speed up [1]? That patch was tested and worked fine.


>
> Cheers,
>
> [1] 
> https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg01924.html
>
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index f219de9..555acfc 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>
 
+/*
+ * Deferred probe list is used to keep track of devices for which driver
+ * requested deferred probing (returned -EAGAIN).
+ */
+static __initdata LIST_HEAD(deferred_probe_list);
+
 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,21 @@  int __init iommu_hardware_setup(void)
         rc = device_init(np, DEVICE_IOMMU, NULL);
         if ( !rc )
             num_iommus++;
+        else if ( rc == -EAGAIN )
+        {
+            /*
+             * We expect nobody uses device's domain_list at such early stage,
+             * so we can re-use it to link the device in the deferred list to
+             * avoid introducing extra list_head field in struct dt_device_node.
+             */
+            ASSERT(list_empty(&np->domain_list));
+
+            /*
+             * Driver requested deferred probing, so add this device to
+             * the deferred list for further processing.
+             */
+            list_add(&np->domain_list, &deferred_probe_list);
+        }
         /*
          * Ignore the following error codes:
          *   - EBADF: Indicate the current not is not an IOMMU
@@ -61,7 +82,38 @@  int __init iommu_hardware_setup(void)
             return rc;
     }
 
-    return ( num_iommus > 0 ) ? 0 : -ENODEV;
+    /* Return immediately if there are no initialized devices. */
+    if ( !num_iommus )
+        return list_empty(&deferred_probe_list) ? -ENODEV : -EAGAIN;
+
+    rc = 0;
+
+    /*
+     * Process devices in the deferred list if it is not empty.
+     * Check that at least one device is initialized at each loop, otherwise
+     * we may get an infinite loop. Also stop processing if we got an error
+     * other than -EAGAIN.
+     */
+    while ( !list_empty(&deferred_probe_list) && num_iommus )
+    {
+        num_iommus = 0;
+
+        list_for_each_entry_safe ( np, tmp, &deferred_probe_list, domain_list )
+        {
+            rc = device_init(np, DEVICE_IOMMU, NULL);
+            if ( !rc )
+            {
+                num_iommus++;
+
+                /* Remove initialized device from the deferred list. */
+                list_del_init(&np->domain_list);
+            }
+            else if ( rc != -EAGAIN )
+                return rc;
+        }
+    }
+
+    return rc;
 }
 
 void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
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 9a7a8f2..3702e9b 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -92,6 +92,13 @@  struct dt_device_node {
 
     /* IOMMU specific fields */
     bool is_protected;
+    /*
+     * The main purpose of this list node is to link the structure in the list
+     * of devices assigned to domain.
+     *
+     * Boot code (iommu_hardware_setup) re-uses this list to link the structure
+     * in the list of devices for which driver requested deferred probing.
+     */
     struct list_head domain_list;
 
     struct device dev;