[PATCH 0/7] Generic IOMMU bindings support for Xen platform devices

[PATCH 0/7] Generic IOMMU bindings support for Xen platform devices

[Wei Chen]

The "mmu-masters" is a property of ARM legacy SMMU dt-binding. This
property will be deprecated in favour of the generic IOMMU bindings.
In this case we have to add the generic IOMMU bindings support for
Xen platform devices.

---
This series has been tested on Seattle SoftIron server. The platform device
can be passthrough to guest with both Legacy and Generic IOMMU bindings.
I noticed that Sameer has a iommu_fwspec series in review. It would be nice
to look at it and see if we can re-use it in this series.

Wei Chen (7):
  xen/arm: SMMU: Implement the add_device callback in SMMU
  xen/arm: SMMU: Introduce a helper to add DT device to SMMU
  xen/arm: Prepare SMMU resources for protected devices
  xen/arm: SMMU: Detect types of device tree binding
  xen/arm: SMMU: Keep registering legacy master in SMMU probe
  xen/arm: SMMU: Support generic IOMMU bindings
  xen: Fix a typo in error message of iommu_do_dt_domctl

 xen/arch/arm/domain_build.c           |  12 ++
 xen/drivers/passthrough/arm/smmu.c    | 232 ++++++++++++++++++++++++++++++----
 xen/drivers/passthrough/device_tree.c |  24 +++-
 xen/include/xen/iommu.h               |   1 +
 4 files changed, 243 insertions(+), 26 deletions(-)

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/7] xen/arm: SMMU: Implement the add_device callback in SMMU

[Wei Chen]

This add_device callback function is taking care of adding a device
to SMMU and make sure it is fully prepare to be used by the SMMU
afterwards.

In previous code, we don't implement the add_device callback in
iommu_ops for ARM SMMU. We placed the work of add_device to
assign_device callback. The function assign_device should not care
about adding the device to an iommu_group. It might not even be
able to decide how to do that. In this patch, we move this work
back to add_device callback.

This add_device callback is only called while we are handling all
devices for constructing the Domain0.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 74c09b0..2efa52d 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2591,6 +2591,26 @@ static void arm_smmu_destroy_iommu_domain(struct iommu_domain *domain)
 	xfree(domain);
 }
 
+static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
+{
+	if (dt_device_is_protected(dev->of_node)) {
+		if (!dev->archdata.iommu) {
+			dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
+			if (!dev->archdata.iommu)
+				return -ENOMEM;
+		}
+
+		if (!dev_iommu_group(dev))
+			return arm_smmu_add_device(dev);
+	}
+
+	/*
+	 * Return 0 if the device is not protected to follow the behavior
+	 * of PCI add device.
+	 */
+	return 0;
+}
+
 static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
 			       struct device *dev, u32 flag)
 {
@@ -2600,17 +2620,8 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
 
 	xen_domain = dom_iommu(d)->arch.priv;
 
-	if (!dev->archdata.iommu) {
-		dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
-		if (!dev->archdata.iommu)
-			return -ENOMEM;
-	}
-
-	if (!dev_iommu_group(dev)) {
-		ret = arm_smmu_add_device(dev);
-		if (ret)
-			return ret;
-	}
+	if (!dev_iommu_group(dev))
+	    return -ENODEV;
 
 	spin_lock(&xen_domain->lock);
 
@@ -2784,6 +2795,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
     .teardown = arm_smmu_iommu_domain_teardown,
     .iotlb_flush = arm_smmu_iotlb_flush,
     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
+    .add_device = arm_smmu_xen_add_device,
     .assign_device = arm_smmu_assign_dev,
     .reassign_device = arm_smmu_reassign_dev,
     .map_page = arm_smmu_map_page,
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/7] xen/arm: SMMU: Introduce a helper to add DT device to SMMU

[Wei Chen]

In current code, we only have the iommu_add_device to add PCI device
to IOMMU. But for ARM SMMU, we don't have a separate helper to add
platform device with device tree to SMMU. This work was included in
the iommu_assign_dt_device. But sometimes, we just want to add device
to SMMU to do some preparation for further use. In this case, we can't
call iommu_assign_dt_device.

In previous patch, we have implement the add_device callback for SMMU,
so we can separate this work from assign_device now.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++++++
 xen/include/xen/iommu.h               |  1 +
 2 files changed, 21 insertions(+)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 99ed49e..a8f403a 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -24,6 +24,26 @@
 
 static spinlock_t dtdevs_lock = SPIN_LOCK_UNLOCKED;
 
+int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev)
+{
+    int rc;
+
+    struct domain_iommu *hd = dom_iommu(d);
+
+    if ( !iommu_enabled || !hd->platform_ops ||
+         !hd->platform_ops->add_device )
+        return 0;
+
+    spin_lock(&dtdevs_lock);
+
+    /* The devfn field doesn't matter to DT device. */
+    rc = hd->platform_ops->add_device(0, dt_to_dev(dev));
+
+    spin_unlock(&dtdevs_lock);
+
+    return rc;
+}
+
 int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
 {
     int rc = -EBUSY;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5803e3f..ec03faa 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -132,6 +132,7 @@ void iommu_read_msi_from_ire(struct msi_desc *msi_desc, struct msi_msg *msg);
 #ifdef CONFIG_HAS_DEVICE_TREE
 #include <xen/device_tree.h>
 
+int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev);
 int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
 int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
 int iommu_dt_domain_init(struct domain *d);
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/7] xen/arm: Prepare SMMU resources for protected devices

[Wei Chen]

In previous code, while we are constructing Dom0, we will assign
all devices except passthrough devices to Dom0. In the later, when
we start the DomU, the assign_device will prepare SMMU resources
for the devices passthrough to DomU. This is ok when we kept the
add_device code in assign_device. But currently, we have separated
add_device from assign_device. If we don't prepare SMMU resources
for passthrough devices, these devices would not work properly in
DomU.

So, while we are handling all devices DT node in construction Dom0,
we will call add_device to prepare SMMU resources for all protected
devices, regardless of passthrough or not.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/arch/arm/domain_build.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c6776d7..6aea427 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1082,6 +1082,18 @@ static int handle_device(struct domain *d, struct dt_device_node *dev,
     dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n",
                dt_node_full_name(dev), need_mapping, nirq, naddr);
 
+    /*
+     * If this device is behind the SMMU, the add_device callback will
+     * prepare resource for it. Otherwise, add_device has no effect.
+     */
+    res = iommu_add_dt_device(d, dev);
+    if ( res )
+    {
+        printk(XENLOG_ERR "Failed to add device to IOMMU for %s\n",
+               dt_node_full_name(dev));
+        return res;
+    }
+
     if ( dt_device_is_protected(dev) && need_mapping )
     {
         dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/7] xen/arm: SMMU: Detect types of device tree binding

[Wei Chen]

The device tree provides two types of IOMMU bindings, one is legacy
another is generic. The legacy bindings will be depercated in favour
of the generic bindings. But in the transitional period, we have to
support both of them.

The codes to handle these two types of bindings are very differnet,
so we have to detect the binding types while doing SMMU probing.

This detect code is based on Linux ARM SMMUv2 driver:
https://github.com/torvalds/linux/blob/master/drivers/iommu/arm-smmu.c

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 2efa52d..441c296 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -143,6 +143,8 @@ typedef enum irqreturn irqreturn_t;
 
 #define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
 
+#define pr_notice(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
+
 /* Alias to Xen allocation helpers */
 #define kfree xfree
 #define kmalloc(size, flags)		_xmalloc(size, sizeof(void *))
@@ -681,6 +683,8 @@ struct arm_smmu_option_prop {
 	const char *prop;
 };
 
+static bool using_legacy_binding, using_generic_binding;
+
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
 	{ 0, NULL},
@@ -2289,6 +2293,25 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	struct rb_node *node;
 	struct of_phandle_args masterspec;
 	int num_irqs, i, err;
+	bool legacy_binding;
+
+	/*
+	 * Xen: Do the same check as Linux. Checking the SMMU device tree bindings
+	 * are either using generic or legacy one.
+	 *
+	 * The "mmu-masters" property is only existed in legacy bindings.
+	 */
+	legacy_binding = dt_find_property(dev->of_node, "mmu-masters", NULL);
+	if (legacy_binding && !using_generic_binding) {
+		if (!using_legacy_binding)
+			pr_notice("deprecated \"mmu-masters\" DT property in use\n");
+		using_legacy_binding = true;
+	} else if (!legacy_binding && !using_legacy_binding) {
+		using_generic_binding = true;
+	} else {
+		dev_err(dev, "not probing due to mismatched DT properties\n");
+		return -ENODEV;
+	}
 
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
 	if (!smmu) {
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 5/7] xen/arm: SMMU: Keep registering legacy master in SMMU probe

[Wei Chen]

The legacy IOMMU bindings place the SMMU MasterIDs in the SMMU device
tree node. In current code, we register the SMMU masters while probing
SMMU. It's better to keep registering legacy masters in the SMMU probing
progress. If we move registering legacy SMMU masters to add_device or
assign_device, we have to go through all SMMUs to find correct SMMU for
master device. It's inefficient and doesn't bring any enhancement.

Similarly, if we want to register generic masters in SMMU probing, we
have to go through whole device tree to find master devices for all
SMMUs. So we only keep registering legacy master in SMMU probing.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 441c296..895024c 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2375,21 +2375,28 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	i = 0;
 	smmu->masters = RB_ROOT;
-	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
-					   "#stream-id-cells", i,
-					   &masterspec)) {
-		err = register_smmu_master(smmu, dev, &masterspec);
-		if (err) {
-			dev_err(dev, "failed to add master %s\n",
-				masterspec.np->name);
-			goto out_put_masters;
-		}
+	/*
+	 * The SMMU MasterIDs are listed in SMMU device tree node while using
+	 * the legacy IOMMU bindins. So in the SMMU probing progress, we will
+	 * register the SMMU master only for legacy bindings.
+	 */
+	if (using_legacy_binding) {
+		i = 0;
+		while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
+							"#stream-id-cells", i,
+							&masterspec)) {
+			err = register_smmu_master(smmu, dev, &masterspec);
+			if (err) {
+				dev_err(dev, "failed to add master %s\n",
+					masterspec.np->name);
+				goto out_put_masters;
+			}
 
-		i++;
+			i++;
+		}
+		dev_notice(dev, "registered %d legacy master devices\n", i);
 	}
-	dev_notice(dev, "registered %d master devices\n", i);
 
 	parse_driver_options(smmu);
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Wei Chen]

The SMMU MasterIDs are placed at the master devices' DT node while
using the generic bindings. In this case, it's very hard for us to
register SMMU masters while probing SMMU as we had done for legacy
bindings. Because we have to go through whole device tree for all
SMMU devices to find their master devices.

It's better to register SMMU master for generic bindings in add_device
callback. This callback will only be called while constructing Dom0.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 144 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 143 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 895024c..25f2207 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2621,8 +2621,150 @@ static void arm_smmu_destroy_iommu_domain(struct iommu_domain *domain)
 	xfree(domain);
 }
 
