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

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




On 09.09.19 18:04, Julien Grall wrote:
Hi Oleksandr,

Hi Julien




On 8/20/19 7:09 PM, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

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.

Good point. I will update description.





[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt

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

---
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".

Sounds reasonable.




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

Agree. Will do.




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

s/According/According to/

ok




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

ok



+         * It should also initialize/verify that device.

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

Yes. Saying "initialize/verify that device" I meant to verify passed DT IOMMU specifier and initialize driver's private data for this device (iommu_priv). But, this is obvious.

I will remove confusing word "initialize" or even the whole sentence.




+         */
+        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.

Yes. The same as for comment above. I will remove confusing word "initialize".


--
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®.