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

Re: [Xen-devel] [PATCH V2 5/6] iommu/arm: Introduce iommu_add_dt_device API

On 13.08.19 16:49, Julien Grall wrote:
Hi Oleksandr,

Hi Julien

On 8/2/19 5:39 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 binding [1] and previously
added "iommu_fwspec" support.

New function parses the DT binding, prepares "dev->iommu_fwspec"
with correct information and calls the IOMMU driver using "add_device"
callback to register new DT device.
The IOMMU driver's responsibility is to check whether "dev->iommu_fwspec"
is initialized and mark that device as protected.

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.

The upcoming IPMMU driver will have "add_device" callback implemented.

I hope, this patch won't break SMMU driver's functionality,
which doesn't have this callback implemented.

The last two sentence does not really belong to the commit message. So I think they should go after ---.

Anyway, the only concern for the SMMU is to not break the old bindings. New bindings are not supported, so it does not matter whether they are broken or not. Once this series is merged, we can have a look how new bindings for the SMMU can be supported.

Sounds reasonable.

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

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
  xen/arch/arm/domain_build.c         | 12 ++++++++++
  xen/drivers/passthrough/arm/iommu.c | 45 +++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/iommu.h         |  3 +++
  3 files changed, 60 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d983677..d67f7d4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1241,6 +1241,18 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
      u64 addr, size;
      bool need_mapping = !dt_device_for_passthrough(dev);
  +    if ( dt_parse_phandle(dev, "iommus", 0) )

I don't particularly like this check. dt_parse_phandle is non-trivial to execute.

TBH, what we should do is trying to call iommu_add_dt_device if IOMMU is enabled. We can then return a recognizable value to tell the device is not protected.

Well. Don't really mind.

+    {
+        dt_dprintk("%s add to iommu\n", dt_node_full_name(dev));
+        res = iommu_add_dt_device(dev);
+        if ( res )
+        {
+            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 3195919..19516af 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -113,3 +113,48 @@ 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 || !ops || !ops->add_device )
+        return 0;

While I agree that !iommu_enabled should return 0, for the two others I am not entirely sure this is the right thing to do.

!ops is definitely an error because if you have the IOMMU enabled then you should have ops installed.


!ops->add_device means that you will not be able to add any device using the new bindings. IOW, the device will be unusable later one as most likely the IOMMU was configured to deny any transaction. Therefore, this should return an error.

The initial reason *was* to not break SMMU which hasn't had add_device callback implemented yet. But, I got your point regarding SMMU above (the only concern for the SMMU is to not break the old bindings), so agree here.

+    if ( dev_iommu_fwspec_get(dev) )
+        return -EEXIST;
+    /* According to the Documentation/devicetree/bindings/iommu/iommu.txt */

This file does not exist in Xen, so Linux should at least be mentioned in the comment.

Will do.

+    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;
+        rc = iommu_fwspec_add_ids(dev, iommu_spec.args, 1);

Here you assume that there will at least always be one cells and the first cell is the IDs. For a first, #iommu-cells may be 0 (and therefore no cells) when the master IOMMU device cannot be configured.

Furthermore, the content of the #iommu-cells depends on the driver. This is why Linux provides a callback of_xlate to let the driver decide how to interpret it.

For instance, the SMMU can support either 1 or 2 cells. It also may need to look-up for other properties in the node (e.g stream-match-mask).

So I think we also want to provide the of_xlate in Xen.

Hmm, I was thinking how to end up with only one callback re-used (add_device), really didn't want to add a new one (of_xlate). But, I didn't take into the account that this stuff is a really driver-depended. So, likely yes, we need to provide of_xlate callback.

May I ask some questions to clarify:

1. Do you want me to introduce of_xlate callback in a separate patch (under CONFIG_ARM?)?

2. Can we avoid introducing new API for that callback? iommu_add_dt_device will be able to call it directly.


Oleksandr Tyshchenko

Xen-devel mailing list



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