+static int arm_smmu_add_generic_master_id(struct arm_smmu_device *smmu,
+				struct device *master_dev, u16 fwid)
+{
+	struct arm_smmu_master *master;
+	struct device_node *master_np = master_dev->of_node;
+
+	master = find_smmu_master(smmu, master_np);
+	if (!master) {
+		dev_notice(smmu->dev,
+			"This smmu master [%s] hasn't been registered, creating now!\n",
+			master_np->full_name);
+		master = devm_kzalloc(smmu->dev, sizeof(*master), GFP_KERNEL);
+		if (!master)
+			return -ENOMEM;
+
+		master->of_node = master_np;
+		master->cfg.num_streamids = 0;
+
+		/*
+		 * Xen: Let Xen know that the device is protected by a SMMU.
+		 * Only do while registering the master.
+		 */
+		dt_device_set_protected(master_np);
+	}
+
+	/*
+	 * If the smmu is using the stream index mode, check whether
+	 * the streamid exceeds the max allowed id,
+	 */
+	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
+		(fwid >= smmu->num_mapping_groups)) {
+		dev_err(smmu->dev,
+			"Stream ID for master device %s greater than maximum allowed (%d)\n",
+			master_np->name, smmu->num_mapping_groups);
+		return -ERANGE;
+	}
+
+	if (master->cfg.num_streamids >= MAX_MASTER_STREAMIDS) {
+		dev_err(smmu->dev,
+			"Reached maximum number (%d) of stream IDs for master device %s\n",
+			MAX_MASTER_STREAMIDS, master_np->name);
+		return -ENOSPC;
+	}
+
+	/*
+	 * If this is the first time we add id to this master,
+	 * we have to register this master to rb tree.
+	 */
+	if (!master->cfg.num_streamids) {
+		int ret;
+		ret = insert_smmu_master(smmu, master);
+		if ( ret && ret != -EEXIST ) {
+			dev_err(smmu->dev,
+				"Insert %s to smmu's master rb tree failed\n", master_np->name);
+			return ret;
+		}
+	}
+
+	master->cfg.streamids[master->cfg.num_streamids] = fwid;
+	master->cfg.num_streamids++;
+	dev_dbg(smmu->dev,
+		"Add new streamid [%d] to smmu [%s] for master [%s]!\n",
+		fwid, smmu->dev->of_node->name, master_np->name);
+
+	return 0;
+}
+
+static struct arm_smmu_device *find_smmu(const struct device *dev);
+
+static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+	struct arm_smmu_device *smmu;
+	u32 mask = 0, fwid = 0;
+
+	smmu = find_smmu(dt_to_dev(args->np));
+	if (!smmu) {
+		dev_err(dev, "Could not find smmu device!\n");
+		return -ENODEV;
+	}
+
+	if (args->args_count > 0)
+		fwid |= (u16)args->args[0];
+
+	if (args->args_count > 1)
+		fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
+	else if (!of_property_read_u32(args->np, "stream-match-mask", &mask))
+		fwid |= (u16)mask << SMR_MASK_SHIFT;
+
+	dev_dbg(dev, "%s fwid:%08x mask:%08x args_count:%d\n",
+			   args->np->full_name, fwid,
+			   mask, args->args_count);
+
+	return arm_smmu_add_generic_master_id(smmu, dev, (u16)fwid);
+}
+
+/*
+ * Parse "iommus" information from generic bindings of platfomr master
+ * device, and then xlate to master IDs and register to SMMU device.
+ */
+static int arm_smmu_platform_iommu_init(struct device *dev)
+{
+	struct of_phandle_args iommu_spec;
+	int idx = 0, ret;
+
+	/*
+	 * We don't currently walk up the tree looking for a parent IOMMU.
+	 * See the `Notes:' section of
+	 * Documentation/devicetree/bindings/iommu/iommu.txt
+	 */
+	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
+				"#iommu-cells",
+				idx, &iommu_spec)) {
+		ret = arm_smmu_of_xlate(dev, &iommu_spec);
+		if (ret) {
+			dev_err(dev,
+				"Do of_xlate for platform device failed, err=%d\n", ret);
+			return ret;
+		}
+
+		idx++;
+	}
+
+	/*
+	 * Return 0 if the device is not protected to follow the behavior
+	 * of PCI add device.
+	 */
+	return 0;
+}
+
 static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
 {
+	int ret;
+
+	/*
+	 * iommu_add_dt_device() is only called for the hardware domain.
+	 * If the SMMU is using generic bindings, we should parse and
+	 * register Master IDs while this function had been invoked.
+	 */
+	if (using_generic_binding) {
+		ret = arm_smmu_platform_iommu_init(dev);
+		if (ret)
+			return ret;
+	}
+
 	if (dt_device_is_protected(dev->of_node)) {
 		if (!dev->archdata.iommu) {
 			dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
@@ -2832,7 +2974,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
     .unmap_page = arm_smmu_unmap_page,
 };
 
-static __init const struct arm_smmu_device *find_smmu(const struct device *dev)
+static struct arm_smmu_device *find_smmu(const struct device *dev)
 {
 	struct arm_smmu_device *smmu;
 	bool found = false;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 7/7] xen: Fix a typo in error message of iommu_do_dt_domctl

[Wei Chen]

It's a error message about XEN_DOMCTL_deassign_device, but the
print message is XEN_DOMCTL_assign_device.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/drivers/passthrough/device_tree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index a8f403a..92adea6 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -209,8 +209,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
         ret = iommu_deassign_dt_device(d, dev);
 
         if ( ret )
-            printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign \"%s\""
-                   " to dom%u failed (%d)\n",
+            printk(XENLOG_G_ERR "XEN_DOMCTL_deassign_device: deassign \"%s\""
+                   " from dom%u failed (%d)\n",
                    dt_node_full_name(dev), d->domain_id, ret);
         break;
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 7/7] xen: Fix a typo in error message of iommu_do_dt_domctl

[Stefano Stabellini]

On Fri, 30 Jun 2017, Wei Chen wrote:
> It's a error message about XEN_DOMCTL_deassign_device, but the
> print message is XEN_DOMCTL_assign_device.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/drivers/passthrough/device_tree.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index a8f403a..92adea6 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -209,8 +209,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>          ret = iommu_deassign_dt_device(d, dev);
>  
>          if ( ret )
> -            printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign \"%s\""
> -                   " to dom%u failed (%d)\n",
> +            printk(XENLOG_G_ERR "XEN_DOMCTL_deassign_device: deassign \"%s\""
> +                   " from dom%u failed (%d)\n",
>                     dt_node_full_name(dev), d->domain_id, ret);
>          break;
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/7] xen/arm: SMMU: Implement the add_device callback in SMMU

[Stefano Stabellini]

On Fri, 30 Jun 2017, Wei Chen wrote:
> This add_device callback function is taking care of adding a device
> to SMMU and make sure it is fully prepare to be used by the SMMU
> afterwards.
> 
> In previous code, we don't implement the add_device callback in
> iommu_ops for ARM SMMU. We placed the work of add_device to
> assign_device callback. The function assign_device should not care
> about adding the device to an iommu_group. It might not even be
> able to decide how to do that. In this patch, we move this work
> back to add_device callback.
> 
> This add_device callback is only called while we are handling all
> devices for constructing the Domain0.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 74c09b0..2efa52d 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2591,6 +2591,26 @@ static void arm_smmu_destroy_iommu_domain(struct iommu_domain *domain)
>  	xfree(domain);
>  }
>  
> +static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> +{
> +	if (dt_device_is_protected(dev->of_node)) {
> +		if (!dev->archdata.iommu) {
> +			dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> +			if (!dev->archdata.iommu)
> +				return -ENOMEM;
> +		}
> +
> +		if (!dev_iommu_group(dev))
> +			return arm_smmu_add_device(dev);
> +	}
> +
> +	/*
> +	 * Return 0 if the device is not protected to follow the behavior
> +	 * of PCI add device.

What does this comment mean?


> +	 */
> +	return 0;
> +}
> +
>  static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
>  			       struct device *dev, u32 flag)
>  {
> @@ -2600,17 +2620,8 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
>  
>  	xen_domain = dom_iommu(d)->arch.priv;
>  
> -	if (!dev->archdata.iommu) {
> -		dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> -		if (!dev->archdata.iommu)
> -			return -ENOMEM;
> -	}
> -
> -	if (!dev_iommu_group(dev)) {
> -		ret = arm_smmu_add_device(dev);
> -		if (ret)
> -			return ret;
> -	}
> +	if (!dev_iommu_group(dev))
> +	    return -ENODEV;
>  
>  	spin_lock(&xen_domain->lock);
>  
> @@ -2784,6 +2795,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
>      .teardown = arm_smmu_iommu_domain_teardown,
>      .iotlb_flush = arm_smmu_iotlb_flush,
>      .iotlb_flush_all = arm_smmu_iotlb_flush_all,
> +    .add_device = arm_smmu_xen_add_device,
>      .assign_device = arm_smmu_assign_dev,
>      .reassign_device = arm_smmu_reassign_dev,
>      .map_page = arm_smmu_map_page,
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/7] xen/arm: SMMU: Introduce a helper to add DT device to SMMU

[Stefano Stabellini]

On Fri, 30 Jun 2017, Wei Chen wrote:
> In current code, we only have the iommu_add_device to add PCI device
> to IOMMU. But for ARM SMMU, we don't have a separate helper to add
> platform device with device tree to SMMU. This work was included in
> the iommu_assign_dt_device. But sometimes, we just want to add device
> to SMMU to do some preparation for further use. In this case, we can't
> call iommu_assign_dt_device.
> 
> In previous patch, we have implement the add_device callback for SMMU,
> so we can separate this work from assign_device now.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>  xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++++++
>  xen/include/xen/iommu.h               |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 99ed49e..a8f403a 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -24,6 +24,26 @@
>  
>  static spinlock_t dtdevs_lock = SPIN_LOCK_UNLOCKED;
>  
> +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev)
> +{
> +    int rc;
> +
> +    struct domain_iommu *hd = dom_iommu(d);
> +
> +    if ( !iommu_enabled || !hd->platform_ops ||
> +         !hd->platform_ops->add_device )
> +        return 0;

Shouldn't we also have:

  if ( !dt_device_is_protected(dev) )
        return 0;

?


> +    spin_lock(&dtdevs_lock);
> +
> +    /* The devfn field doesn't matter to DT device. */
> +    rc = hd->platform_ops->add_device(0, dt_to_dev(dev));
> +
> +    spin_unlock(&dtdevs_lock);
> +
> +    return rc;
> +}
> +
>  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>  {
>      int rc = -EBUSY;
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 5803e3f..ec03faa 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -132,6 +132,7 @@ void iommu_read_msi_from_ire(struct msi_desc *msi_desc, struct msi_msg *msg);
>  #ifdef CONFIG_HAS_DEVICE_TREE
>  #include <xen/device_tree.h>
>  
> +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev);
>  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>  int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
>  int iommu_dt_domain_init(struct domain *d);
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/7] xen/arm: Prepare SMMU resources for protected devices

[Stefano Stabellini]

On Fri, 30 Jun 2017, Wei Chen wrote:
> In previous code, while we are constructing Dom0, we will assign
> all devices except passthrough devices to Dom0. In the later, when
> we start the DomU, the assign_device will prepare SMMU resources
> for the devices passthrough to DomU. This is ok when we kept the
> add_device code in assign_device. But currently, we have separated
> add_device from assign_device. If we don't prepare SMMU resources
> for passthrough devices, these devices would not work properly in
> DomU.
> 
> So, while we are handling all devices DT node in construction Dom0,
> we will call add_device to prepare SMMU resources for all protected
> devices, regardless of passthrough or not.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/domain_build.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c6776d7..6aea427 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1082,6 +1082,18 @@ static int handle_device(struct domain *d, struct dt_device_node *dev,
>      dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n",
>                 dt_node_full_name(dev), need_mapping, nirq, naddr);
>  
> +    /*
> +     * If this device is behind the SMMU, the add_device callback will
> +     * prepare resource for it. Otherwise, add_device has no effect.
> +     */
> +    res = iommu_add_dt_device(d, dev);
> +    if ( res )
> +    {
> +        printk(XENLOG_ERR "Failed to add device to IOMMU for %s\n",
> +               dt_node_full_name(dev));
> +        return res;
> +    }
> +
>      if ( dt_device_is_protected(dev) && need_mapping )
>      {
>          dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/7] xen/arm: SMMU: Detect types of device tree binding

[Stefano Stabellini]

On Fri, 30 Jun 2017, Wei Chen wrote:
> The device tree provides two types of IOMMU bindings, one is legacy
> another is generic. The legacy bindings will be depercated in favour
                                                  ^ deprecated

> of the generic bindings. But in the transitional period, we have to
> support both of them.
> 
> The codes to handle these two types of bindings are very differnet,
      ^ code                                               ^ different

Please use a spellchecker.

> so we have to detect the binding types while doing SMMU probing.
> 
> This detect code is based on Linux ARM SMMUv2 driver:
> https://github.com/torvalds/linux/blob/master/drivers/iommu/arm-smmu.c
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 2efa52d..441c296 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -143,6 +143,8 @@ typedef enum irqreturn irqreturn_t;
>  
>  #define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
>  
> +#define pr_notice(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
> +
>  /* Alias to Xen allocation helpers */
>  #define kfree xfree
>  #define kmalloc(size, flags)		_xmalloc(size, sizeof(void *))
> @@ -681,6 +683,8 @@ struct arm_smmu_option_prop {
>  	const char *prop;
>  };
>  
> +static bool using_legacy_binding, using_generic_binding;

__initdata?

But why do these two variables need to be static? Can't they just be
local variables in arm_smmu_device_dt_probe?

Is it to enforce that all smmus on a given platform are either using the
legacy or the generic bindings, but not a mix of the two? If so, it
should be clearly written. Also, I am not sure we should really be
checking for that. It seems to me that one smmu could be using generic
bindings and another could be using legacy bindings. Is it specified
anywhere that it cannot be the case?


>  static struct arm_smmu_option_prop arm_smmu_options[] = {
>  	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
>  	{ 0, NULL},
> @@ -2289,6 +2293,25 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  	struct rb_node *node;
>  	struct of_phandle_args masterspec;
>  	int num_irqs, i, err;
> +	bool legacy_binding;
> +
> +	/*
> +	 * Xen: Do the same check as Linux. Checking the SMMU device tree bindings
            ^ do                        ^ Check that


> +	 * are either using generic or legacy one.
                                          ^ bindings

> +	 *
> +	 * The "mmu-masters" property is only existed in legacy bindings.
                                  ^ only exists in the legacy bindings

> +	 */
> +	legacy_binding = dt_find_property(dev->of_node, "mmu-masters", NULL);
> +	if (legacy_binding && !using_generic_binding) {
> +		if (!using_legacy_binding)
> +			pr_notice("deprecated \"mmu-masters\" DT property in use\n");
> +		using_legacy_binding = true;
> +	} else if (!legacy_binding && !using_legacy_binding) {
> +		using_generic_binding = true;

Please simplify this series of if/else.


> +	} else {
> +		dev_err(dev, "not probing due to mismatched DT properties\n");
> +		return -ENODEV;
> +	}




>  	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
>  	if (!smmu) {
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/7] xen/arm: SMMU: Keep registering legacy master in SMMU probe

[Stefano Stabellini]

On Fri, 30 Jun 2017, Wei Chen wrote:
> The legacy IOMMU bindings place the SMMU MasterIDs in the SMMU device
> tree node. In current code, we register the SMMU masters while probing
> SMMU. It's better to keep registering legacy masters in the SMMU probing
> progress. If we move registering legacy SMMU masters to add_device or
> assign_device, we have to go through all SMMUs to find correct SMMU for
> master device. It's inefficient and doesn't bring any enhancement.
> 
> Similarly, if we want to register generic masters in SMMU probing, we
> have to go through whole device tree to find master devices for all
> SMMUs. So we only keep registering legacy master in SMMU probing.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/drivers/passthrough/arm/smmu.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 441c296..895024c 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2375,21 +2375,28 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> -	i = 0;
>  	smmu->masters = RB_ROOT;
> -	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
> -					   "#stream-id-cells", i,
> -					   &masterspec)) {
> -		err = register_smmu_master(smmu, dev, &masterspec);
> -		if (err) {
> -			dev_err(dev, "failed to add master %s\n",
> -				masterspec.np->name);
> -			goto out_put_masters;
> -		}
> +	/*
> +	 * The SMMU MasterIDs are listed in SMMU device tree node while using
> +	 * the legacy IOMMU bindins. So in the SMMU probing progress, we will
> +	 * register the SMMU master only for legacy bindings.
> +	 */
> +	if (using_legacy_binding) {
> +		i = 0;
> +		while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
> +							"#stream-id-cells", i,
> +							&masterspec)) {
> +			err = register_smmu_master(smmu, dev, &masterspec);
> +			if (err) {
> +				dev_err(dev, "failed to add master %s\n",
> +					masterspec.np->name);
> +				goto out_put_masters;
> +			}
>  
> -		i++;
> +			i++;
> +		}
> +		dev_notice(dev, "registered %d legacy master devices\n", i);
>  	}
> -	dev_notice(dev, "registered %d master devices\n", i);
>  
>  	parse_driver_options(smmu);
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Stefano Stabellini]

