[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




On 09.09.19 15:24, Julien Grall wrote:
Hi Oleksandr,

Hi, Julien



The code looks code, few comments below.

On 8/20/19 7:09 PM, 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.

While here, order the headers alphabetically.

It is common to clean code you modify in the same patch if they are not complex, but this is not the case here... Indeed, the headers are not touched. So I would prefer this to be in a separate patch unless it breaks the compilation without it.

Well, will make this change in a separate patch.





Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>

---
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 | 59 ++++++++++++++++++++++++++++++++++---
  xen/include/asm-arm/device.h        |  6 +++-
  xen/include/xen/device_tree.h       |  7 +++++
  3 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index f219de9..72a30e0 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -15,11 +15,20 @@
   * GNU General Public License for more details.
   */
  -#include <xen/lib.h>
-#include <xen/iommu.h>
  #include <xen/device_tree.h>
+#include <xen/iommu.h>
+#include <xen/lib.h>
+
  #include <asm/device.h>
  +/*
+ * Deferred probe list is used to keep track of devices for which driver
+ * requested deferred probing (returned -EAGAIN).
+ *
+ * We re-use device's domain_list to link the device in the deferred list.
+ */
+static __initdata LIST_HEAD(deferred_probe_list);
+
  static const struct iommu_ops *iommu_ops;
    const struct iommu_ops *iommu_get_ops(void)
@@ -42,7 +51,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 +60,17 @@ 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 domain_list at such early stage. */

AFAICT, this comment is only an English version of the next line. It would be best if you explain why domain_list is re-used here.

Will do.




+ 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 +81,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;

NIT: Do you need the outer ()?

No, I don't.


--
Regards,

Oleksandr Tyshchenko


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.