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

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

Hi Oleksandr,

On 13/08/2019 13:35, Oleksandr wrote:

On 12.08.19 22:46, Julien Grall wrote:
Hi Oleksandr,

Hi, Julien

On 8/12/19 1:01 PM, Oleksandr wrote:
On 12.08.19 14:11, Julien Grall wrote:
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->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)?

Agree that only boot code uses deferred_probe field. I will consider re-using domain_list. Could you please clarify regarding ASSERT (where to put and what to check).

What I meant is adding an ASSERT to check that np->domain_list is at empty at least before trying to add in the list. This would help to debug any potential issue if we end up to use domain_list earlier in the future. I can't see why it would as iommu is called earlier, but who knows :).

Got it. Thank you for clarification.

  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 )
+        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.

Sounds reasonable. Will do.

Just to clarify:

 >>> A better condition would be to check that at least one IOMMU is added at each loop.

Maybe, not only added (rc == 0), but driver didn't request deferred probe (rc != -EAGAIN).

I think adding an IOMMU is enough. If you return an error other than -EAGAIN here after deferring probing, then you are likely going to fail at the next loop. So better to stop early.

It makes sense.

I realize this not what the current code is doing (I know I wrote it ;)). But I am not sure it is sane to continue if only part of the IOMMUs are initialized. Most likely you will see an error much later that may be not trivial to find out.

Imagine you want to passthrough you network card to a guest but the IOMMU initialization failed...

Oh, agree.

As I understand, the new strict logic would be the following:

If initialization for at least one IOMMU device failed (device_init returns an error other than -EAGAIN), we should stop and return an error to upper layer (even if num_iommus > 0). No matter whether it is during the first attempt or after deferring probe. We don't allow the "I/O virtualisation" to be enabled (iommu_enabled == true) with only part of the IOMMU devices being initialized. Is my understanding correct?

Let me summarize the discussion we had on IRC :). Without your patch, Xen may initialize only half the IOMMUs. If the device is behind an IOMMU that wasn't initialized, then we have two possibility: 1) The device was already mark as protected (if using the old binding in the SMMU). Xen will not be able to assign the device to Dom0 and therefore Xen will crash (not able to build dom0). For domU, it will depend whether the configuration contain the options 'dtdev'. If the option is specified, then guest will fail to build. On the contrary if the option isn't specified then the guest will boot and this could either lead to transaction failure (if the IOMMU was already reset) or bypassing the IOMMU. Note that the latter can today happen if your IOMMU was disabled. But we can't do much against it. 2) The device is not marked as protected. Xen will not be able to "assign" the device to Dom0 and this could either lead to the device bypassing the IOMMU or a transaction failure. For domU, the problem is similar to 1).

In the case of the SMMU driver, we only support old bindings. So devices are marked as protected during SMMU initialization. This is done before the SMMU is reset. Before reset the SMMU will bypassed.

So the risk is to have an half secure system and may be unnoticed until later. I realize this is the current behavior, so not very ideal.

It feels to me if the user requested to use IOMMU then if we should panic if any of the available IOMMU are not initialized correctly. This will save a lot of debug afterwards.

@Stefano, any opinions?


Julien Grall

Xen-devel mailing list



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