On Fri, 30 Jun 2017, Wei Chen wrote:
> The SMMU MasterIDs are placed at the master devices' DT node while
> using the generic bindings. In this case, it's very hard for us to
> register SMMU masters while probing SMMU as we had done for legacy
> bindings. Because we have to go through whole device tree for all
> SMMU devices to find their master devices.
> 
> It's better to register SMMU master for generic bindings in add_device
> callback. This callback will only be called while constructing Dom0.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 144 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 895024c..25f2207 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2621,8 +2621,150 @@ static void arm_smmu_destroy_iommu_domain(struct iommu_domain *domain)
>  	xfree(domain);
>  }
>  
> +static int arm_smmu_add_generic_master_id(struct arm_smmu_device *smmu,
> +				struct device *master_dev, u16 fwid)
> +{
> +	struct arm_smmu_master *master;
> +	struct device_node *master_np = master_dev->of_node;
> +
> +	master = find_smmu_master(smmu, master_np);
> +	if (!master) {
> +		dev_notice(smmu->dev,
> +			"This smmu master [%s] hasn't been registered, creating now!\n",
> +			master_np->full_name);
> +		master = devm_kzalloc(smmu->dev, sizeof(*master), GFP_KERNEL);
> +		if (!master)
> +			return -ENOMEM;
> +
> +		master->of_node = master_np;
> +		master->cfg.num_streamids = 0;
> +
> +		/*
> +		 * Xen: Let Xen know that the device is protected by a SMMU.
> +		 * Only do while registering the master.
> +		 */
> +		dt_device_set_protected(master_np);
> +	}
> +
> +	/*
> +	 * If the smmu is using the stream index mode, check whether
> +	 * the streamid exceeds the max allowed id,
> +	 */
> +	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
> +		(fwid >= smmu->num_mapping_groups)) {
> +		dev_err(smmu->dev,
> +			"Stream ID for master device %s greater than maximum allowed (%d)\n",
> +			master_np->name, smmu->num_mapping_groups);
> +		return -ERANGE;
> +	}
> +
> +	if (master->cfg.num_streamids >= MAX_MASTER_STREAMIDS) {
> +		dev_err(smmu->dev,
> +			"Reached maximum number (%d) of stream IDs for master device %s\n",
> +			MAX_MASTER_STREAMIDS, master_np->name);
> +		return -ENOSPC;
> +	}
> +
> +	/*
> +	 * If this is the first time we add id to this master,
> +	 * we have to register this master to rb tree.
> +	 */
> +	if (!master->cfg.num_streamids) {
> +		int ret;
> +		ret = insert_smmu_master(smmu, master);
> +		if ( ret && ret != -EEXIST ) {
> +			dev_err(smmu->dev,
> +				"Insert %s to smmu's master rb tree failed\n", master_np->name);
> +			return ret;
> +		}
> +	}
> +
> +	master->cfg.streamids[master->cfg.num_streamids] = fwid;
> +	master->cfg.num_streamids++;
> +	dev_dbg(smmu->dev,
> +		"Add new streamid [%d] to smmu [%s] for master [%s]!\n",
> +		fwid, smmu->dev->of_node->name, master_np->name);
> +
> +	return 0;
> +}
> +
> +static struct arm_smmu_device *find_smmu(const struct device *dev);
> +
> +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> +{
> +	struct arm_smmu_device *smmu;
> +	u32 mask = 0, fwid = 0;
> +
> +	smmu = find_smmu(dt_to_dev(args->np));
> +	if (!smmu) {
> +		dev_err(dev, "Could not find smmu device!\n");
> +		return -ENODEV;
> +	}
> +
> +	if (args->args_count > 0)
> +		fwid |= (u16)args->args[0];
> +
> +	if (args->args_count > 1)
> +		fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
> +	else if (!of_property_read_u32(args->np, "stream-match-mask", &mask))
> +		fwid |= (u16)mask << SMR_MASK_SHIFT;
> +	dev_dbg(dev, "%s fwid:%08x mask:%08x args_count:%d\n",
> +			   args->np->full_name, fwid,
> +			   mask, args->args_count);

I don't understand why fwid is declared as u32 but used as a u16 below.
Shouldn't it be declared as u16 in the first place?


> +	return arm_smmu_add_generic_master_id(smmu, dev, (u16)fwid);
> +}
> +
> +/*
> + * Parse "iommus" information from generic bindings of platfomr master
> + * device, and then xlate to master IDs and register to SMMU device.
> + */
> +static int arm_smmu_platform_iommu_init(struct device *dev)
> +{
> +	struct of_phandle_args iommu_spec;
> +	int idx = 0, ret;
> +
> +	/*
> +	 * We don't currently walk up the tree looking for a parent IOMMU.
> +	 * See the `Notes:' section of
> +	 * Documentation/devicetree/bindings/iommu/iommu.txt
> +	 */
> +	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> +				"#iommu-cells",
> +				idx, &iommu_spec)) {
> +		ret = arm_smmu_of_xlate(dev, &iommu_spec);
> +		if (ret) {
> +			dev_err(dev,
> +				"Do of_xlate for platform device failed, err=%d\n", ret);
                 ^ remove Do

> +			return ret;
> +		}
> +
> +		idx++;
> +	}
> +
> +	/*
> +	 * Return 0 if the device is not protected to follow the behavior
> +	 * of PCI add device.
> +	 */
> +	return 0;
> +}
> +
>  static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
>  {
> +	int ret;
> +
> +	/*
> +	 * iommu_add_dt_device() is only called for the hardware domain.
> +	 * If the SMMU is using generic bindings, we should parse and
> +	 * register Master IDs while this function had been invoked.
> +	 */
> +	if (using_generic_binding) {
> +		ret = arm_smmu_platform_iommu_init(dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (dt_device_is_protected(dev->of_node)) {
>  		if (!dev->archdata.iommu) {
>  			dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> @@ -2832,7 +2974,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
>      .unmap_page = arm_smmu_unmap_page,
>  };
>  
> -static __init const struct arm_smmu_device *find_smmu(const struct device *dev)
> +static struct arm_smmu_device *find_smmu(const struct device *dev)
>  {
>  	struct arm_smmu_device *smmu;
>  	bool found = false;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/7] xen/arm: SMMU: Implement the add_device callback in SMMU

[Wei Chen]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月4日 5:58
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> <Julien.Grall@arm.com>; nd <nd@arm.com>
> Subject: Re: [Xen-devel] [PATCH 1/7] xen/arm: SMMU: Implement the add_device
> callback in SMMU
> 
> On Fri, 30 Jun 2017, Wei Chen wrote:
> > This add_device callback function is taking care of adding a device
> > to SMMU and make sure it is fully prepare to be used by the SMMU
> > afterwards.
> >
> > In previous code, we don't implement the add_device callback in
> > iommu_ops for ARM SMMU. We placed the work of add_device to
> > assign_device callback. The function assign_device should not care
> > about adding the device to an iommu_group. It might not even be
> > able to decide how to do that. In this patch, we move this work
> > back to add_device callback.
> >
> > This add_device callback is only called while we are handling all
> > devices for constructing the Domain0.
> >
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > ---
> >  xen/drivers/passthrough/arm/smmu.c | 34 +++++++++++++++++++++++-----------
> >  1 file changed, 23 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> b/xen/drivers/passthrough/arm/smmu.c
> > index 74c09b0..2efa52d 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -2591,6 +2591,26 @@ static void arm_smmu_destroy_iommu_domain(struct
> iommu_domain *domain)
> >  	xfree(domain);
> >  }
> >
> > +static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> > +{
> > +	if (dt_device_is_protected(dev->of_node)) {
> > +		if (!dev->archdata.iommu) {
> > +			dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> > +			if (!dev->archdata.iommu)
> > +				return -ENOMEM;
> > +		}
> > +
> > +		if (!dev_iommu_group(dev))
> > +			return arm_smmu_add_device(dev);
> > +	}
> > +
> > +	/*
> > +	 * Return 0 if the device is not protected to follow the behavior
> > +	 * of PCI add device.
> 
> What does this comment mean?
> 

While I was looking at the iommu_add_device which is used by PCI counterpart.
I found it will always return 0 even for device that are not protected by an IOMMU.
I would much prefer to keep platform devices have similar behavior as PCI devices.

So while I was implementing iommu_add_dt_device and arm_smmu_xen_add_device, I
returned 0 if the device is not protected by IOMMU.

> 
> > +	 */
> > +	return 0;
> > +}
> > +
> >  static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
> >  			       struct device *dev, u32 flag)
> >  {
> > @@ -2600,17 +2620,8 @@ static int arm_smmu_assign_dev(struct domain *d, u8
> devfn,
> >
> >  	xen_domain = dom_iommu(d)->arch.priv;
> >
> > -	if (!dev->archdata.iommu) {
> > -		dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> > -		if (!dev->archdata.iommu)
> > -			return -ENOMEM;
> > -	}
> > -
> > -	if (!dev_iommu_group(dev)) {
> > -		ret = arm_smmu_add_device(dev);
> > -		if (ret)
> > -			return ret;
> > -	}
> > +	if (!dev_iommu_group(dev))
> > +	    return -ENODEV;
> >
> >  	spin_lock(&xen_domain->lock);
> >
> > @@ -2784,6 +2795,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
> >      .teardown = arm_smmu_iommu_domain_teardown,
> >      .iotlb_flush = arm_smmu_iotlb_flush,
> >      .iotlb_flush_all = arm_smmu_iotlb_flush_all,
> > +    .add_device = arm_smmu_xen_add_device,
> >      .assign_device = arm_smmu_assign_dev,
> >      .reassign_device = arm_smmu_reassign_dev,
> >      .map_page = arm_smmu_map_page,
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/7] xen/arm: SMMU: Introduce a helper to add DT device to SMMU

[Wei Chen]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月4日 6:03
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> <Julien.Grall@arm.com>; nd <nd@arm.com>
> Subject: Re: [Xen-devel] [PATCH 2/7] xen/arm: SMMU: Introduce a helper to add
> DT device to SMMU
> 
> On Fri, 30 Jun 2017, Wei Chen wrote:
> > In current code, we only have the iommu_add_device to add PCI device
> > to IOMMU. But for ARM SMMU, we don't have a separate helper to add
> > platform device with device tree to SMMU. This work was included in
> > the iommu_assign_dt_device. But sometimes, we just want to add device
> > to SMMU to do some preparation for further use. In this case, we can't
> > call iommu_assign_dt_device.
> >
> > In previous patch, we have implement the add_device callback for SMMU,
> > so we can separate this work from assign_device now.
> >
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > ---
> >  xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++++++
> >  xen/include/xen/iommu.h               |  1 +
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/xen/drivers/passthrough/device_tree.c
> b/xen/drivers/passthrough/device_tree.c
> > index 99ed49e..a8f403a 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -24,6 +24,26 @@
> >
> >  static spinlock_t dtdevs_lock = SPIN_LOCK_UNLOCKED;
> >
> > +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev)
> > +{
> > +    int rc;
> > +
> > +    struct domain_iommu *hd = dom_iommu(d);
> > +
> > +    if ( !iommu_enabled || !hd->platform_ops ||
> > +         !hd->platform_ops->add_device )
> > +        return 0;
> 
> Shouldn't we also have:
> 
>   if ( !dt_device_is_protected(dev) )
>         return 0;
> 
> ?
> 

When we're using the legacy binding, the master IDs will be registered to SMMU and
the protected flag of relevant master devices will be set to true (dt_device_is_protected
will return true). But for generic IOMMU bindings, before we call ops->add_device,
we didn't register the master device's master id to SMMU and hadn't set the protected
flag, The dt_device_is_protected will always return false.

In this case, we can't call dt_device_is_protected(dev) here.

> 
> > +    spin_lock(&dtdevs_lock);
> > +
> > +    /* The devfn field doesn't matter to DT device. */
> > +    rc = hd->platform_ops->add_device(0, dt_to_dev(dev));
> > +
> > +    spin_unlock(&dtdevs_lock);
> > +
> > +    return rc;
> > +}
> > +
> >  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
> >  {
> >      int rc = -EBUSY;
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index 5803e3f..ec03faa 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -132,6 +132,7 @@ void iommu_read_msi_from_ire(struct msi_desc *msi_desc,
> struct msi_msg *msg);
> >  #ifdef CONFIG_HAS_DEVICE_TREE
> >  #include <xen/device_tree.h>
> >
> > +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev);
> >  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
> >  int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
> >  int iommu_dt_domain_init(struct domain *d);
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/7] xen/arm: SMMU: Detect types of device tree binding

[Wei Chen]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月4日 6:30
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> <Julien.Grall@arm.com>; nd <nd@arm.com>
> Subject: Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device
> tree binding
> 
> On Fri, 30 Jun 2017, Wei Chen wrote:
> > The device tree provides two types of IOMMU bindings, one is legacy
> > another is generic. The legacy bindings will be depercated in favour
>                                                   ^ deprecated
> 
> > of the generic bindings. But in the transitional period, we have to
> > support both of them.
> >
> > The codes to handle these two types of bindings are very differnet,
>       ^ code                                               ^ different
> 
> Please use a spellchecker.

Thanks, I will fix them.

> 
> > so we have to detect the binding types while doing SMMU probing.
> >
> > This detect code is based on Linux ARM SMMUv2 driver:
> > https://github.com/torvalds/linux/blob/master/drivers/iommu/arm-smmu.c
> >
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > ---
> >  xen/drivers/passthrough/arm/smmu.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> b/xen/drivers/passthrough/arm/smmu.c
> > index 2efa52d..441c296 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -143,6 +143,8 @@ typedef enum irqreturn irqreturn_t;
> >
> >  #define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
> >
> > +#define pr_notice(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
> > +
> >  /* Alias to Xen allocation helpers */
> >  #define kfree xfree
> >  #define kmalloc(size, flags)		_xmalloc(size, sizeof(void *))
> > @@ -681,6 +683,8 @@ struct arm_smmu_option_prop {
> >  	const char *prop;
> >  };
> >
> > +static bool using_legacy_binding, using_generic_binding;
> 
> __initdata?
> 

I think these two variables are not only used in initialization. They also
have been used in ops->add_device. Althrough the add_device is only be
invoked while construct_dom0.

> But why do these two variables need to be static? Can't they just be
> local variables in arm_smmu_device_dt_probe?
> 
> Is it to enforce that all smmus on a given platform are either using the
> legacy or the generic bindings, but not a mix of the two? If so, it
> should be clearly written. Also, I am not sure we should really be

Yes, this checking will enforce all smmus are using the same bindings.

> checking for that. It seems to me that one smmu could be using generic
> bindings and another could be using legacy bindings. Is it specified
> anywhere that it cannot be the case?
> 

In theory, different SMMUs can use different bindings. About this concern,
I have discussed with Robin Murphy and Julien. We have three reasons:

The first is that, we ported this checking from Linux, we are trying to keep
the code very close to the Linux driver. To ease backporting changes.

The second is that, we think it is a good change to try to phase out the 
legacy binding and request to use generic bindings everywhere if you 
start to use in one SMMU.
 
The other less technical reason for not supporting both at once is that anyone
who can update their DT to add or update SMMUs with the new binding has no good
excuse for not updating the whole lot. It's the likes of Seattle and ThunderX
boxes with firmware that won't get updated for which we have to preserve "mmu-masters"
support.

> 
> >  static struct arm_smmu_option_prop arm_smmu_options[] = {
> >  	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
> >  	{ 0, NULL},
> > @@ -2289,6 +2293,25 @@ static int arm_smmu_device_dt_probe(struct
> platform_device *pdev)
> >  	struct rb_node *node;
> >  	struct of_phandle_args masterspec;
> >  	int num_irqs, i, err;
> > +	bool legacy_binding;
> > +
> > +	/*
> > +	 * Xen: Do the same check as Linux. Checking the SMMU device tree
> bindings
>             ^ do                        ^ Check that
> 
> 
> > +	 * are either using generic or legacy one.
>                                           ^ bindings
> 
> > +	 *
> > +	 * The "mmu-masters" property is only existed in legacy bindings.
>                                   ^ only exists in the legacy bindings
> 

Thanks, I will fix above typos.

> > +	 */
> > +	legacy_binding = dt_find_property(dev->of_node, "mmu-masters", NULL);
> > +	if (legacy_binding && !using_generic_binding) {
> > +		if (!using_legacy_binding)
> > +			pr_notice("deprecated \"mmu-masters\" DT property in use\n");
> > +		using_legacy_binding = true;
> > +	} else if (!legacy_binding && !using_legacy_binding) {
> > +		using_generic_binding = true;
> 
> Please simplify this series of if/else.
> 

This code is the same as Linux SMMU driver. If we agree on enforcing all smmus
are using the same binding, I prefer to keep the code the same.

> 
> > +	} else { 
> > +		dev_err(dev, "not probing due to mismatched DT properties\n");
> > +		return -ENODEV;
> > +	}
> 
> 
> 
> 
> >  	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> >  	if (!smmu) {
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Wei Chen]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月4日 7:00
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> <Julien.Grall@arm.com>; nd <nd@arm.com>
> Subject: Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU
> bindings
> 
> On Fri, 30 Jun 2017, Wei Chen wrote:
> > The SMMU MasterIDs are placed at the master devices' DT node while
> > using the generic bindings. In this case, it's very hard for us to
> > register SMMU masters while probing SMMU as we had done for legacy
> > bindings. Because we have to go through whole device tree for all
> > SMMU devices to find their master devices.
> >
> > It's better to register SMMU master for generic bindings in add_device
> > callback. This callback will only be called while constructing Dom0.
> >
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > ---
> >  xen/drivers/passthrough/arm/smmu.c | 144
> ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 143 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> b/xen/drivers/passthrough/arm/smmu.c
> > index 895024c..25f2207 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -2621,8 +2621,150 @@ static void arm_smmu_destroy_iommu_domain(struct
> iommu_domain *domain)
> >  	xfree(domain);
> >  }
> >
> > +static int arm_smmu_add_generic_master_id(struct arm_smmu_device *smmu,
> > +				struct device *master_dev, u16 fwid)
> > +{
> > +	struct arm_smmu_master *master;
> > +	struct device_node *master_np = master_dev->of_node;
> > +
> > +	master = find_smmu_master(smmu, master_np);
> > +	if (!master) {
> > +		dev_notice(smmu->dev,
> > +			"This smmu master [%s] hasn't been registered, creating
> now!\n",
> > +			master_np->full_name);
> > +		master = devm_kzalloc(smmu->dev, sizeof(*master), GFP_KERNEL);
> > +		if (!master)
> > +			return -ENOMEM;
> > +
> > +		master->of_node = master_np;
> > +		master->cfg.num_streamids = 0;
> > +
> > +		/*
> > +		 * Xen: Let Xen know that the device is protected by a SMMU.
> > +		 * Only do while registering the master.
> > +		 */
> > +		dt_device_set_protected(master_np);
> > +	}
> > +
> > +	/*
> > +	 * If the smmu is using the stream index mode, check whether
> > +	 * the streamid exceeds the max allowed id,
> > +	 */
> > +	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
> > +		(fwid >= smmu->num_mapping_groups)) {
> > +		dev_err(smmu->dev,
> > +			"Stream ID for master device %s greater than maximum allowed
> (%d)\n",
> > +			master_np->name, smmu->num_mapping_groups);
> > +		return -ERANGE;
> > +	}
> > +
> > +	if (master->cfg.num_streamids >= MAX_MASTER_STREAMIDS) {
> > +		dev_err(smmu->dev,
> > +			"Reached maximum number (%d) of stream IDs for master
> device %s\n",
> > +			MAX_MASTER_STREAMIDS, master_np->name);
> > +		return -ENOSPC;
> > +	}
> > +
> > +	/*
> > +	 * If this is the first time we add id to this master,
> > +	 * we have to register this master to rb tree.
> > +	 */
> > +	if (!master->cfg.num_streamids) {
> > +		int ret;
> > +		ret = insert_smmu_master(smmu, master);
> > +		if ( ret && ret != -EEXIST ) {
> > +			dev_err(smmu->dev,
> > +				"Insert %s to smmu's master rb tree failed\n",
> master_np->name);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	master->cfg.streamids[master->cfg.num_streamids] = fwid;
> > +	master->cfg.num_streamids++;
> > +	dev_dbg(smmu->dev,
> > +		"Add new streamid [%d] to smmu [%s] for master [%s]!\n",
> > +		fwid, smmu->dev->of_node->name, master_np->name);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct arm_smmu_device *find_smmu(const struct device *dev);
> > +
> > +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args
> *args)
> > +{
> > +	struct arm_smmu_device *smmu;
> > +	u32 mask = 0, fwid = 0;
> > +
> > +	smmu = find_smmu(dt_to_dev(args->np));
> > +	if (!smmu) {
> > +		dev_err(dev, "Could not find smmu device!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (args->args_count > 0)
> > +		fwid |= (u16)args->args[0];
> > +
> > +	if (args->args_count > 1)
> > +		fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
> > +	else if (!of_property_read_u32(args->np, "stream-match-mask", &mask))
> > +		fwid |= (u16)mask << SMR_MASK_SHIFT;
> > +	dev_dbg(dev, "%s fwid:%08x mask:%08x args_count:%d\n",
> > +			   args->np->full_name, fwid,
> > +			   mask, args->args_count);
> 
> I don't understand why fwid is declared as u32 but used as a u16 below.
> Shouldn't it be declared as u16 in the first place?
> 

Oh, it's my mistake. In Linux, the fwid will be passed to iommu_fwspec_add_ids,
it requires u32 parameter. But after I ported this code to Xen, we passed
the fwid to arm_smmu_add_generic_master_id, a u16 parameter is enough.

I will fix it.

> 
> > +	return arm_smmu_add_generic_master_id(smmu, dev, (u16)fwid);
> > +}
> > +
> > +/*
> > + * Parse "iommus" information from generic bindings of platfomr master
> > + * device, and then xlate to master IDs and register to SMMU device.
> > + */
> > +static int arm_smmu_platform_iommu_init(struct device *dev)
> > +{
> > +	struct of_phandle_args iommu_spec;
> > +	int idx = 0, ret;
> > +
> > +	/*
> > +	 * We don't currently walk up the tree looking for a parent IOMMU.
> > +	 * See the `Notes:' section of
> > +	 * Documentation/devicetree/bindings/iommu/iommu.txt
> > +	 */
> > +	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> > +				"#iommu-cells",
> > +				idx, &iommu_spec)) {
> > +		ret = arm_smmu_of_xlate(dev, &iommu_spec);
> > +		if (ret) {
> > +			dev_err(dev,
> > +				"Do of_xlate for platform device failed, err=%d\n",
> ret);
>                  ^ remove Do
> 

Ok.

> > +			return ret;
> > +		}
> > +
> > +		idx++;
> > +	}
> > +
> > +	/*
> > +	 * Return 0 if the device is not protected to follow the behavior
> > +	 * of PCI add device.
> > +	 */
> > +	return 0;
> > +}
> > +
> >  static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> >  {
> > +	int ret;
> > +
> > +	/*
> > +	 * iommu_add_dt_device() is only called for the hardware domain.
> > +	 * If the SMMU is using generic bindings, we should parse and
> > +	 * register Master IDs while this function had been invoked.
> > +	 */
> > +	if (using_generic_binding) {
> > +		ret = arm_smmu_platform_iommu_init(dev);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	if (dt_device_is_protected(dev->of_node)) {
> >  		if (!dev->archdata.iommu) {
> >  			dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> > @@ -2832,7 +2974,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
> >      .unmap_page = arm_smmu_unmap_page,
> >  };
> >
> > -static __init const struct arm_smmu_device *find_smmu(const struct device
> *dev)
> > +static struct arm_smmu_device *find_smmu(const struct device *dev)
> >  {
> >  	struct arm_smmu_device *smmu;
> >  	bool found = false;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Julien Grall]

Hi,

On 07/04/2017 07:27 AM, Wei Chen wrote:
> Hi Stefano,
> 
>> -----Original Message-----
>> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
>> Sent: 2017年7月4日 7:00
>> To: Wei Chen <Wei.Chen@arm.com>
>> Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
>> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
>> <Julien.Grall@arm.com>; nd <nd@arm.com>
>> Subject: Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU
>> bindings
>> 
>> On Fri, 30 Jun 2017, Wei Chen wrote:
>> > The SMMU MasterIDs are placed at the master devices' DT node while
>> > using the generic bindings. In this case, it's very hard for us to
>> > register SMMU masters while probing SMMU as we had done for legacy
>> > bindings. Because we have to go through whole device tree for all
>> > SMMU devices to find their master devices.
>> >
>> > It's better to register SMMU master for generic bindings in add_device
>> > callback. This callback will only be called while constructing Dom0.
>> >
>> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>> > ---
>> >  xen/drivers/passthrough/arm/smmu.c | 144
>> ++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 143 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/xen/drivers/passthrough/arm/smmu.c
>> b/xen/drivers/passthrough/arm/smmu.c
>> > index 895024c..25f2207 100644
>> > --- a/xen/drivers/passthrough/arm/smmu.c
>> > +++ b/xen/drivers/passthrough/arm/smmu.c
>> > @@ -2621,8 +2621,150 @@ static void arm_smmu_destroy_iommu_domain(struct
>> iommu_domain *domain)
>> >      xfree(domain);
>> >  }
>> >
>> > +static int arm_smmu_add_generic_master_id(struct arm_smmu_device *smmu,
>> > +                           struct device *master_dev, u16 fwid)
>> > +{
>> > +   struct arm_smmu_master *master;
>> > +   struct device_node *master_np = master_dev->of_node;
>> > +
>> > +   master = find_smmu_master(smmu, master_np);
>> > +   if (!master) {
>> > +           dev_notice(smmu->dev,
>> > +                   "This smmu master [%s] hasn't been registered, creating
>> now!\n",
>> > +                   master_np->full_name);
>> > +           master = devm_kzalloc(smmu->dev, sizeof(*master), GFP_KERNEL);
>> > +           if (!master)
>> > +                   return -ENOMEM;
>> > +
>> > +           master->of_node = master_np;
>> > +           master->cfg.num_streamids = 0;
>> > +
>> > +           /*
>> > +            * Xen: Let Xen know that the device is protected by a SMMU.
>> > +            * Only do while registering the master.
>> > +            */
>> > +           dt_device_set_protected(master_np);
>> > +   }
>> > +
>> > +   /*
>> > +    * If the smmu is using the stream index mode, check whether
>> > +    * the streamid exceeds the max allowed id,
>> > +    */
>> > +   if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
>> > +           (fwid >= smmu->num_mapping_groups)) {
>> > +           dev_err(smmu->dev,
>> > +                   "Stream ID for master device %s greater than maximum allowed
>> (%d)\n",t
>> > +                   master_np->name, smmu->num_mapping_groups);

You allocate memory that will be lost forever if it fails for the first ID.

>> > +           return -ERANGE;
>> > +   }
>> > +
>> > +   if (master->cfg.num_streamids >= MAX_MASTER_STREAMIDS) {
>> > +           dev_err(smmu->dev,
>> > +                   "Reached maximum number (%d) of stream IDs for master
>> device %s\n",
>> > +                   MAX_MASTER_STREAMIDS, master_np->name);
>> > +           return -ENOSPC;

Ditto.

>> > +   }
>> > +
>> > +   /*
>> > +    * If this is the first time we add id to this master,
>> > +    * we have to register this master to rb tree.
>> > +    */
>> > +   if (!master->cfg.num_streamids) {
>> > +           int ret;
>> > +           ret = insert_smmu_master(smmu, master);
>> > +           if ( ret && ret != -EEXIST ) {
>> > +                   dev_err(smmu->dev,
>> > +                           "Insert %s to smmu's master rb tree failed\n",
>> master_np->name);
>> > +                   return ret;
>> > +           }
>> > +   }
>> > +
>> > +   master->cfg.streamids[master->cfg.num_streamids] = fwid;
>> > +   master->cfg.num_streamids++;
>> > +   dev_dbg(smmu->dev,
>> > +           "Add new streamid [%d] to smmu [%s] for master [%s]!\n",
>> > +           fwid, smmu->dev->of_node->name, master_np->name);
>> > +
>> > +   return 0;
>> > +}
>> > +
>> > +static struct arm_smmu_device *find_smmu(const struct device *dev);
>> > +
>> > +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args
>> *args)
>> > +{
>> > +   struct arm_smmu_device *smmu;
>> > +   u32 mask = 0, fwid = 0;
>> > +
>> > +   smmu = find_smmu(dt_to_dev(args->np));
>> > +   if (!smmu) {
>> > +           dev_err(dev, "Could not find smmu device!\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   if (args->args_count > 0)
>> > +           fwid |= (u16)args->args[0];
>> > +
>> > +   if (args->args_count > 1)
>> > +           fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
>> > +   else if (!of_property_read_u32(args->np, "stream-match-mask", &mask))
>> > +           fwid |= (u16)mask << SMR_MASK_SHIFT;
>> > +   dev_dbg(dev, "%s fwid:%08x mask:%08x args_count:%d\n",
>> > +                      args->np->full_name, fwid,
>> > +                      mask, args->args_count);
>> 
>> I don't understand why fwid is declared as u32 but used as a u16 below.
>> Shouldn't it be declared as u16 in the first place?
>> 
> 
> Oh, it's my mistake. In Linux, the fwid will be passed to 
> iommu_fwspec_add_ids,
> it requires u32 parameter. But after I ported this code to Xen, we passed
> the fwid to arm_smmu_add_generic_master_id, a u16 parameter is enough
u16 is not enough. If you look at the code you ported:

if (args->args_count > 0)
   fwid |= (u16)args->args[0];

if (!of_property_read_u32(args->np, "stream-match-mask", &mask)
   fwid |= mask << SMR_MASK_SHIFT;

SMR_MASK_SHIFT is 16, so the top 16-bit will be set to the mask. With  
your u16 cast you loose those bits and therefore will not support  
properly SMMU when the property "stream-match-mask" has been set.

Looking at our SMMU driver, I think we don't yet support sharing SMRS  
(you would need to double check). I would be ok if you don't support  
them, but at least you need to avoid blindly skipping the property  
because this is going to be difficult to debug. This means we need to  
print an error message and bail out if someone set that.

This kind of porting error could have been mitigated if this series was  
rebased as suggested multiple time on top of the fwspec work from QC  
(see [1]).

Regardless that, I would much prefer to rebase this work on top of the  
fwspec series. This is going to simplify a lot the logic and avoid code  
duplication, arm_smmu_add_generic_master_id is very similar to  
register_smmu_master.

Lastly, as I mentioned to you, any code not present in the Linux SMMU  
driver should be commented with /* Xen: ... */. This is helping us to  
know what has changed. For instance, I cannot find  
arm_smmu_add_generic_master_id in Linux code.

Cheers,

[1] https://lists.xen.org/archives/html/xen-devel/2017-06/msg00862.html

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/7] xen/arm: SMMU: Implement the add_device callback in SMMU

[Julien Grall]

Hi Wei,

On 06/30/2017 04:15 AM, Wei Chen wrote:
> This add_device callback function is taking care of adding a device
> to SMMU and make sure it is fully prepare to be used by the SMMU
> afterwards.
> 
> In previous code, we don't implement the add_device callback in
> iommu_ops for ARM SMMU. We placed the work of add_device to
> assign_device callback. The function assign_device should not care
> about adding the device to an iommu_group. It might not even be
> able to decide how to do that. In this patch, we move this work
> back to add_device callback.
> 
> This add_device callback is only called while we are handling all
> devices for constructing the Domain0.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>   xen/drivers/passthrough/arm/smmu.c | 34 +++++++++++++++++++++++-----------
>   1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 74c09b0..2efa52d 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2591,6 +2591,26 @@ static void arm_smmu_destroy_iommu_domain(struct iommu_domain *domain)
>   	xfree(domain);
>   }
>   
> +static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)

Coding style: struct device *dev

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Wei Chen]

Hi Julien,

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 2017年7月4日 15:27
> To: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini <sstabellini@kernel.org>
> Cc: xen-devel@lists.xen.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; nd <nd@arm.com>
> Subject: Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU
> bindings
> 
> Hi,
> 
> On 07/04/2017 07:27 AM, Wei Chen wrote:
> > Hi Stefano,
> >
> >> -----Original Message-----
> >> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> >> Sent: 2017年7月4日 7:00
> >> To: Wei Chen <Wei.Chen@arm.com>
> >> Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> >> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> >> <Julien.Grall@arm.com>; nd <nd@arm.com>
> >> Subject: Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU
> >> bindings
> >>
> >> On Fri, 30 Jun 2017, Wei Chen wrote:
> >> > The SMMU MasterIDs are placed at the master devices' DT node while
> >> > using the generic bindings. In this case, it's very hard for us to
> >> > register SMMU masters while probing SMMU as we had done for legacy
> >> > bindings. Because we have to go through whole device tree for all
> >> > SMMU devices to find their master devices.
> >> >
> >> > It's better to register SMMU master for generic bindings in add_device
> >> > callback. This callback will only be called while constructing Dom0.
> >> >
> >> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> >> > ---
> >> >  xen/drivers/passthrough/arm/smmu.c | 144
> >> ++++++++++++++++++++++++++++++++++++-
> >> >  1 file changed, 143 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> >> b/xen/drivers/passthrough/arm/smmu.c
> >> > index 895024c..25f2207 100644
> >> > --- a/xen/drivers/passthrough/arm/smmu.c
> >> > +++ b/xen/drivers/passthrough/arm/smmu.c
> >> > @@ -2621,8 +2621,150 @@ static void arm_smmu_destroy_iommu_domain(struct
> >> iommu_domain *domain)
> >> >      xfree(domain);
> >> >  }
> >> >
> >> > +static int arm_smmu_add_generic_master_id(struct arm_smmu_device *smmu,
> >> > +                           struct device *master_dev, u16 fwid)
> >> > +{
> >> > +   struct arm_smmu_master *master;
> >> > +   struct device_node *master_np = master_dev->of_node;
> >> > +
> >> > +   master = find_smmu_master(smmu, master_np);
> >> > +   if (!master) {
> >> > +           dev_notice(smmu->dev,
> >> > +                   "This smmu master [%s] hasn't been registered,
> creating

> >> now!\n",
> >> > +                   master_np->full_name);
> >> > +           master = devm_kzalloc(smmu->dev, sizeof(*master), GFP_KERNEL);
> >> > +           if (!master)
> >> > +                   return -ENOMEM;
> >> > +
> >> > +           master->of_node = master_np;
> >> > +           master->cfg.num_streamids = 0;
> >> > +
> >> > +           /*
> >> > +            * Xen: Let Xen know that the device is protected by a SMMU.
> >> > +            * Only do while registering the master.
> >> > +            */
> >> > +           dt_device_set_protected(master_np);
> >> > +   }
> >> > +
> >> > +   /*
> >> > +    * If the smmu is using the stream index mode, check whether
> >> > +    * the streamid exceeds the max allowed id,
> >> > +    */
> >> > +   if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
> >> > +           (fwid >= smmu->num_mapping_groups)) {
> >> > +           dev_err(smmu->dev,
> >> > +                   "Stream ID for master device %s greater than maximum
> allowed
> >> (%d)\n",t
> >> > +                   master_np->name, smmu->num_mapping_groups);
> 
> You allocate memory that will be lost forever if it fails for the first ID.
> 

Yes, it seems the register_smmu_master has the same issue. I will try to fix it, and
abstract the same logic of register_smmu_master and arm_smmu_add_generic_master_id
to a separate function.


> >> > +           return -ERANGE;
> >> > +   }
> >> > +
> >> > +   if (master->cfg.num_streamids >= MAX_MASTER_STREAMIDS) {
> >> > +           dev_err(smmu->dev,
> >> > +                   "Reached maximum number (%d) of stream IDs for master
> >> device %s\n",
> >> > +                   MAX_MASTER_STREAMIDS, master_np->name);
> >> > +           return -ENOSPC;
> 
> Ditto.

Got it.

> 
> >> > +   }
> >> > +
> >> > +   /*
> >> > +    * If this is the first time we add id to this master,
> >> > +    * we have to register this master to rb tree.
> >> > +    */
> >> > +   if (!master->cfg.num_streamids) {
> >> > +           int ret;
> >> > +           ret = insert_smmu_master(smmu, master);
> >> > +           if ( ret && ret != -EEXIST ) {
> >> > +                   dev_err(smmu->dev,
> >> > +                           "Insert %s to smmu's master rb tree failed\n",
> >> master_np->name);
> >> > +                   return ret;
> >> > +           }
> >> > +   }
> >> > +
> >> > +   master->cfg.streamids[master->cfg.num_streamids] = fwid;
> >> > +   master->cfg.num_streamids++;
> >> > +   dev_dbg(smmu->dev,
> >> > +           "Add new streamid [%d] to smmu [%s] for master [%s]!\n",
> >> > +           fwid, smmu->dev->of_node->name, master_np->name);
> >> > +
> >> > +   return 0;
> >> > +}
> >> > +
> >> > +static struct arm_smmu_device *find_smmu(const struct device *dev);
> >> > +
> >> > +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args
> >> *args)
> >> > +{
> >> > +   struct arm_smmu_device *smmu;
> >> > +   u32 mask = 0, fwid = 0;
> >> > +
> >> > +   smmu = find_smmu(dt_to_dev(args->np));
> >> > +   if (!smmu) {
> >> > +           dev_err(dev, "Could not find smmu device!\n");
> >> > +           return -ENODEV;
> >> > +   }
> >> > +
> >> > +   if (args->args_count > 0)
> >> > +           fwid |= (u16)args->args[0];
> >> > +
> >> > +   if (args->args_count > 1)
> >> > +           fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
> >> > +   else if (!of_property_read_u32(args->np, "stream-match-mask", &mask))
> >> > +           fwid |= (u16)mask << SMR_MASK_SHIFT;
> >> > +   dev_dbg(dev, "%s fwid:%08x mask:%08x args_count:%d\n",
> >> > +                      args->np->full_name, fwid,
> >> > +                      mask, args->args_count);
> >>
> >> I don't understand why fwid is declared as u32 but used as a u16 below.
> >> Shouldn't it be declared as u16 in the first place?
> >>
> >
> > Oh, it's my mistake. In Linux, the fwid will be passed to
> > iommu_fwspec_add_ids,
> > it requires u32 parameter. But after I ported this code to Xen, we passed
> > the fwid to arm_smmu_add_generic_master_id, a u16 parameter is enough
> u16 is not enough. If you look at the code you ported:
> 
> if (args->args_count > 0)
>    fwid |= (u16)args->args[0];
> 
> if (!of_property_read_u32(args->np, "stream-match-mask", &mask)
>    fwid |= mask << SMR_MASK_SHIFT;
> 
> SMR_MASK_SHIFT is 16, so the top 16-bit will be set to the mask. With
> your u16 cast you loose those bits and therefore will not support
> properly SMMU when the property "stream-match-mask" has been set.
> 

Yes, that's the reason why we use u32. Thanks for your reminder,
Although the master->cfg.streamids is using u16, we'd better to keep
U32 here and add a warning message to notice whom set "stream-match-mask"

> Looking at our SMMU driver, I think we don't yet support sharing SMRS
> (you would need to double check). I would be ok if you don't support
> them, but at least you need to avoid blindly skipping the property
> because this is going to be difficult to debug. This means we need to
> print an error message and bail out if someone set that.
> 

Yes, we don't currently share SMRs.

> This kind of porting error could have been mitigated if this series was
> rebased as suggested multiple time on top of the fwspec work from QC
> (see [1]).
> 
> Regardless that, I would much prefer to rebase this work on top of the
> fwspec series. This is going to simplify a lot the logic and avoid code
> duplication, arm_smmu_add_generic_master_id is very similar to
> register_smmu_master.
> 

If the fwspec work can be merged recently, I think it's good to rebase
On it.

> Lastly, as I mentioned to you, any code not present in the Linux SMMU
> driver should be commented with /* Xen: ... */. This is helping us to
> know what has changed. For instance, I cannot find
> arm_smmu_add_generic_master_id in Linux code.
> 

Sorry about it, I forgot this comment. I will add this comment to code.

> Cheers,
> 
> [1] https://lists.xen.org/archives/html/xen-devel/2017-06/msg00862.html
> 
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/7] xen/arm: SMMU: Implement the add_device callback in SMMU

[Wei Chen]

Hi Julien,

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 2017年7月4日 23:41
> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xen.org
> Cc: sstabellini@kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH 1/7] xen/arm: SMMU: Implement the add_device callback in
> SMMU
> 
> Hi Wei,
> 
> On 06/30/2017 04:15 AM, Wei Chen wrote:
> > This add_device callback function is taking care of adding a device
> > to SMMU and make sure it is fully prepare to be used by the SMMU
> > afterwards.
> >
> > In previous code, we don't implement the add_device callback in
> > iommu_ops for ARM SMMU. We placed the work of add_device to
> > assign_device callback. The function assign_device should not care
> > about adding the device to an iommu_group. It might not even be
> > able to decide how to do that. In this patch, we move this work
> > back to add_device callback.
> >
> > This add_device callback is only called while we are handling all
> > devices for constructing the Domain0.
> >
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > ---
> >   xen/drivers/passthrough/arm/smmu.c | 34 +++++++++++++++++++++++-----------
> >   1 file changed, 23 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> b/xen/drivers/passthrough/arm/smmu.c
> > index 74c09b0..2efa52d 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -2591,6 +2591,26 @@ static void arm_smmu_destroy_iommu_domain(struct
> iommu_domain *domain)
> >   	xfree(domain);
> >   }
> >
> > +static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> 
> Coding style: struct device *dev
> 

This is the second comment that I hadn't addressed. I will fix it.
Thank you!

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Julien Grall]

On 05/07/17 08:04, Wei Chen wrote:
> Hi Julien,

Hi Wei,

Please avoid replying in HTML on the xen-devel.

>> This kind of porting error could have been mitigated if this series was
>> rebased as suggested multiple time on top of the fwspec work from QC
>> (see [1]).
>>
>> Regardless that, I would much prefer to rebase this work on top of the
>> fwspec series. This is going to simplify a lot the logic and avoid code
>> duplication, arm_smmu_add_generic_master_id is very similar to
>> register_smmu_master.
>>
>
> If the fwspec work can be merged recently, I think it's good to rebase
> On it.

I am not sure to understand what you mean here. It is possible to rebase 
on a series without the series to be merged upstream.

Anyway, I have CCed Sameer to get a status update here.

>
>> Lastly, as I mentioned to you, any code not present in the Linux SMMU
>> driver should be commented with /* Xen: ... */. This is helping us to
>> know what has changed. For instance, I cannot find
>> arm_smmu_add_generic_master_id in Linux code.
>>
>
> Sorry about it, I forgot this comment. I will add this comment to code.
>
>> Cheers,
>>
>> [1] https://lists.xen.org/archives/html/xen-devel/2017-06/msg00862.html
>>
>> --
>> Julien Grall

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/7] xen/arm: SMMU: Implement the add_device callback in SMMU

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4295 bytes --]

On Tue, 4 Jul 2017, Wei Chen wrote:
> Hi Stefano,
> 
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > Sent: 2017年7月4日 5:58
> > To: Wei Chen <Wei.Chen@arm.com>
> > Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> > <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> > <Julien.Grall@arm.com>; nd <nd@arm.com>
> > Subject: Re: [Xen-devel] [PATCH 1/7] xen/arm: SMMU: Implement the add_device
> > callback in SMMU
> > 
> > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > This add_device callback function is taking care of adding a device
> > > to SMMU and make sure it is fully prepare to be used by the SMMU
> > > afterwards.
> > >
> > > In previous code, we don't implement the add_device callback in
> > > iommu_ops for ARM SMMU. We placed the work of add_device to
> > > assign_device callback. The function assign_device should not care
> > > about adding the device to an iommu_group. It might not even be
> > > able to decide how to do that. In this patch, we move this work
> > > back to add_device callback.
> > >
> > > This add_device callback is only called while we are handling all
> > > devices for constructing the Domain0.
> > >
> > > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > > ---
> > >  xen/drivers/passthrough/arm/smmu.c | 34 +++++++++++++++++++++++-----------
> > >  1 file changed, 23 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > b/xen/drivers/passthrough/arm/smmu.c
> > > index 74c09b0..2efa52d 100644
> > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > @@ -2591,6 +2591,26 @@ static void arm_smmu_destroy_iommu_domain(struct
> > iommu_domain *domain)
> > >  	xfree(domain);
> > >  }
> > >
> > > +static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> > > +{
> > > +	if (dt_device_is_protected(dev->of_node)) {
> > > +		if (!dev->archdata.iommu) {
> > > +			dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> > > +			if (!dev->archdata.iommu)
> > > +				return -ENOMEM;
> > > +		}
> > > +
> > > +		if (!dev_iommu_group(dev))
> > > +			return arm_smmu_add_device(dev);
> > > +	}
> > > +
> > > +	/*
> > > +	 * Return 0 if the device is not protected to follow the behavior
> > > +	 * of PCI add device.
> > 
> > What does this comment mean?
> > 
> 
> While I was looking at the iommu_add_device which is used by PCI counterpart.
> I found it will always return 0 even for device that are not protected by an IOMMU.
> I would much prefer to keep platform devices have similar behavior as PCI devices.
> 
> So while I was implementing iommu_add_dt_device and arm_smmu_xen_add_device, I
> returned 0 if the device is not protected by IOMMU.

I understand now. Please rephrase to:

  "Return 0 (not an error) if the device is not protected by an SMMU, to
  match the behavior of iommu_add_device."


> > 
> > > +	 */
> > > +	return 0;
> > > +}
> > > +
> > >  static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
> > >  			       struct device *dev, u32 flag)
> > >  {
> > > @@ -2600,17 +2620,8 @@ static int arm_smmu_assign_dev(struct domain *d, u8
> > devfn,
> > >
> > >  	xen_domain = dom_iommu(d)->arch.priv;
> > >
> > > -	if (!dev->archdata.iommu) {
> > > -		dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> > > -		if (!dev->archdata.iommu)
> > > -			return -ENOMEM;
> > > -	}
> > > -
> > > -	if (!dev_iommu_group(dev)) {
> > > -		ret = arm_smmu_add_device(dev);
> > > -		if (ret)
> > > -			return ret;
> > > -	}
> > > +	if (!dev_iommu_group(dev))
> > > +	    return -ENODEV;
> > >
> > >  	spin_lock(&xen_domain->lock);
> > >
> > > @@ -2784,6 +2795,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
> > >      .teardown = arm_smmu_iommu_domain_teardown,
> > >      .iotlb_flush = arm_smmu_iotlb_flush,
> > >      .iotlb_flush_all = arm_smmu_iotlb_flush_all,
> > > +    .add_device = arm_smmu_xen_add_device,
> > >      .assign_device = arm_smmu_assign_dev,
> > >      .reassign_device = arm_smmu_reassign_dev,
> > >      .map_page = arm_smmu_map_page,
> > > --
> > > 2.7.4
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > https://lists.xen.org/xen-devel
> > >
> 

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/7] xen/arm: SMMU: Introduce a helper to add DT device to SMMU

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4012 bytes --]

On Tue, 4 Jul 2017, Wei Chen wrote:
> Hi Stefano,
> 
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > Sent: 2017年7月4日 6:03
> > To: Wei Chen <Wei.Chen@arm.com>
> > Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> > <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> > <Julien.Grall@arm.com>; nd <nd@arm.com>
> > Subject: Re: [Xen-devel] [PATCH 2/7] xen/arm: SMMU: Introduce a helper to add
> > DT device to SMMU
> > 
> > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > In current code, we only have the iommu_add_device to add PCI device
> > > to IOMMU. But for ARM SMMU, we don't have a separate helper to add
> > > platform device with device tree to SMMU. This work was included in
> > > the iommu_assign_dt_device. But sometimes, we just want to add device
> > > to SMMU to do some preparation for further use. In this case, we can't
> > > call iommu_assign_dt_device.
> > >
> > > In previous patch, we have implement the add_device callback for SMMU,
> > > so we can separate this work from assign_device now.
> > >
> > > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > > ---
> > >  xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++++++
> > >  xen/include/xen/iommu.h               |  1 +
> > >  2 files changed, 21 insertions(+)
> > >
> > > diff --git a/xen/drivers/passthrough/device_tree.c
> > b/xen/drivers/passthrough/device_tree.c
> > > index 99ed49e..a8f403a 100644
> > > --- a/xen/drivers/passthrough/device_tree.c
> > > +++ b/xen/drivers/passthrough/device_tree.c
> > > @@ -24,6 +24,26 @@
> > >
> > >  static spinlock_t dtdevs_lock = SPIN_LOCK_UNLOCKED;
> > >
> > > +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev)
> > > +{
> > > +    int rc;
> > > +
> > > +    struct domain_iommu *hd = dom_iommu(d);
> > > +
> > > +    if ( !iommu_enabled || !hd->platform_ops ||
> > > +         !hd->platform_ops->add_device )
> > > +        return 0;
> > 
> > Shouldn't we also have:
> > 
> >   if ( !dt_device_is_protected(dev) )
> >         return 0;
> > 
> > ?
> > 
> 
> When we're using the legacy binding, the master IDs will be registered to SMMU and
> the protected flag of relevant master devices will be set to true (dt_device_is_protected
> will return true). But for generic IOMMU bindings, before we call ops->add_device,
> we didn't register the master device's master id to SMMU and hadn't set the protected
> flag, The dt_device_is_protected will always return false.
> 
> In this case, we can't call dt_device_is_protected(dev) here.

I get it. Please add an in-code comment here to explain the situation.


> > 
> > > +    spin_lock(&dtdevs_lock);
> > > +
> > > +    /* The devfn field doesn't matter to DT device. */
> > > +    rc = hd->platform_ops->add_device(0, dt_to_dev(dev));
> > > +
> > > +    spin_unlock(&dtdevs_lock);
> > > +
> > > +    return rc;
> > > +}
> > > +
> > >  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
> > >  {
> > >      int rc = -EBUSY;
> > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > > index 5803e3f..ec03faa 100644
> > > --- a/xen/include/xen/iommu.h
> > > +++ b/xen/include/xen/iommu.h
> > > @@ -132,6 +132,7 @@ void iommu_read_msi_from_ire(struct msi_desc *msi_desc,
> > struct msi_msg *msg);
> > >  #ifdef CONFIG_HAS_DEVICE_TREE
> > >  #include <xen/device_tree.h>
> > >
> > > +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev);
> > >  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
> > >  int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
> > >  int iommu_dt_domain_init(struct domain *d);
> > > --
> > > 2.7.4
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > https://lists.xen.org/xen-devel
> > >
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/7] xen/arm: SMMU: Detect types of device tree binding

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6391 bytes --]

On Tue, 4 Jul 2017, Wei Chen wrote:
> Hi Stefano,
> 
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > Sent: 2017年7月4日 6:30
> > To: Wei Chen <Wei.Chen@arm.com>
> > Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> > <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> > <Julien.Grall@arm.com>; nd <nd@arm.com>
> > Subject: Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device
> > tree binding
> > 
> > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > The device tree provides two types of IOMMU bindings, one is legacy
> > > another is generic. The legacy bindings will be depercated in favour
> >                                                   ^ deprecated
> > 
> > > of the generic bindings. But in the transitional period, we have to
> > > support both of them.
> > >
> > > The codes to handle these two types of bindings are very differnet,
> >       ^ code                                               ^ different
> > 
> > Please use a spellchecker.
> 
> Thanks, I will fix them.
> 
> > 
> > > so we have to detect the binding types while doing SMMU probing.
> > >
> > > This detect code is based on Linux ARM SMMUv2 driver:
> > > https://github.com/torvalds/linux/blob/master/drivers/iommu/arm-smmu.c
> > >
> > > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > > ---
> > >  xen/drivers/passthrough/arm/smmu.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > b/xen/drivers/passthrough/arm/smmu.c
> > > index 2efa52d..441c296 100644
> > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > @@ -143,6 +143,8 @@ typedef enum irqreturn irqreturn_t;
> > >
> > >  #define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
> > >
> > > +#define pr_notice(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
> > > +
> > >  /* Alias to Xen allocation helpers */
> > >  #define kfree xfree
> > >  #define kmalloc(size, flags)		_xmalloc(size, sizeof(void *))
> > > @@ -681,6 +683,8 @@ struct arm_smmu_option_prop {
> > >  	const char *prop;
> > >  };
> > >
> > > +static bool using_legacy_binding, using_generic_binding;
> > 
> > __initdata?
> > 
> 
> I think these two variables are not only used in initialization. They also
> have been used in ops->add_device. Althrough the add_device is only be
> invoked while construct_dom0.

I don't think that add_device is supposed to be limited at boot. It's
best to avoid __initdata then.


> > But why do these two variables need to be static? Can't they just be
> > local variables in arm_smmu_device_dt_probe?
> > 
> > Is it to enforce that all smmus on a given platform are either using the
> > legacy or the generic bindings, but not a mix of the two? If so, it
> > should be clearly written. Also, I am not sure we should really be
> 
> Yes, this checking will enforce all smmus are using the same bindings.
> 
> > checking for that. It seems to me that one smmu could be using generic
> > bindings and another could be using legacy bindings. Is it specified
> > anywhere that it cannot be the case?
> > 
> 
> In theory, different SMMUs can use different bindings. About this concern,
> I have discussed with Robin Murphy and Julien. We have three reasons:
> 
> The first is that, we ported this checking from Linux, we are trying to keep
> the code very close to the Linux driver. To ease backporting changes.
> 
> The second is that, we think it is a good change to try to phase out the 
> legacy binding and request to use generic bindings everywhere if you 
> start to use in one SMMU.
>  
> The other less technical reason for not supporting both at once is that anyone
> who can update their DT to add or update SMMUs with the new binding has no good
> excuse for not updating the whole lot. It's the likes of Seattle and ThunderX
> boxes with firmware that won't get updated for which we have to preserve "mmu-masters"
> support.

I would like these reasons to be written in the commit message. I would
also like to detect and print a clear warning when SMMUs are using
mismatched bindings.


> > 
> > >  static struct arm_smmu_option_prop arm_smmu_options[] = {
> > >  	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
> > >  	{ 0, NULL},
> > > @@ -2289,6 +2293,25 @@ static int arm_smmu_device_dt_probe(struct
> > platform_device *pdev)
> > >  	struct rb_node *node;
> > >  	struct of_phandle_args masterspec;
> > >  	int num_irqs, i, err;
> > > +	bool legacy_binding;
> > > +
> > > +	/*
> > > +	 * Xen: Do the same check as Linux. Checking the SMMU device tree
> > bindings
> >             ^ do                        ^ Check that
> > 
> > 
> > > +	 * are either using generic or legacy one.
> >                                           ^ bindings
> > 
> > > +	 *
> > > +	 * The "mmu-masters" property is only existed in legacy bindings.
> >                                   ^ only exists in the legacy bindings
> > 
> 
> Thanks, I will fix above typos.
> 
> > > +	 */
> > > +	legacy_binding = dt_find_property(dev->of_node, "mmu-masters", NULL);
> > > +	if (legacy_binding && !using_generic_binding) {
> > > +		if (!using_legacy_binding)
> > > +			pr_notice("deprecated \"mmu-masters\" DT property in use\n");
> > > +		using_legacy_binding = true;
> > > +	} else if (!legacy_binding && !using_legacy_binding) {
> > > +		using_generic_binding = true;
> > 
> > Please simplify this series of if/else.
> > 
> 
> This code is the same as Linux SMMU driver. If we agree on enforcing all smmus
> are using the same binding, I prefer to keep the code the same.

Is it?! Wow... All right then, but I would still like a warning to be
printed when we find out that an SMMU is using legacy bindings and
others are using generic bindings.


> > 
> > > +	} else { 
> > > +		dev_err(dev, "not probing due to mismatched DT properties\n");
> > > +		return -ENODEV;
> > > +	}
> > 
> > 
> > 
> > 
> > >  	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> > >  	if (!smmu) {
> > > --
> > > 2.7.4
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > https://lists.xen.org/xen-devel
> > >
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Stefano Stabellini]

On Wed, 5 Jul 2017, Wei Chen wrote:
> > >> > +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args
> > >> *args)
> > >> > +{
> > >> > +   struct arm_smmu_device *smmu;
> > >> > +   u32 mask = 0, fwid = 0;
> > >> > +
> > >> > +   smmu = find_smmu(dt_to_dev(args->np));
> > >> > +   if (!smmu) {
> > >> > +           dev_err(dev, "Could not find smmu device!\n");
> > >> > +           return -ENODEV;
> > >> > +   }
> > >> > +
> > >> > +   if (args->args_count > 0)
> > >> > +           fwid |= (u16)args->args[0];
> > >> > +
> > >> > +   if (args->args_count > 1)
> > >> > +           fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
> > >> > +   else if (!of_property_read_u32(args->np, "stream-match-mask", &mask))
> > >> > +           fwid |= (u16)mask << SMR_MASK_SHIFT;
> > >> > +   dev_dbg(dev, "%s fwid:%08x mask:%08x args_count:%d\n",
> > >> > +                      args->np->full_name, fwid,
> > >> > +                      mask, args->args_count);
> > >>
> > >> I don't understand why fwid is declared as u32 but used as a u16 below.
> > >> Shouldn't it be declared as u16 in the first place?
> > >>
> > >
> > > Oh, it's my mistake. In Linux, the fwid will be passed to
> > > iommu_fwspec_add_ids,
> > > it requires u32 parameter. But after I ported this code to Xen, we passed
> > > the fwid to arm_smmu_add_generic_master_id, a u16 parameter is enough
> > u16 is not enough. If you look at the code you ported:
> > 
> > if (args->args_count > 0)
> >    fwid |= (u16)args->args[0];
> > 
> > if (!of_property_read_u32(args->np, "stream-match-mask", &mask)
> >    fwid |= mask << SMR_MASK_SHIFT;
> > 
> > SMR_MASK_SHIFT is 16, so the top 16-bit will be set to the mask. With
> > your u16 cast you loose those bits and therefore will not support
> > properly SMMU when the property "stream-match-mask" has been set.
> > 
> 
> Yes, that's the reason why we use u32. Thanks for your reminder,
> Although the master->cfg.streamids is using u16, we'd better to keep
> U32 here and add a warning message to notice whom set "stream-match-mask"

Even if you are using a u32 for fwid, you are still losing all the top
16 bits in the operations above because of the (u16) casts. This code
looks wrong. 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Wei Chen]

Hi Julien,

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 2017年7月5日 21:08
> To: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini <sstabellini@kernel.org>
> Cc: xen-devel@lists.xen.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; nd <nd@arm.com>; Sameer Goel <sgoel@codeaurora.org>
> Subject: Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU
> bindings
> 
> 
> 
> On 05/07/17 08:04, Wei Chen wrote:
> > Hi Julien,
> 
> Hi Wei,
> 
> Please avoid replying in HTML on the xen-devel.
> 

Sorry about it.
It's very strange, I have checked my client, the configuration is plain text.
If the issue is still existed, please remind me.

Thank you!

> >> This kind of porting error could have been mitigated if this series was
> >> rebased as suggested multiple time on top of the fwspec work from QC
> >> (see [1]).
> >>
> >> Regardless that, I would much prefer to rebase this work on top of the
> >> fwspec series. This is going to simplify a lot the logic and avoid code
> >> duplication, arm_smmu_add_generic_master_id is very similar to
> >> register_smmu_master.
> >>
> >
> > If the fwspec work can be merged recently, I think it's good to rebase
> > On it.
> 
> I am not sure to understand what you mean here. It is possible to rebase
> on a series without the series to be merged upstream.
> 

Now, I understand. I had always thought we must rebase a merged series otherwise
If my series had been merged, we would encounter compiling issue.

I think this my mis-understanding. If the rebase series doesn't need to be merged,
I think it's ok to rebase on fwspec.

> Anyway, I have CCed Sameer to get a status update here.
> 
> >
> >> Lastly, as I mentioned to you, any code not present in the Linux SMMU
> >> driver should be commented with /* Xen: ... */. This is helping us to
> >> know what has changed. For instance, I cannot find
> >> arm_smmu_add_generic_master_id in Linux code.
> >>
> >
> > Sorry about it, I forgot this comment. I will add this comment to code.
> >
> >> Cheers,
> >>
> >> [1] https://lists.xen.org/archives/html/xen-devel/2017-06/msg00862.html
> >>
> >> --
> >> Julien Grall
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/7] xen/arm: SMMU: Implement the add_device callback in SMMU

[Wei Chen]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月6日 1:58
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xen.org;
> Steve Capper <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> <Julien.Grall@arm.com>; nd <nd@arm.com>
> Subject: RE: [Xen-devel] [PATCH 1/7] xen/arm: SMMU: Implement the add_device
> callback in SMMU
> 
> On Tue, 4 Jul 2017, Wei Chen wrote:
> > Hi Stefano,
> >
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > > Sent: 2017年7月4日 5:58
> > > To: Wei Chen <Wei.Chen@arm.com>
> > > Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> > > <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> > > <Julien.Grall@arm.com>; nd <nd@arm.com>
> > > Subject: Re: [Xen-devel] [PATCH 1/7] xen/arm: SMMU: Implement the
> add_device
> > > callback in SMMU
> > >
> > > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > > This add_device callback function is taking care of adding a device
> > > > to SMMU and make sure it is fully prepare to be used by the SMMU
> > > > afterwards.
> > > >
> > > > In previous code, we don't implement the add_device callback in
> > > > iommu_ops for ARM SMMU. We placed the work of add_device to
> > > > assign_device callback. The function assign_device should not care
> > > > about adding the device to an iommu_group. It might not even be
> > > > able to decide how to do that. In this patch, we move this work
> > > > back to add_device callback.
> > > >
> > > > This add_device callback is only called while we are handling all
> > > > devices for constructing the Domain0.
> > > >
> > > > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > > > ---
> > > >  xen/drivers/passthrough/arm/smmu.c | 34 +++++++++++++++++++++++--------
> ---
> > > >  1 file changed, 23 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > > b/xen/drivers/passthrough/arm/smmu.c
> > > > index 74c09b0..2efa52d 100644
> > > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > > @@ -2591,6 +2591,26 @@ static void arm_smmu_destroy_iommu_domain(struct
> > > iommu_domain *domain)
> > > >  	xfree(domain);
> > > >  }
> > > >
> > > > +static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> > > > +{
> > > > +	if (dt_device_is_protected(dev->of_node)) {
> > > > +		if (!dev->archdata.iommu) {
> > > > +			dev->archdata.iommu = xzalloc(struct
> arm_smmu_xen_device);
> > > > +			if (!dev->archdata.iommu)
> > > > +				return -ENOMEM;
> > > > +		}
> > > > +
> > > > +		if (!dev_iommu_group(dev))
> > > > +			return arm_smmu_add_device(dev);
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Return 0 if the device is not protected to follow the behavior
> > > > +	 * of PCI add device.
> > >
> > > What does this comment mean?
> > >
> >
> > While I was looking at the iommu_add_device which is used by PCI counterpart.
> > I found it will always return 0 even for device that are not protected by an
> IOMMU.
> > I would much prefer to keep platform devices have similar behavior as PCI
> devices.
> >
> > So while I was implementing iommu_add_dt_device and arm_smmu_xen_add_device,
> I
> > returned 0 if the device is not protected by IOMMU.
> 
> I understand now. Please rephrase to:
> 
>   "Return 0 (not an error) if the device is not protected by an SMMU, to
>   match the behavior of iommu_add_device."
> 

Ok, I will do it in next version.

> 
> > >
> > > > +	 */
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
> > > >  			       struct device *dev, u32 flag)
> > > >  {
> > > > @@ -2600,17 +2620,8 @@ static int arm_smmu_assign_dev(struct domain *d,
> u8
> > > devfn,
> > > >
> > > >  	xen_domain = dom_iommu(d)->arch.priv;
> > > >
> > > > -	if (!dev->archdata.iommu) {
> > > > -		dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
> > > > -		if (!dev->archdata.iommu)
> > > > -			return -ENOMEM;
> > > > -	}
> > > > -
> > > > -	if (!dev_iommu_group(dev)) {
> > > > -		ret = arm_smmu_add_device(dev);
> > > > -		if (ret)
> > > > -			return ret;
> > > > -	}
> > > > +	if (!dev_iommu_group(dev))
> > > > +	    return -ENODEV;
> > > >
> > > >  	spin_lock(&xen_domain->lock);
> > > >
> > > > @@ -2784,6 +2795,7 @@ static const struct iommu_ops arm_smmu_iommu_ops =
> {
> > > >      .teardown = arm_smmu_iommu_domain_teardown,
> > > >      .iotlb_flush = arm_smmu_iotlb_flush,
> > > >      .iotlb_flush_all = arm_smmu_iotlb_flush_all,
> > > > +    .add_device = arm_smmu_xen_add_device,
> > > >      .assign_device = arm_smmu_assign_dev,
> > > >      .reassign_device = arm_smmu_reassign_dev,
> > > >      .map_page = arm_smmu_map_page,
> > > > --
> > > > 2.7.4
> > > >
> > > >
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@lists.xen.org
> > > > https://lists.xen.org/xen-devel
> > > >
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/7] xen/arm: SMMU: Introduce a helper to add DT device to SMMU

[Wei Chen]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月6日 2:02
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Kaly Xin <Kaly.Xin@arm.com>;
> Julien Grall <Julien.Grall@arm.com>; Steve Capper <Steve.Capper@arm.com>; nd
> <nd@arm.com>; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH 2/7] xen/arm: SMMU: Introduce a helper to add
> DT device to SMMU
> 
> On Tue, 4 Jul 2017, Wei Chen wrote:
> > Hi Stefano,
> >
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > > Sent: 2017年7月4日 6:03
> > > To: Wei Chen <Wei.Chen@arm.com>
> > > Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> > > <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> > > <Julien.Grall@arm.com>; nd <nd@arm.com>
> > > Subject: Re: [Xen-devel] [PATCH 2/7] xen/arm: SMMU: Introduce a helper to
> add
> > > DT device to SMMU
> > >
> > > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > > In current code, we only have the iommu_add_device to add PCI device
> > > > to IOMMU. But for ARM SMMU, we don't have a separate helper to add
> > > > platform device with device tree to SMMU. This work was included in
> > > > the iommu_assign_dt_device. But sometimes, we just want to add device
> > > > to SMMU to do some preparation for further use. In this case, we can't
> > > > call iommu_assign_dt_device.
> > > >
> > > > In previous patch, we have implement the add_device callback for SMMU,
> > > > so we can separate this work from assign_device now.
> > > >
> > > > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > > > ---
> > > >  xen/drivers/passthrough/device_tree.c | 20 ++++++++++++++++++++
> > > >  xen/include/xen/iommu.h               |  1 +
> > > >  2 files changed, 21 insertions(+)
> > > >
> > > > diff --git a/xen/drivers/passthrough/device_tree.c
> > > b/xen/drivers/passthrough/device_tree.c
> > > > index 99ed49e..a8f403a 100644
> > > > --- a/xen/drivers/passthrough/device_tree.c
> > > > +++ b/xen/drivers/passthrough/device_tree.c
> > > > @@ -24,6 +24,26 @@
> > > >
> > > >  static spinlock_t dtdevs_lock = SPIN_LOCK_UNLOCKED;
> > > >
> > > > +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev)
> > > > +{
> > > > +    int rc;
> > > > +
> > > > +    struct domain_iommu *hd = dom_iommu(d);
> > > > +
> > > > +    if ( !iommu_enabled || !hd->platform_ops ||
> > > > +         !hd->platform_ops->add_device )
> > > > +        return 0;
> > >
> > > Shouldn't we also have:
> > >
> > >   if ( !dt_device_is_protected(dev) )
> > >         return 0;
> > >
> > > ?
> > >
> >
> > When we're using the legacy binding, the master IDs will be registered to
> SMMU and
> > the protected flag of relevant master devices will be set to true
> (dt_device_is_protected
> > will return true). But for generic IOMMU bindings, before we call ops-
> >add_device,
> > we didn't register the master device's master id to SMMU and hadn't set the
> protected
> > flag, The dt_device_is_protected will always return false.
> >
> > In this case, we can't call dt_device_is_protected(dev) here.
> 
> I get it. Please add an in-code comment here to explain the situation.
> 

Ok, I will do it.

> 
> > >
> > > > +    spin_lock(&dtdevs_lock);
> > > > +
> > > > +    /* The devfn field doesn't matter to DT device. */
> > > > +    rc = hd->platform_ops->add_device(0, dt_to_dev(dev));
> > > > +
> > > > +    spin_unlock(&dtdevs_lock);
> > > > +
> > > > +    return rc;
> > > > +}
> > > > +
> > > >  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
> > > >  {
> > > >      int rc = -EBUSY;
> > > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > > > index 5803e3f..ec03faa 100644
> > > > --- a/xen/include/xen/iommu.h
> > > > +++ b/xen/include/xen/iommu.h
> > > > @@ -132,6 +132,7 @@ void iommu_read_msi_from_ire(struct msi_desc
> *msi_desc,
> > > struct msi_msg *msg);
> > > >  #ifdef CONFIG_HAS_DEVICE_TREE
> > > >  #include <xen/device_tree.h>
> > > >
> > > > +int iommu_add_dt_device(struct domain *d, struct dt_device_node *dev);
> > > >  int iommu_assign_dt_device(struct domain *d, struct dt_device_node
> *dev);
> > > >  int iommu_deassign_dt_device(struct domain *d, struct dt_device_node
> *dev);
> > > >  int iommu_dt_domain_init(struct domain *d);
> > > > --
> > > > 2.7.4
> > > >
> > > >
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@lists.xen.org
> > > > https://lists.xen.org/xen-devel
> > > >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/7] xen/arm: SMMU: Detect types of device tree binding

[Wei Chen]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月6日 2:08
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Kaly Xin <Kaly.Xin@arm.com>;
> Julien Grall <Julien.Grall@arm.com>; Steve Capper <Steve.Capper@arm.com>; nd
> <nd@arm.com>; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device
> tree binding
> 
> On Tue, 4 Jul 2017, Wei Chen wrote:
> > Hi Stefano,
> >
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > > Sent: 2017年7月4日 6:30
> > > To: Wei Chen <Wei.Chen@arm.com>
> > > Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper
> > > <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Julien Grall
> > > <Julien.Grall@arm.com>; nd <nd@arm.com>
> > > Subject: Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device
> > > tree binding
> > >
> > > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > > The device tree provides two types of IOMMU bindings, one is legacy
> > > > another is generic. The legacy bindings will be depercated in favour
> > >                                                   ^ deprecated
> > >
> > > > of the generic bindings. But in the transitional period, we have to
> > > > support both of them.
> > > >
> > > > The codes to handle these two types of bindings are very differnet,
> > >       ^ code                                               ^ different
> > >
> > > Please use a spellchecker.
> >
> > Thanks, I will fix them.
> >
> > >
> > > > so we have to detect the binding types while doing SMMU probing.
> > > >
> > > > This detect code is based on Linux ARM SMMUv2 driver:
> > > > https://github.com/torvalds/linux/blob/master/drivers/iommu/arm-smmu.c
> > > >
> > > > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > > > ---
> > > >  xen/drivers/passthrough/arm/smmu.c | 23 +++++++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > > b/xen/drivers/passthrough/arm/smmu.c
> > > > index 2efa52d..441c296 100644
> > > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > > @@ -143,6 +143,8 @@ typedef enum irqreturn irqreturn_t;
> > > >
> > > >  #define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
> > > >
> > > > +#define pr_notice(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
> > > > +
> > > >  /* Alias to Xen allocation helpers */
> > > >  #define kfree xfree
> > > >  #define kmalloc(size, flags)		_xmalloc(size, sizeof(void *))
> > > > @@ -681,6 +683,8 @@ struct arm_smmu_option_prop {
> > > >  	const char *prop;
> > > >  };
> > > >
> > > > +static bool using_legacy_binding, using_generic_binding;
> > >
> > > __initdata?
> > >
> >
> > I think these two variables are not only used in initialization. They also
> > have been used in ops->add_device. Althrough the add_device is only be
> > invoked while construct_dom0.
> 
> I don't think that add_device is supposed to be limited at boot. It's
> best to avoid __initdata then.
> 

Yes. So let keep these two variables without __initdata here.

> 
> > > But why do these two variables need to be static? Can't they just be
> > > local variables in arm_smmu_device_dt_probe?
> > >
> > > Is it to enforce that all smmus on a given platform are either using the
> > > legacy or the generic bindings, but not a mix of the two? If so, it
> > > should be clearly written. Also, I am not sure we should really be
> >
> > Yes, this checking will enforce all smmus are using the same bindings.
> >
> > > checking for that. It seems to me that one smmu could be using generic
> > > bindings and another could be using legacy bindings. Is it specified
> > > anywhere that it cannot be the case?
> > >
> >
> > In theory, different SMMUs can use different bindings. About this concern,
> > I have discussed with Robin Murphy and Julien. We have three reasons:
> >
> > The first is that, we ported this checking from Linux, we are trying to keep
> > the code very close to the Linux driver. To ease backporting changes.
> >
> > The second is that, we think it is a good change to try to phase out the
> > legacy binding and request to use generic bindings everywhere if you
> > start to use in one SMMU.
> >
> > The other less technical reason for not supporting both at once is that
> anyone
> > who can update their DT to add or update SMMUs with the new binding has no
> good
> > excuse for not updating the whole lot. It's the likes of Seattle and
> ThunderX
> > boxes with firmware that won't get updated for which we have to preserve
> "mmu-masters"
> > support.
> 
> I would like these reasons to be written in the commit message. I would
> also like to detect and print a clear warning when SMMUs are using
> mismatched bindings.
> 

Ok.

> 
> > >
> > > >  static struct arm_smmu_option_prop arm_smmu_options[] = {
> > > >  	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-
> access" },
> > > >  	{ 0, NULL},
> > > > @@ -2289,6 +2293,25 @@ static int arm_smmu_device_dt_probe(struct
> > > platform_device *pdev)
> > > >  	struct rb_node *node;
> > > >  	struct of_phandle_args masterspec;
> > > >  	int num_irqs, i, err;
> > > > +	bool legacy_binding;
> > > > +
> > > > +	/*
> > > > +	 * Xen: Do the same check as Linux. Checking the SMMU device tree
> > > bindings
> > >             ^ do                        ^ Check that
> > >
> > >
> > > > +	 * are either using generic or legacy one.
> > >                                           ^ bindings
> > >
> > > > +	 *
> > > > +	 * The "mmu-masters" property is only existed in legacy bindings.
> > >                                   ^ only exists in the legacy bindings
> > >
> >
> > Thanks, I will fix above typos.
> >
> > > > +	 */
> > > > +	legacy_binding = dt_find_property(dev->of_node, "mmu-masters",
> NULL);
> > > > +	if (legacy_binding && !using_generic_binding) {
> > > > +		if (!using_legacy_binding)
> > > > +			pr_notice("deprecated \"mmu-masters\" DT property in
> use\n");
> > > > +		using_legacy_binding = true;
> > > > +	} else if (!legacy_binding && !using_legacy_binding) {
> > > > +		using_generic_binding = true;
> > >
> > > Please simplify this series of if/else.
> > >
> >
> > This code is the same as Linux SMMU driver. If we agree on enforcing all
> smmus
> > are using the same binding, I prefer to keep the code the same.
> 
> Is it?! Wow... All right then, but I would still like a warning to be
> printed when we find out that an SMMU is using legacy bindings and
> others are using generic bindings.
> 

Ok, I will add this warning message.

> 
> > >
> > > > +	} else {
> > > > +		dev_err(dev, "not probing due to mismatched DT
> properties\n");
> > > > +		return -ENODEV;
> > > > +	}
> > >
> > >
> > >
> > >
> > > >  	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> > > >  	if (!smmu) {
> > > > --
> > > > 2.7.4
> > > >
> > > >
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@lists.xen.org
> > > > https://lists.xen.org/xen-devel
> > > >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Wei Chen]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月6日 2:15
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: Julien Grall <Julien.Grall@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Kaly Xin <Kaly.Xin@arm.com>; nd <nd@arm.com>; Steve
> Capper <Steve.Capper@arm.com>; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU
> bindings
> 
> On Wed, 5 Jul 2017, Wei Chen wrote:
> > > >> > +static int arm_smmu_of_xlate(struct device *dev, struct
> of_phandle_args
> > > >> *args)
> > > >> > +{
> > > >> > +   struct arm_smmu_device *smmu;
> > > >> > +   u32 mask = 0, fwid = 0;
> > > >> > +
> > > >> > +   smmu = find_smmu(dt_to_dev(args->np));
> > > >> > +   if (!smmu) {
> > > >> > +           dev_err(dev, "Could not find smmu device!\n");
> > > >> > +           return -ENODEV;
> > > >> > +   }
> > > >> > +
> > > >> > +   if (args->args_count > 0)
> > > >> > +           fwid |= (u16)args->args[0];
> > > >> > +
> > > >> > +   if (args->args_count > 1)
> > > >> > +           fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
> > > >> > +   else if (!of_property_read_u32(args->np, "stream-match-mask",
> &mask))
> > > >> > +           fwid |= (u16)mask << SMR_MASK_SHIFT;
> > > >> > +   dev_dbg(dev, "%s fwid:%08x mask:%08x args_count:%d\n",
> > > >> > +                      args->np->full_name, fwid,
> > > >> > +                      mask, args->args_count);
> > > >>
> > > >> I don't understand why fwid is declared as u32 but used as a u16 below.
> > > >> Shouldn't it be declared as u16 in the first place?
> > > >>
> > > >
> > > > Oh, it's my mistake. In Linux, the fwid will be passed to
> > > > iommu_fwspec_add_ids,
> > > > it requires u32 parameter. But after I ported this code to Xen, we
> passed
> > > > the fwid to arm_smmu_add_generic_master_id, a u16 parameter is enough
> > > u16 is not enough. If you look at the code you ported:
> > >
> > > if (args->args_count > 0)
> > >    fwid |= (u16)args->args[0];
> > >
> > > if (!of_property_read_u32(args->np, "stream-match-mask", &mask)
> > >    fwid |= mask << SMR_MASK_SHIFT;
> > >
> > > SMR_MASK_SHIFT is 16, so the top 16-bit will be set to the mask. With
> > > your u16 cast you loose those bits and therefore will not support
> > > properly SMMU when the property "stream-match-mask" has been set.
> > >
> >
> > Yes, that's the reason why we use u32. Thanks for your reminder,
> > Although the master->cfg.streamids is using u16, we'd better to keep
> > U32 here and add a warning message to notice whom set "stream-match-mask"
> 
> Even if you are using a u32 for fwid, you are still losing all the top
> 16 bits in the operations above because of the (u16) casts. This code
> looks wrong.

I think it's better to change the
arm_smmu_add_generic_master_id(..., u16 fwid)
to
arm_smmu_add_generic_master_id(..., u32 fwid). And keep the fwid in arm_smmu_of_xlate
to u32 to guarantee the data integrity from device tree. Let arm_smmu_add_generic_master_id
to determine whether to drop top 16-bits or not.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel