[PATCH v3 0/3] Generic SMMU Bindings

[PATCH v3 0/3] Generic SMMU Bindings

[Stefano Stabellini]

Hi all,

This series introduces support for the generic SMMU bindings to
xen/drivers/passthrough/arm/smmu.c.

The last version of the series was
https://marc.info/?l=xen-devel&m=159539053406643

I realize that it is late for 4.15 -- I think it is OK if this series
goes in afterwards.

Cheers,

Stefano


Brian Woods (3):
      arm,smmu: switch to using iommu_fwspec functions
      arm,smmu: restructure code in preparation to new bindings support
      arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.

 xen/drivers/passthrough/arm/smmu.c    | 162 ++++++++++++++++++++++++----------
 xen/drivers/passthrough/device_tree.c |  24 ++---
 2 files changed, 123 insertions(+), 63 deletions(-)

[PATCH v3 1/3] arm,smmu: switch to using iommu_fwspec functions

[Stefano Stabellini]

From: Brian Woods <brian.woods@xilinx.com>

Modify the smmu driver so that it uses the iommu_fwspec helper
functions.  This means both ARM IOMMU drivers will both use the
iommu_fwspec helper functions, making enabling generic device tree
bindings in the SMMU driver much cleaner.

Signed-off-by: Brian Woods <brian.woods@xilinx.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v3:
- add a comment in iommu_add_dt_device
- don't allocate fwspec twice in arm_smmu_add_device
- reuse existing fwspec pointer, don't add a second one
- add comment about supporting fwspec at the top of the file
---
 xen/drivers/passthrough/arm/smmu.c    | 98 ++++++++++++++++-----------
 xen/drivers/passthrough/device_tree.c |  7 ++
 2 files changed, 66 insertions(+), 39 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 3e8aa37866..3898d1d737 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -32,6 +32,9 @@
  *	- 4k and 64k pages, with contiguous pte hints.
  *	- Up to 48-bit addressing (dependent on VA_BITS)
  *	- Context fault reporting
+ *
+ * Changes compared to Linux driver:
+ *	- support for fwspec
  */
 
 
@@ -49,6 +52,7 @@
 #include <asm/atomic.h>
 #include <asm/device.h>
 #include <asm/io.h>
+#include <asm/iommu_fwspec.h>
 #include <asm/platform.h>
 
 /* Xen: The below defines are redefined within the file. Undef it */
@@ -302,9 +306,6 @@ static struct iommu_group *iommu_group_get(struct device *dev)
 
 /***** Start of Linux SMMU code *****/
 
-/* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
-
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
 
@@ -597,8 +598,6 @@ struct arm_smmu_smr {
 };
 
 struct arm_smmu_master_cfg {
-	int				num_streamids;
-	u16				streamids[MAX_MASTER_STREAMIDS];
 	struct arm_smmu_smr		*smrs;
 };
 
@@ -686,6 +685,14 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ 0, NULL},
 };
 
+static inline struct iommu_fwspec *
+arm_smmu_get_fwspec(struct arm_smmu_master_cfg *cfg)
+{
+	struct arm_smmu_master *master = container_of(cfg,
+			                                      struct arm_smmu_master, cfg);
+	return dev_iommu_fwspec_get(&master->of_node->dev);
+}
+
 static void parse_driver_options(struct arm_smmu_device *smmu)
 {
 	int i = 0;
@@ -779,8 +786,9 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
 				struct of_phandle_args *masterspec)
 {
-	int i;
+	int i, ret = 0;
 	struct arm_smmu_master *master;
+	struct iommu_fwspec *fwspec;
 
 	master = find_smmu_master(smmu, masterspec->np);
 	if (master) {
@@ -790,34 +798,37 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 		return -EBUSY;
 	}
 
-	if (masterspec->args_count > MAX_MASTER_STREAMIDS) {
-		dev_err(dev,
-			"reached maximum number (%d) of stream IDs for master device %s\n",
-			MAX_MASTER_STREAMIDS, masterspec->np->name);
-		return -ENOSPC;
-	}
-
 	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
 	if (!master)
 		return -ENOMEM;
+	master->of_node = masterspec->np;
 
-	master->of_node			= masterspec->np;
-	master->cfg.num_streamids	= masterspec->args_count;
+	ret = iommu_fwspec_init(&master->of_node->dev, smmu->dev);
+	if (ret) {
+		kfree(master);
+		return ret;
+	}
+	fwspec = dev_iommu_fwspec_get(dev);
+
+	/* adding the ids here */
+	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
+				   masterspec->args,
+				   masterspec->args_count);
+	if (ret)
+		return ret;
 
 	/* Xen: Let Xen know that the device is protected by an SMMU */
 	dt_device_set_protected(masterspec->np);
 
-	for (i = 0; i < master->cfg.num_streamids; ++i) {
-		u16 streamid = masterspec->args[i];
-
-		if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
-		     (streamid >= smmu->num_mapping_groups)) {
-			dev_err(dev,
-				"stream ID for master device %s greater than maximum allowed (%d)\n",
-				masterspec->np->name, smmu->num_mapping_groups);
-			return -ERANGE;
+	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
+		for (i = 0; i < fwspec->num_ids; ++i) {
+			if (masterspec->args[i] >= smmu->num_mapping_groups) {
+				dev_err(dev,
+					"stream ID for master device %s greater than maximum allowed (%d)\n",
+					masterspec->np->name, smmu->num_mapping_groups);
+				return -ERANGE;
+			}
 		}
-		master->cfg.streamids[i] = streamid;
 	}
 	return insert_smmu_master(smmu, master);
 }
@@ -1390,6 +1401,7 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 	int i;
 	struct arm_smmu_smr *smrs;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
 
 	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH))
 		return 0;
@@ -1397,15 +1409,14 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 	if (cfg->smrs)
 		return -EEXIST;
 
-	smrs = kmalloc_array(cfg->num_streamids, sizeof(*smrs), GFP_KERNEL);
+	smrs = kmalloc_array(fwspec->num_ids, sizeof(*smrs), GFP_KERNEL);
 	if (!smrs) {
-		dev_err(smmu->dev, "failed to allocate %d SMRs\n",
-			cfg->num_streamids);
+		dev_err(smmu->dev, "failed to allocate %d SMRs\n", fwspec->num_ids);
 		return -ENOMEM;
 	}
 
 	/* Allocate the SMRs on the SMMU */
-	for (i = 0; i < cfg->num_streamids; ++i) {
+	for (i = 0; i < fwspec->num_ids; ++i) {
 		int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
 						  smmu->num_mapping_groups);
 		if (IS_ERR_VALUE(idx)) {
@@ -1416,12 +1427,12 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 		smrs[i] = (struct arm_smmu_smr) {
 			.idx	= idx,
 			.mask	= 0, /* We don't currently share SMRs */
-			.id	= cfg->streamids[i],
+			.id	= fwspec->ids[i],
 		};
 	}
 
 	/* It worked! Now, poke the actual hardware */
-	for (i = 0; i < cfg->num_streamids; ++i) {
+	for (i = 0; i < fwspec->num_ids; ++i) {
 		u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
 			  smrs[i].mask << SMR_MASK_SHIFT;
 		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_SMR(smrs[i].idx));
@@ -1443,12 +1454,13 @@ static void arm_smmu_master_free_smrs(struct arm_smmu_device *smmu,
 	int i;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 	struct arm_smmu_smr *smrs = cfg->smrs;
+	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
 
 	if (!smrs)
 		return;
 
 	/* Invalidate the SMRs before freeing back to the allocator */
-	for (i = 0; i < cfg->num_streamids; ++i) {
+	for (i = 0; i < fwspec->num_ids; ++i) {
 		u8 idx = smrs[i].idx;
 
 		writel_relaxed(~SMR_VALID, gr0_base + ARM_SMMU_GR0_SMR(idx));
@@ -1465,16 +1477,17 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	int i, ret;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
 
 	/* Devices in an IOMMU group may already be configured */
 	ret = arm_smmu_master_configure_smrs(smmu, cfg);
 	if (ret)
 		return ret == -EEXIST ? 0 : ret;
 
-	for (i = 0; i < cfg->num_streamids; ++i) {
+	for (i = 0; i < fwspec->num_ids; ++i) {
 		u32 idx, s2cr;
 
-		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
+		idx = cfg->smrs ? cfg->smrs[i].idx : fwspec->ids[i];
 		s2cr = S2CR_TYPE_TRANS |
 		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
@@ -1489,6 +1502,7 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
 	int i;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
 
 	/* An IOMMU group is torn down by the first device to be removed */
 	if ((smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) && !cfg->smrs)
@@ -1499,8 +1513,8 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
 	 * that it can be re-allocated immediately.
 	 * Xen: Unlike Linux, any access to non-configured stream will fault.
 	 */
-	for (i = 0; i < cfg->num_streamids; ++i) {
-		u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
+	for (i = 0; i < fwspec->num_ids; ++i) {
+		u32 idx = cfg->smrs ? cfg->smrs[i].idx : fwspec->ids[i];
 
 		writel_relaxed(S2CR_TYPE_FAULT,
 			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
@@ -1903,9 +1917,9 @@ static int arm_smmu_add_device(struct device *dev)
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_master_cfg *cfg;
 	struct iommu_group *group;
+	struct iommu_fwspec *fwspec;
 	void (*releasefn)(void *) = NULL;
 	int ret;
-
 	smmu = find_smmu_for_device(dev);
 	if (!smmu)
 		return -ENODEV;
@@ -1925,13 +1939,19 @@ static int arm_smmu_add_device(struct device *dev)
 			goto out_put_group;
 		}
 
-		cfg->num_streamids = 1;
+		ret = iommu_fwspec_init(dev, smmu->dev);
+		if (ret) {
+			kfree(cfg);
+			goto out_put_group;
+		}
+		fwspec = dev_iommu_fwspec_get(dev);
+
 		/*
 		 * Assume Stream ID == Requester ID for now.
 		 * We need a way to describe the ID mappings in FDT.
 		 */
 		pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid,
-				       &cfg->streamids[0]);
+				       &fwspec->ids[0]);
 		releasefn = __arm_smmu_release_pci_iommudata;
 	} else {
 		struct arm_smmu_master *master;
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 999b831d90..a51ae3c9c3 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -140,6 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
     if ( !ops )
         return -EINVAL;
 
+	/*
+	 * This is needed in case a device has both the iommus property and
+	 * also apperars in the mmu-masters list.
+	 */
+    if ( dt_device_is_protected(np) )
+        return 0;
+
     if ( dev_iommu_fwspec_get(dev) )
         return -EEXIST;
 
-- 
2.17.1

Re: [PATCH v3 1/3] arm,smmu: switch to using iommu_fwspec functions

[Rahul Singh]

Hello Stefano,

> On 26 Jan 2021, at 10:58 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> From: Brian Woods <brian.woods@xilinx.com>
> 
> Modify the smmu driver so that it uses the iommu_fwspec helper
> functions.  This means both ARM IOMMU drivers will both use the
> iommu_fwspec helper functions, making enabling generic device tree
> bindings in the SMMU driver much cleaner.
> 
> Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
> ---
> Changes in v3:
> - add a comment in iommu_add_dt_device
> - don't allocate fwspec twice in arm_smmu_add_device
> - reuse existing fwspec pointer, don't add a second one
> - add comment about supporting fwspec at the top of the file
> ---
> xen/drivers/passthrough/arm/smmu.c    | 98 ++++++++++++++++-----------
> xen/drivers/passthrough/device_tree.c |  7 ++
> 2 files changed, 66 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 3e8aa37866..3898d1d737 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -32,6 +32,9 @@
>  *	- 4k and 64k pages, with contiguous pte hints.
>  *	- Up to 48-bit addressing (dependent on VA_BITS)
>  *	- Context fault reporting
> + *
> + * Changes compared to Linux driver:
> + *	- support for fwspec
>  */
> 
> 
> @@ -49,6 +52,7 @@
> #include <asm/atomic.h>
> #include <asm/device.h>
> #include <asm/io.h>
> +#include <asm/iommu_fwspec.h>
> #include <asm/platform.h>
> 
> /* Xen: The below defines are redefined within the file. Undef it */
> @@ -302,9 +306,6 @@ static struct iommu_group *iommu_group_get(struct device *dev)
> 
> /***** Start of Linux SMMU code *****/
> 
> -/* Maximum number of stream IDs assigned to a single device */
> -#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
> -
> /* Maximum number of context banks per SMMU */
> #define ARM_SMMU_MAX_CBS		128
> 
> @@ -597,8 +598,6 @@ struct arm_smmu_smr {
> };
> 
> struct arm_smmu_master_cfg {
> -	int				num_streamids;
> -	u16				streamids[MAX_MASTER_STREAMIDS];
> 	struct arm_smmu_smr		*smrs;
> };
> 
> @@ -686,6 +685,14 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
> 	{ 0, NULL},
> };
> 
> +static inline struct iommu_fwspec *
> +arm_smmu_get_fwspec(struct arm_smmu_master_cfg *cfg)
> +{
> +	struct arm_smmu_master *master = container_of(cfg,
> +			                                      struct arm_smmu_master, cfg);
> +	return dev_iommu_fwspec_get(&master->of_node->dev);
> +}
> +
> static void parse_driver_options(struct arm_smmu_device *smmu)
> {
> 	int i = 0;
> @@ -779,8 +786,9 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
> 				struct device *dev,
> 				struct of_phandle_args *masterspec)
> {
> -	int i;
> +	int i, ret = 0;
> 	struct arm_smmu_master *master;
> +	struct iommu_fwspec *fwspec;
> 
> 	master = find_smmu_master(smmu, masterspec->np);
> 	if (master) {
> @@ -790,34 +798,37 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
> 		return -EBUSY;
> 	}
> 
> -	if (masterspec->args_count > MAX_MASTER_STREAMIDS) {
> -		dev_err(dev,
> -			"reached maximum number (%d) of stream IDs for master device %s\n",
> -			MAX_MASTER_STREAMIDS, masterspec->np->name);
> -		return -ENOSPC;
> -	}
> -
> 	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
> 	if (!master)
> 		return -ENOMEM;
> +	master->of_node = masterspec->np;
> 
> -	master->of_node			= masterspec->np;
> -	master->cfg.num_streamids	= masterspec->args_count;
> +	ret = iommu_fwspec_init(&master->of_node->dev, smmu->dev);
> +	if (ret) {
> +		kfree(master);
> +		return ret;
> +	}
> +	fwspec = dev_iommu_fwspec_get(dev);
> +
> +	/* adding the ids here */
> +	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
> +				   masterspec->args,
> +				   masterspec->args_count);
> +	if (ret)
> +		return ret;
> 
> 	/* Xen: Let Xen know that the device is protected by an SMMU */
> 	dt_device_set_protected(masterspec->np);
> 
> -	for (i = 0; i < master->cfg.num_streamids; ++i) {
> -		u16 streamid = masterspec->args[i];
> -
> -		if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
> -		     (streamid >= smmu->num_mapping_groups)) {
> -			dev_err(dev,
> -				"stream ID for master device %s greater than maximum allowed (%d)\n",
> -				masterspec->np->name, smmu->num_mapping_groups);
> -			return -ERANGE;
> +	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
> +		for (i = 0; i < fwspec->num_ids; ++i) {
> +			if (masterspec->args[i] >= smmu->num_mapping_groups) {
> +				dev_err(dev,
> +					"stream ID for master device %s greater than maximum allowed (%d)\n",
> +					masterspec->np->name, smmu->num_mapping_groups);
> +				return -ERANGE;
> +			}
> 		}
> -		master->cfg.streamids[i] = streamid;
> 	}
> 	return insert_smmu_master(smmu, master);
> }
> @@ -1390,6 +1401,7 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
> 	int i;
> 	struct arm_smmu_smr *smrs;
> 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> +	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
> 
> 	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH))
> 		return 0;
> @@ -1397,15 +1409,14 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
> 	if (cfg->smrs)
> 		return -EEXIST;
> 
> -	smrs = kmalloc_array(cfg->num_streamids, sizeof(*smrs), GFP_KERNEL);
> +	smrs = kmalloc_array(fwspec->num_ids, sizeof(*smrs), GFP_KERNEL);
> 	if (!smrs) {
> -		dev_err(smmu->dev, "failed to allocate %d SMRs\n",
> -			cfg->num_streamids);
> +		dev_err(smmu->dev, "failed to allocate %d SMRs\n", fwspec->num_ids);
> 		return -ENOMEM;
> 	}
> 
> 	/* Allocate the SMRs on the SMMU */
> -	for (i = 0; i < cfg->num_streamids; ++i) {
> +	for (i = 0; i < fwspec->num_ids; ++i) {
> 		int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
> 						  smmu->num_mapping_groups);
> 		if (IS_ERR_VALUE(idx)) {
> @@ -1416,12 +1427,12 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
> 		smrs[i] = (struct arm_smmu_smr) {
> 			.idx	= idx,
> 			.mask	= 0, /* We don't currently share SMRs */
> -			.id	= cfg->streamids[i],
> +			.id	= fwspec->ids[i],
> 		};
> 	}
> 
> 	/* It worked! Now, poke the actual hardware */
> -	for (i = 0; i < cfg->num_streamids; ++i) {
> +	for (i = 0; i < fwspec->num_ids; ++i) {
> 		u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
> 			  smrs[i].mask << SMR_MASK_SHIFT;
> 		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_SMR(smrs[i].idx));
> @@ -1443,12 +1454,13 @@ static void arm_smmu_master_free_smrs(struct arm_smmu_device *smmu,
> 	int i;
> 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> 	struct arm_smmu_smr *smrs = cfg->smrs;
> +	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
> 
> 	if (!smrs)
> 		return;
> 
> 	/* Invalidate the SMRs before freeing back to the allocator */
> -	for (i = 0; i < cfg->num_streamids; ++i) {
> +	for (i = 0; i < fwspec->num_ids; ++i) {
> 		u8 idx = smrs[i].idx;
> 
> 		writel_relaxed(~SMR_VALID, gr0_base + ARM_SMMU_GR0_SMR(idx));
> @@ -1465,16 +1477,17 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
> 	int i, ret;
> 	struct arm_smmu_device *smmu = smmu_domain->smmu;
> 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> +	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
> 
> 	/* Devices in an IOMMU group may already be configured */
> 	ret = arm_smmu_master_configure_smrs(smmu, cfg);
> 	if (ret)
> 		return ret == -EEXIST ? 0 : ret;
> 
> -	for (i = 0; i < cfg->num_streamids; ++i) {
> +	for (i = 0; i < fwspec->num_ids; ++i) {
> 		u32 idx, s2cr;
> 
> -		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
> +		idx = cfg->smrs ? cfg->smrs[i].idx : fwspec->ids[i];
> 		s2cr = S2CR_TYPE_TRANS |
> 		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
> 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
> @@ -1489,6 +1502,7 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
> 	int i;
> 	struct arm_smmu_device *smmu = smmu_domain->smmu;
> 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> +	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
> 
> 	/* An IOMMU group is torn down by the first device to be removed */
> 	if ((smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) && !cfg->smrs)
> @@ -1499,8 +1513,8 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
> 	 * that it can be re-allocated immediately.
> 	 * Xen: Unlike Linux, any access to non-configured stream will fault.
> 	 */
> -	for (i = 0; i < cfg->num_streamids; ++i) {
> -		u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
> +	for (i = 0; i < fwspec->num_ids; ++i) {
> +		u32 idx = cfg->smrs ? cfg->smrs[i].idx : fwspec->ids[i];
> 
> 		writel_relaxed(S2CR_TYPE_FAULT,
> 			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
> @@ -1903,9 +1917,9 @@ static int arm_smmu_add_device(struct device *dev)
> 	struct arm_smmu_device *smmu;
> 	struct arm_smmu_master_cfg *cfg;
> 	struct iommu_group *group;
> +	struct iommu_fwspec *fwspec;
> 	void (*releasefn)(void *) = NULL;
> 	int ret;
> -
> 	smmu = find_smmu_for_device(dev);
> 	if (!smmu)
> 		return -ENODEV;
> @@ -1925,13 +1939,19 @@ static int arm_smmu_add_device(struct device *dev)
> 			goto out_put_group;
> 		}
> 
> -		cfg->num_streamids = 1;
> +		ret = iommu_fwspec_init(dev, smmu->dev);
> +		if (ret) {
> +			kfree(cfg);
> +			goto out_put_group;
> +		}
> +		fwspec = dev_iommu_fwspec_get(dev);
> +
> 		/*
> 		 * Assume Stream ID == Requester ID for now.
> 		 * We need a way to describe the ID mappings in FDT.
> 		 */
> 		pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid,
> -				       &cfg->streamids[0]);
> +				       &fwspec->ids[0]);
> 		releasefn = __arm_smmu_release_pci_iommudata;
> 	} else {
> 		struct arm_smmu_master *master;
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 999b831d90..a51ae3c9c3 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -140,6 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>     if ( !ops )
>         return -EINVAL;
> 
> +	/*
> +	 * This is needed in case a device has both the iommus property and
> +	 * also apperars in the mmu-masters list.
> +	 */
> +    if ( dt_device_is_protected(np) )
> +        return 0;
> +
>     if ( dev_iommu_fwspec_get(dev) )
>         return -EEXIST;
> 
> -- 
> 2.17.1
> 
> 

[PATCH v3 2/3] arm,smmu: restructure code in preparation to new bindings support

[Stefano Stabellini]

From: Brian Woods <brian.woods@xilinx.com>

Restructure some of the code and add supporting functions for adding
generic device tree (DT) binding support.  This will allow for using
current Linux device trees with just modifying the chosen field to
enable Xen.

Signed-off-by: Brian Woods <brian.woods@xilinx.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v3:
- split patch
---
 xen/drivers/passthrough/arm/smmu.c | 60 +++++++++++++++++-------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 3898d1d737..9687762283 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -782,50 +782,36 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 	return 0;
 }
 
-static int register_smmu_master(struct arm_smmu_device *smmu,
-				struct device *dev,
-				struct of_phandle_args *masterspec)
+static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
+					 struct device *dev,
+					 struct iommu_fwspec *fwspec)
 {
-	int i, ret = 0;
+	int i;
 	struct arm_smmu_master *master;
-	struct iommu_fwspec *fwspec;
+	struct device_node *dev_node = dev_get_dev_node(dev);
 
-	master = find_smmu_master(smmu, masterspec->np);
+	master = find_smmu_master(smmu, dev_node);
 	if (master) {
 		dev_err(dev,
 			"rejecting multiple registrations for master device %s\n",
-			masterspec->np->name);
+			dev_node->name);
 		return -EBUSY;
 	}
 
 	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
 	if (!master)
 		return -ENOMEM;
-	master->of_node = masterspec->np;
-
-	ret = iommu_fwspec_init(&master->of_node->dev, smmu->dev);
-	if (ret) {
-		kfree(master);
-		return ret;
-	}
-	fwspec = dev_iommu_fwspec_get(dev);
-
-	/* adding the ids here */
-	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
-				   masterspec->args,
-				   masterspec->args_count);
-	if (ret)
-		return ret;
+	master->of_node = dev_node;
 
 	/* Xen: Let Xen know that the device is protected by an SMMU */
-	dt_device_set_protected(masterspec->np);
+	dt_device_set_protected(dev_node);
 
 	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
 		for (i = 0; i < fwspec->num_ids; ++i) {
-			if (masterspec->args[i] >= smmu->num_mapping_groups) {
+			if (fwspec->ids[i] >= smmu->num_mapping_groups) {
 				dev_err(dev,
 					"stream ID for master device %s greater than maximum allowed (%d)\n",
-					masterspec->np->name, smmu->num_mapping_groups);
+					dev_node->name, smmu->num_mapping_groups);
 				return -ERANGE;
 			}
 		}
@@ -833,6 +819,30 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	return insert_smmu_master(smmu, master);
 }
 
+static int register_smmu_master(struct arm_smmu_device *smmu,
+				struct device *dev,
+				struct of_phandle_args *masterspec)
+{
+	int ret = 0;
+	struct iommu_fwspec *fwspec;
+
+	ret = iommu_fwspec_init(&masterspec->np->dev, smmu->dev);
+	if (ret)
+		return ret;
+
+	fwspec = dev_iommu_fwspec_get(&masterspec->np->dev);
+
+	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
+				   masterspec->args,
+				   masterspec->args_count);
+	if (ret)
+		return ret;
+
+	return arm_smmu_dt_add_device_legacy(smmu,
+					     &masterspec->np->dev,
+					     fwspec);
+}
+
 static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
-- 
2.17.1

Re: [PATCH v3 2/3] arm,smmu: restructure code in preparation to new bindings support

[Rahul Singh]

Hello Stefano,

> On 26 Jan 2021, at 10:58 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> From: Brian Woods <brian.woods@xilinx.com>
> 
> Restructure some of the code and add supporting functions for adding
> generic device tree (DT) binding support.  This will allow for using
> current Linux device trees with just modifying the chosen field to
> enable Xen.
> 
> Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
> ---
> Changes in v3:
> - split patch
> ---
> xen/drivers/passthrough/arm/smmu.c | 60 +++++++++++++++++-------------
> 1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 3898d1d737..9687762283 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -782,50 +782,36 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
> 	return 0;
> }
> 
> -static int register_smmu_master(struct arm_smmu_device *smmu,
> -				struct device *dev,
> -				struct of_phandle_args *masterspec)
> +static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
> +					 struct device *dev,
> +					 struct iommu_fwspec *fwspec)
> {
> -	int i, ret = 0;
> +	int i;
> 	struct arm_smmu_master *master;
> -	struct iommu_fwspec *fwspec;
> +	struct device_node *dev_node = dev_get_dev_node(dev);
> 
> -	master = find_smmu_master(smmu, masterspec->np);
> +	master = find_smmu_master(smmu, dev_node);
> 	if (master) {
> 		dev_err(dev,
> 			"rejecting multiple registrations for master device %s\n",
> -			masterspec->np->name);
> +			dev_node->name);
> 		return -EBUSY;
> 	}
> 
> 	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
> 	if (!master)
> 		return -ENOMEM;
> -	master->of_node = masterspec->np;
> -
> -	ret = iommu_fwspec_init(&master->of_node->dev, smmu->dev);
> -	if (ret) {
> -		kfree(master);
> -		return ret;
> -	}
> -	fwspec = dev_iommu_fwspec_get(dev);
> -
> -	/* adding the ids here */
> -	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
> -				   masterspec->args,
> -				   masterspec->args_count);
> -	if (ret)
> -		return ret;
> +	master->of_node = dev_node;
> 
> 	/* Xen: Let Xen know that the device is protected by an SMMU */
> -	dt_device_set_protected(masterspec->np);
> +	dt_device_set_protected(dev_node);
> 
> 	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
> 		for (i = 0; i < fwspec->num_ids; ++i) {
> -			if (masterspec->args[i] >= smmu->num_mapping_groups) {
> +			if (fwspec->ids[i] >= smmu->num_mapping_groups) {
> 				dev_err(dev,
> 					"stream ID for master device %s greater than maximum allowed (%d)\n",
> -					masterspec->np->name, smmu->num_mapping_groups);
> +					dev_node->name, smmu->num_mapping_groups);
> 				return -ERANGE;
> 			}
> 		}
> @@ -833,6 +819,30 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
> 	return insert_smmu_master(smmu, master);
> }
> 
> +static int register_smmu_master(struct arm_smmu_device *smmu,
> +				struct device *dev,
> +				struct of_phandle_args *masterspec)
> +{
> +	int ret = 0;
> +	struct iommu_fwspec *fwspec;
> +
> +	ret = iommu_fwspec_init(&masterspec->np->dev, smmu->dev);
> +	if (ret)
> +		return ret;
> +
> +	fwspec = dev_iommu_fwspec_get(&masterspec->np->dev);
> +
> +	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
> +				   masterspec->args,
> +				   masterspec->args_count);
> +	if (ret)
> +		return ret;
> +
> +	return arm_smmu_dt_add_device_legacy(smmu,
> +					     &masterspec->np->dev,
> +					     fwspec);
> +}
> +
> static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
> {
> 	struct arm_smmu_device *smmu;
> -- 
> 2.17.1
> 
> 

[PATCH v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.

[Stefano Stabellini]

From: Brian Woods <brian.woods@xilinx.com>

Now that all arm iommu drivers support generic bindings we can remove
the workaround from iommu_add_dt_device().

Note that if both legacy bindings and generic bindings are present in
device tree, the legacy bindings are the ones that are used.

Signed-off-by: Brian Woods <brian.woods@xilinx.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v3:
- split patch
- make find_smmu return non-const so that we can use it in arm_smmu_dt_add_device_generic
- use dt_phandle_args
- update commit message
---
 xen/drivers/passthrough/arm/smmu.c    | 40 ++++++++++++++++++++++++++-
 xen/drivers/passthrough/device_tree.c | 17 +-----------
 2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 9687762283..620ba5a4b5 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -254,6 +254,8 @@ struct iommu_group
 	atomic_t ref;
 };
 
+static struct arm_smmu_device *find_smmu(const struct device *dev);
+
 static struct iommu_group *iommu_group_alloc(void)
 {
 	struct iommu_group *group = xzalloc(struct iommu_group);
@@ -843,6 +845,40 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 					     fwspec);
 }
 
+static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
+{
+	struct arm_smmu_device *smmu;
+	struct iommu_fwspec *fwspec;
+
+	fwspec = dev_iommu_fwspec_get(dev);
+	if (fwspec == NULL)
+		return -ENXIO;
+
+	smmu = find_smmu(fwspec->iommu_dev);
+	if (smmu == NULL)
+		return -ENXIO;
+
+	return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
+}
+
+static int arm_smmu_dt_xlate_generic(struct device *dev,
+				    const struct dt_phandle_args *spec)
+{
+	uint32_t mask, fwid = 0;
+
+	if (spec->args_count > 0)
+		fwid |= (SMR_ID_MASK & spec->args[0]) << SMR_ID_SHIFT;
+
+	if (spec->args_count > 1)
+		fwid |= (SMR_MASK_MASK & spec->args[1]) << SMR_MASK_SHIFT;
+	else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask))
+		fwid |= (SMR_MASK_MASK & mask) << SMR_MASK_SHIFT;
+
+	return iommu_fwspec_add_ids(dev,
+				    &fwid,
+				    1);
+}
+
 static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
@@ -2766,6 +2802,7 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
 static const struct iommu_ops arm_smmu_iommu_ops = {
     .init = arm_smmu_iommu_domain_init,
     .hwdom_init = arm_smmu_iommu_hwdom_init,
+    .add_device = arm_smmu_dt_add_device_generic,
     .teardown = arm_smmu_iommu_domain_teardown,
     .iotlb_flush = arm_smmu_iotlb_flush,
     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
@@ -2773,9 +2810,10 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
     .reassign_device = arm_smmu_reassign_dev,
     .map_page = arm_iommu_map_page,
     .unmap_page = arm_iommu_unmap_page,
+    .dt_xlate = arm_smmu_dt_xlate_generic,
 };
 
-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;
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index a51ae3c9c3..ae07f272e1 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -162,22 +162,7 @@ int iommu_add_dt_device(struct dt_device_node *np)
          * these callback implemented.
          */
         if ( !ops->add_device || !ops->dt_xlate )
-        {
-            /*
-             * Some Device Trees may expose both legacy SMMU and generic
-             * IOMMU bindings together. However, the SMMU driver is only
-             * supporting the former and will protect them during the
-             * initialization. So we need to skip them and not return
-             * error here.
-             *
-             * XXX: This can be dropped when the SMMU is able to deal
-             * with generic bindings.
-             */
-            if ( dt_device_is_protected(np) )
-                return 0;
-            else
-                return -EINVAL;
-        }
+            return -EINVAL;
 
         if ( !dt_device_is_available(iommu_spec.np) )
             break;
-- 
2.17.1

Re: [PATCH v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.

[Rahul Singh]

Hello Stefano,

> On 26 Jan 2021, at 10:58 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> From: Brian Woods <brian.woods@xilinx.com>
> 
> Now that all arm iommu drivers support generic bindings we can remove
> the workaround from iommu_add_dt_device().
> 
> Note that if both legacy bindings and generic bindings are present in
> device tree, the legacy bindings are the ones that are used.
> 
> Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
> ---
> Changes in v3:
> - split patch
> - make find_smmu return non-const so that we can use it in arm_smmu_dt_add_device_generic
> - use dt_phandle_args
> - update commit message
> ---
> xen/drivers/passthrough/arm/smmu.c    | 40 ++++++++++++++++++++++++++-
> xen/drivers/passthrough/device_tree.c | 17 +-----------
> 2 files changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 9687762283..620ba5a4b5 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -254,6 +254,8 @@ struct iommu_group
> 	atomic_t ref;
> };
> 
> +static struct arm_smmu_device *find_smmu(const struct device *dev);
> +
> static struct iommu_group *iommu_group_alloc(void)
> {
> 	struct iommu_group *group = xzalloc(struct iommu_group);
> @@ -843,6 +845,40 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
> 					     fwspec);
> }
> 
> +static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
> +{
> +	struct arm_smmu_device *smmu;
> +	struct iommu_fwspec *fwspec;
> +
> +	fwspec = dev_iommu_fwspec_get(dev);
> +	if (fwspec == NULL)
> +		return -ENXIO;
> +
> +	smmu = find_smmu(fwspec->iommu_dev);
> +	if (smmu == NULL)
> +		return -ENXIO;
> +
> +	return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
> +}
> +
> +static int arm_smmu_dt_xlate_generic(struct device *dev,
> +				    const struct dt_phandle_args *spec)
> +{
> +	uint32_t mask, fwid = 0;
> +
> +	if (spec->args_count > 0)
> +		fwid |= (SMR_ID_MASK & spec->args[0]) << SMR_ID_SHIFT;
> +
> +	if (spec->args_count > 1)
> +		fwid |= (SMR_MASK_MASK & spec->args[1]) << SMR_MASK_SHIFT;
> +	else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask))
> +		fwid |= (SMR_MASK_MASK & mask) << SMR_MASK_SHIFT;
> +
> +	return iommu_fwspec_add_ids(dev,
> +				    &fwid,
> +				    1);
> +}
> +
> static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
> {
> 	struct arm_smmu_device *smmu;
> @@ -2766,6 +2802,7 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
> static const struct iommu_ops arm_smmu_iommu_ops = {
>     .init = arm_smmu_iommu_domain_init,
>     .hwdom_init = arm_smmu_iommu_hwdom_init,
> +    .add_device = arm_smmu_dt_add_device_generic,
>     .teardown = arm_smmu_iommu_domain_teardown,
>     .iotlb_flush = arm_smmu_iotlb_flush,
>     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
> @@ -2773,9 +2810,10 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
>     .reassign_device = arm_smmu_reassign_dev,
>     .map_page = arm_iommu_map_page,
>     .unmap_page = arm_iommu_unmap_page,
> +    .dt_xlate = arm_smmu_dt_xlate_generic,
> };
> 
> -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;
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index a51ae3c9c3..ae07f272e1 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -162,22 +162,7 @@ int iommu_add_dt_device(struct dt_device_node *np)
>          * these callback implemented.
>          */
>         if ( !ops->add_device || !ops->dt_xlate )
> -        {
> -            /*
> -             * Some Device Trees may expose both legacy SMMU and generic
> -             * IOMMU bindings together. However, the SMMU driver is only
> -             * supporting the former and will protect them during the
> -             * initialization. So we need to skip them and not return
> -             * error here.
> -             *
> -             * XXX: This can be dropped when the SMMU is able to deal
> -             * with generic bindings.
> -             */
> -            if ( dt_device_is_protected(np) )
> -                return 0;
> -            else
> -                return -EINVAL;
> -        }
> +            return -EINVAL;
> 
>         if ( !dt_device_is_available(iommu_spec.np) )
>             break;
> -- 
> 2.17.1
> 

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Rahul Singh]

Hello Stefano,

> On 26 Jan 2021, at 10:58 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> Hi all,
> 
> This series introduces support for the generic SMMU bindings to
> xen/drivers/passthrough/arm/smmu.c.
> 
> The last version of the series was
> https://marc.info/?l=xen-devel&m=159539053406643
> 
> I realize that it is late for 4.15 -- I think it is OK if this series
> goes in afterwards.

I tested the series on the Juno board it is woking fine.  
I found one issue in SMMU driver while testing this series that is not related to this series but already existing issue in SMMU driver.

If there are more than one device behind SMMU and they share the same Stream-Id, SMMU driver is creating the new SMR entry without checking the already configured SMR entry if SMR entry correspond to stream-id is already configured.  Because of this I observed the stream match conflicts on Juno board.

(XEN) smmu: /iommu@7fb30000: Unexpected global fault, this could be serious
(XEN) smmu: /iommu@7fb30000: 	GFSR 0x00000004, GFSYNR0 0x00000006, GFSYNR1 0x00000000, GFSYNR2 0x00000000


Below two patches is required to be ported to Xen to fix the issue from Linux driver.

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=1f3d5ca43019bff1105838712d55be087d93c0da
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=21174240e4f4439bb8ed6c116cdbdc03eba2126e

Regards,
Rahul 
> 
> Cheers,
> 
> Stefano
> 
> 
> Brian Woods (3):
>      arm,smmu: switch to using iommu_fwspec functions
>      arm,smmu: restructure code in preparation to new bindings support
>      arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.
> 
> xen/drivers/passthrough/arm/smmu.c    | 162 ++++++++++++++++++++++++----------
> xen/drivers/passthrough/device_tree.c |  24 ++---
> 2 files changed, 123 insertions(+), 63 deletions(-)

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Stefano Stabellini]

On Tue, 2 Feb 2021, Rahul Singh wrote:
> Hello Stefano,
> 
> > On 26 Jan 2021, at 10:58 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > Hi all,
> > 
> > This series introduces support for the generic SMMU bindings to
> > xen/drivers/passthrough/arm/smmu.c.
> > 
> > The last version of the series was
> > https://marc.info/?l=xen-devel&m=159539053406643
> > 
> > I realize that it is late for 4.15 -- I think it is OK if this series
> > goes in afterwards.
> 
> I tested the series on the Juno board it is woking fine.  
> I found one issue in SMMU driver while testing this series that is not related to this series but already existing issue in SMMU driver.
> 
> If there are more than one device behind SMMU and they share the same Stream-Id, SMMU driver is creating the new SMR entry without checking the already configured SMR entry if SMR entry correspond to stream-id is already configured.  Because of this I observed the stream match conflicts on Juno board.
> 
> (XEN) smmu: /iommu@7fb30000: Unexpected global fault, this could be serious
> (XEN) smmu: /iommu@7fb30000: 	GFSR 0x00000004, GFSYNR0 0x00000006, GFSYNR1 0x00000000, GFSYNR2 0x00000000
> 
> 
> Below two patches is required to be ported to Xen to fix the issue from Linux driver.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=1f3d5ca43019bff1105838712d55be087d93c0da
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=21174240e4f4439bb8ed6c116cdbdc03eba2126e


Good catch and thanks for the pointers! Do you have any interest in
backporting these two patches or should I put them on my TODO list?

Unrelated to who does the job, we should discuss if it makes sense to
try to fix the bug for 4.15. The patches don't seem trivial so I am
tempted to say that it might be best to leave the bug unfixed for 4.15
and fix it later.

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Julien Grall]

Hi,

On 02/02/2021 17:44, Stefano Stabellini wrote:
> On Tue, 2 Feb 2021, Rahul Singh wrote:
>> Hello Stefano,
>>
>>> On 26 Jan 2021, at 10:58 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>
>>> Hi all,
>>>
>>> This series introduces support for the generic SMMU bindings to
>>> xen/drivers/passthrough/arm/smmu.c.
>>>
>>> The last version of the series was
>>> https://marc.info/?l=xen-devel&m=159539053406643
>>>
>>> I realize that it is late for 4.15 -- I think it is OK if this series
>>> goes in afterwards.
>>
>> I tested the series on the Juno board it is woking fine.
>> I found one issue in SMMU driver while testing this series that is not related to this series but already existing issue in SMMU driver.
>>
>> If there are more than one device behind SMMU and they share the same Stream-Id, SMMU driver is creating the new SMR entry without checking the already configured SMR entry if SMR entry correspond to stream-id is already configured.  Because of this I observed the stream match conflicts on Juno board.
>>
>> (XEN) smmu: /iommu@7fb30000: Unexpected global fault, this could be serious
>> (XEN) smmu: /iommu@7fb30000: 	GFSR 0x00000004, GFSYNR0 0x00000006, GFSYNR1 0x00000000, GFSYNR2 0x00000000
>>
>>
>> Below two patches is required to be ported to Xen to fix the issue from Linux driver.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=1f3d5ca43019bff1105838712d55be087d93c0da
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=21174240e4f4439bb8ed6c116cdbdc03eba2126e
> 
> 
> Good catch and thanks for the pointers! Do you have any interest in
> backporting these two patches or should I put them on my TODO list?
> 
> Unrelated to who does the job, we should discuss if it makes sense to
> try to fix the bug for 4.15. The patches don't seem trivial so I am
> tempted to say that it might be best to leave the bug unfixed for 4.15
> and fix it later.

SMMU support on Juno is not that interesting because IIRC the stream-ID 
is the same for all the devices. So it is all or nothing passthrough.

For other HW, this may be a useful feature. Yet we would need a way to 
group the devices for passthrough.

In this context, I would consider it more a feature than a bug because 
the SMMU driver never remotely work on such HW.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Stefano Stabellini]

On Tue, 2 Feb 2021, Julien Grall wrote:
> On 02/02/2021 17:44, Stefano Stabellini wrote:
> > On Tue, 2 Feb 2021, Rahul Singh wrote:
> > > Hello Stefano,
> > > 
> > > > On 26 Jan 2021, at 10:58 pm, Stefano Stabellini <sstabellini@kernel.org>
> > > > wrote:
> > > > 
> > > > Hi all,
> > > > 
> > > > This series introduces support for the generic SMMU bindings to
> > > > xen/drivers/passthrough/arm/smmu.c.
> > > > 
> > > > The last version of the series was
> > > > https://marc.info/?l=xen-devel&m=159539053406643
> > > > 
> > > > I realize that it is late for 4.15 -- I think it is OK if this series
> > > > goes in afterwards.
> > > 
> > > I tested the series on the Juno board it is woking fine.
> > > I found one issue in SMMU driver while testing this series that is not
> > > related to this series but already existing issue in SMMU driver.
> > > 
> > > If there are more than one device behind SMMU and they share the same
> > > Stream-Id, SMMU driver is creating the new SMR entry without checking the
> > > already configured SMR entry if SMR entry correspond to stream-id is
> > > already configured.  Because of this I observed the stream match conflicts
> > > on Juno board.
> > > 
> > > (XEN) smmu: /iommu@7fb30000: Unexpected global fault, this could be
> > > serious
> > > (XEN) smmu: /iommu@7fb30000: 	GFSR 0x00000004, GFSYNR0 0x00000006,
> > > GFSYNR1 0x00000000, GFSYNR2 0x00000000
> > > 
> > > 
> > > Below two patches is required to be ported to Xen to fix the issue from
> > > Linux driver.
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=1f3d5ca43019bff1105838712d55be087d93c0da
> > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=21174240e4f4439bb8ed6c116cdbdc03eba2126e
> > 
> > 
> > Good catch and thanks for the pointers! Do you have any interest in
> > backporting these two patches or should I put them on my TODO list?
> > 
> > Unrelated to who does the job, we should discuss if it makes sense to
> > try to fix the bug for 4.15. The patches don't seem trivial so I am
> > tempted to say that it might be best to leave the bug unfixed for 4.15
> > and fix it later.
> 
> SMMU support on Juno is not that interesting because IIRC the stream-ID is the
> same for all the devices. So it is all or nothing passthrough.
> 
> For other HW, this may be a useful feature. Yet we would need a way to group
> the devices for passthrough.
> 
> In this context, I would consider it more a feature than a bug because the
> SMMU driver never remotely work on such HW.

I see. To be honest I wasn't thinking of Juno (I wasn't aware of its
limitations) but of potential genuine situations where stream-ids are
the same for 2 devices. I know it can happen with PCI devices for
instance, although I am aware we don't have PCI passthrough yet. I don't
know if it is possible for it to happen with non-PCI devices but I
wouldn't be surprised if it can.

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Julien Grall]

Hi Stefano,

On 02/02/2021 18:27, Stefano Stabellini wrote:
> On Tue, 2 Feb 2021, Julien Grall wrote:
>> On 02/02/2021 17:44, Stefano Stabellini wrote:
>>> On Tue, 2 Feb 2021, Rahul Singh wrote:
>>>> Hello Stefano,
>>>>
>>>>> On 26 Jan 2021, at 10:58 pm, Stefano Stabellini <sstabellini@kernel.org>
>>>>> wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> This series introduces support for the generic SMMU bindings to
>>>>> xen/drivers/passthrough/arm/smmu.c.
>>>>>
>>>>> The last version of the series was
>>>>> https://marc.info/?l=xen-devel&m=159539053406643
>>>>>
>>>>> I realize that it is late for 4.15 -- I think it is OK if this series
>>>>> goes in afterwards.
>>>>
>>>> I tested the series on the Juno board it is woking fine.
>>>> I found one issue in SMMU driver while testing this series that is not
>>>> related to this series but already existing issue in SMMU driver.
>>>>
>>>> If there are more than one device behind SMMU and they share the same
>>>> Stream-Id, SMMU driver is creating the new SMR entry without checking the
>>>> already configured SMR entry if SMR entry correspond to stream-id is
>>>> already configured.  Because of this I observed the stream match conflicts
>>>> on Juno board.
>>>>
>>>> (XEN) smmu: /iommu@7fb30000: Unexpected global fault, this could be
>>>> serious
>>>> (XEN) smmu: /iommu@7fb30000: 	GFSR 0x00000004, GFSYNR0 0x00000006,
>>>> GFSYNR1 0x00000000, GFSYNR2 0x00000000
>>>>
>>>>
>>>> Below two patches is required to be ported to Xen to fix the issue from
>>>> Linux driver.
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=1f3d5ca43019bff1105838712d55be087d93c0da
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=21174240e4f4439bb8ed6c116cdbdc03eba2126e
>>>
>>>
>>> Good catch and thanks for the pointers! Do you have any interest in
>>> backporting these two patches or should I put them on my TODO list?
>>>
>>> Unrelated to who does the job, we should discuss if it makes sense to
>>> try to fix the bug for 4.15. The patches don't seem trivial so I am
>>> tempted to say that it might be best to leave the bug unfixed for 4.15
>>> and fix it later.
>>
>> SMMU support on Juno is not that interesting because IIRC the stream-ID is the
>> same for all the devices. So it is all or nothing passthrough.
>>
>> For other HW, this may be a useful feature. Yet we would need a way to group
>> the devices for passthrough.
>>
>> In this context, I would consider it more a feature than a bug because the
>> SMMU driver never remotely work on such HW.
> 
> I see. To be honest I wasn't thinking of Juno (I wasn't aware of its
> limitations) but of potential genuine situations where stream-ids are
> the same for 2 devices. I know it can happen with PCI devices for
> instance, although I am aware we don't have PCI passthrough yet. I don't
> know if it is possible for it to happen with non-PCI devices but I
> wouldn't be surprised if it can.

I merely pointed out Juno because this is where the discussion started. 
Although, my conclusion wasn't solely based on this system nor PCI devices.

It was based on the fact that this could never have worked with the 
current SMMU driver. So this is not a regression and more an improvement 
of the driver to support passthrough for devices using the same stream-ID.

At this stage of the release, I would only consider trivial improvement 
to be merged.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Rahul Singh]

Hello Stefano,

> On 2 Feb 2021, at 5:44 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 2 Feb 2021, Rahul Singh wrote:
>> Hello Stefano,
>> 
>>> On 26 Jan 2021, at 10:58 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> Hi all,
>>> 
>>> This series introduces support for the generic SMMU bindings to
>>> xen/drivers/passthrough/arm/smmu.c.
>>> 
>>> The last version of the series was
>>> https://marc.info/?l=xen-devel&m=159539053406643
>>> 
>>> I realize that it is late for 4.15 -- I think it is OK if this series
>>> goes in afterwards.
>> 
>> I tested the series on the Juno board it is woking fine.  
>> I found one issue in SMMU driver while testing this series that is not related to this series but already existing issue in SMMU driver.
>> 
>> If there are more than one device behind SMMU and they share the same Stream-Id, SMMU driver is creating the new SMR entry without checking the already configured SMR entry if SMR entry correspond to stream-id is already configured.  Because of this I observed the stream match conflicts on Juno board.
>> 
>> (XEN) smmu: /iommu@7fb30000: Unexpected global fault, this could be serious
>> (XEN) smmu: /iommu@7fb30000: 	GFSR 0x00000004, GFSYNR0 0x00000006, GFSYNR1 0x00000000, GFSYNR2 0x00000000
>> 
>> 
>> Below two patches is required to be ported to Xen to fix the issue from Linux driver.
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=1f3d5ca43019bff1105838712d55be087d93c0da
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=21174240e4f4439bb8ed6c116cdbdc03eba2126e
> 
> 
> Good catch and thanks for the pointers! Do you have any interest in
> backporting these two patches or should I put them on my TODO list?

Yes I am happy to backport these patches to XEN. 
I will send the patch for review once 4.15 release branch out from master.
 
Regards,
Rahul
> 
> Unrelated to who does the job, we should discuss if it makes sense to
> try to fix the bug for 4.15. The patches don't seem trivial so I am
> tempted to say that it might be best to leave the bug unfixed for 4.15
> and fix it later.

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Julien Grall]

On 26/01/2021 22:58, Stefano Stabellini wrote:
> Hi all,

Hi Stefano,

> This series introduces support for the generic SMMU bindings to
> xen/drivers/passthrough/arm/smmu.c.
> 
> The last version of the series was
> https://marc.info/?l=xen-devel&m=159539053406643
Some changes in the SMMU drivers went in recently. I believe this 
touched similar area to this series. Would you be able to check that 
this series still work as intented?

Cheers,

-- 
Julien Grall

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Stefano Stabellini]

On Mon, 5 Apr 2021, Julien Grall wrote:
> On 26/01/2021 22:58, Stefano Stabellini wrote:
> > Hi all,
> 
> Hi Stefano,
> 
> > This series introduces support for the generic SMMU bindings to
> > xen/drivers/passthrough/arm/smmu.c.
> > 
> > The last version of the series was
> > https://marc.info/?l=xen-devel&m=159539053406643
> Some changes in the SMMU drivers went in recently. I believe this touched
> similar area to this series. Would you be able to check that this series still
> work as intented?

Thanks for the heads up, no, unfortunately they don't work :-(

They badly clash. I did the forward port of the three patches but they
fail at runtime in my tests. I ran out of time to figure out what is the
problem, and I'll have to pick it up at some point in the future (it
might not be any time soon unfortunately).

Rahul, if you have any ideas about what the problem is please let me
know. This is the branch with the forward port:

http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Rahul Singh]

Hi Stefano,

> On 7 Apr 2021, at 12:59 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Mon, 5 Apr 2021, Julien Grall wrote:
>> On 26/01/2021 22:58, Stefano Stabellini wrote:
>>> Hi all,
>> 
>> Hi Stefano,
>> 
>>> This series introduces support for the generic SMMU bindings to
>>> xen/drivers/passthrough/arm/smmu.c.
>>> 
>>> The last version of the series was
>>> https://marc.info/?l=xen-devel&m=159539053406643
>> Some changes in the SMMU drivers went in recently. I believe this touched
>> similar area to this series. Would you be able to check that this series still
>> work as intented?
> 
> Thanks for the heads up, no, unfortunately they don't work :-(
> 
> They badly clash. I did the forward port of the three patches but they
> fail at runtime in my tests. I ran out of time to figure out what is the
> problem, and I'll have to pick it up at some point in the future (it
> might not be any time soon unfortunately).
> 
> Rahul, if you have any ideas about what the problem is please let me
> know. This is the branch with the forward port:

Let me check and come back to you if I found out anything regarding the same. 

Regards,
Rahul
> 
> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Stefano Stabellini]

On Tue, 6 Apr 2021, Stefano Stabellini wrote:
> On Mon, 5 Apr 2021, Julien Grall wrote:
> > On 26/01/2021 22:58, Stefano Stabellini wrote:
> > > Hi all,
> > 
> > Hi Stefano,
> > 
> > > This series introduces support for the generic SMMU bindings to
> > > xen/drivers/passthrough/arm/smmu.c.
> > > 
> > > The last version of the series was
> > > https://marc.info/?l=xen-devel&m=159539053406643
> > Some changes in the SMMU drivers went in recently. I believe this touched
> > similar area to this series. Would you be able to check that this series still
> > work as intented?
> 
> Thanks for the heads up, no, unfortunately they don't work :-(
> 
> They badly clash. I did the forward port of the three patches but they
> fail at runtime in my tests. I ran out of time to figure out what is the
> problem, and I'll have to pick it up at some point in the future (it
> might not be any time soon unfortunately).
> 
> Rahul, if you have any ideas about what the problem is please let me
> know. This is the branch with the forward port:
> 
> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic

I did some more investigation and spotted a minor error in my forward
port. This an updated branch based on staging:

http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic-2

However, the real issue is that Rahul's patches break SMMU support on
ZynqMP even without my changes. I ran a bisection and found out that
patch #2 is the culprit:

5ee3fa0b21ea xen/arm: smmuv1: Consolidate stream map entry state

It causes the abort appended below. The problem doesn't seem
particularly ZynqMP specific. Rahul, can you reproduce it on your side?



(XEN) smmu: /amba/smmu@fd800000: d0: p2maddr 0x000000087bfa2000
(XEN) Data Abort Trap. Syndrome=0x5
(XEN) Walking Hypervisor VA 0x114ebfff8 on CPU0 via TTBR 0x0000000000f38000
(XEN) 0TH[0x0] = 0x0000000000f3bf7f
(XEN) 1ST[0x4] = 0x0000000000000000
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.14.0  arm64  debug=y   Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     000000000024a77c smmu.c#find_smmu_master+0x8/0x3c
(XEN) LR:     000000000024a8a4
(XEN) SP:     00000000002ff1f0
(XEN) CPSR:   80000249 MODE:64-bit EL2h (Hypervisor, handler)
(XEN)      X0: 0000000114ec0000  X1: 00008000fbfc7478  X2: 00008000fbfc71e0
(XEN)      X3: 00000000002af840  X4: 0000000000000000  X5: 0000000000000001
(XEN)      X6: 0000000000000000  X7: 0000000000000000  X8: 00008000fbf8b9e0
(XEN)      X9: 0000000000000004 X10: 0101010101010101 X11: 0000000000000020
(XEN)     X12: 0000000000000018 X13: ff00000000000000 X14: 0400000084000000
(XEN)     X15: 0000000000000000 X16: 00000000002b1000 X17: 00000000002b1000
(XEN)     X18: 00000000002b2000 X19: 00008000fbffcb70 X20: 00000000002af848
(XEN)     X21: 00008000fbfc7478 X22: 00008000fbfc74d8 X23: 00008000fbfc7508
(XEN)     X24: 0000000000000000 X25: 0000000000000001 X26: 00008000fbfa7c20
(XEN)     X27: 0000000000000000 X28: 0000000000000000  FP: 00000000002ff1f0
(XEN) 
(XEN)   VTCR_EL2: 80023558
(XEN)  VTTBR_EL2: 000000087bf54000
(XEN) 
(XEN)  SCTLR_EL2: 30cd183d
(XEN)    HCR_EL2: 000000000000003a
(XEN)  TTBR0_EL2: 0000000000f38000
(XEN) 
(XEN)    ESR_EL2: 96000005
(XEN)  HPFAR_EL2: 0000000000220000
(XEN)    FAR_EL2: 0000000114ebfff8
(XEN) 
(XEN) Xen stack trace from sp=00000000002ff1f0:
(XEN)    00000000002ff220 000000000024ae80 00008000fbfa5000 00008000fbfc74e8
(XEN)    00008000fbfa5000 0000000800000001 00000000002ff2a0 000000000024c6e8
(XEN)    00008000fbfa5000 00000000fffffff0 00008000fbfc7478 00008000fbfc74d8
(XEN)    00008000fbfc7508 0000000000000000 0000000000000001 0000000000000001
(XEN)    0000000000000000 0000000000000000 00000000002ff2a0 000000000024c6b8
(XEN)    00008000fbfa5000 00000000002ff550 00000000002ff2d0 00000000002c6274
(XEN)    00008000fbfc7478 00000000002ff550 00008000fbfa5000 0000000000000005
(XEN)    00000000002ff390 00000000002c6704 00008000fbfc3ce8 00000000002ff550
(XEN)    00008000fbfa5000 0000000000000005 00008000fbfc7478 0000000000000000
(XEN)    00008000fbff1100 0000000000000000 0000000000000000 0000000000000000
(XEN)    00000000002d28e8 00000000fbf78090 00000000002d28d8 00000000002d1b80
(XEN)    00000000002ff380 00000000ff0b0000 00008000fbfc3ce8 0000000000001000
(XEN)    00008000fbfa5000 00008000fbfa5000 0000000000000005 00000000002c6600
(XEN)    00000000002ff450 00000000002c6704 00008000fbfc0000 00000000002ff550
(XEN)    00008000fbfa5000 0000000000000005 00008000fbfc3ce8 0000000000000000
(XEN)    00008000fbff00a8 0000000000000013 0000000000000000 0000000000000000
(XEN)    00000000002d28e8 00000000fbf78090 00000000002d28d8 00000000002d1b80
(XEN)    00000000002ff440 00000000ff990000 00008000fbfc0000 0000000000001000
(XEN)    00008000fbfa5000 00008000fbfa5000 0000000000000005 00000000002c6600
(XEN)    00000000002ff510 00000000002c6f78 0000000000008090 0000000000e00000
(XEN)    00000000002d2ae8 00008000fbfa5000 000000000000000f 0000000000000004
(XEN)    00000000002e05e0 0000000000000000 0000000880000000 0000000000000002
(XEN)    00000000002d28e8 000000000022d1e4 00000000002d28d8 00000000002d1b80
(XEN)    00000000002ff500 00000000002b7da0 0000000000008090 0000000000e00000
(XEN)    00000000002d2ae8 00008000fbfa5000 0000000000000005 00000000002c6f60
(XEN)    00000000002ffdf0 00000000002cb1fc 00008000fbfa5000 00000000002b0600
(XEN)    0000000000340430 0000000000000004 00000000002a3810 0000000000000000
(XEN)    0000000000000001 00008000fbfa5000 00008000fbf70000 0000000000000000
(XEN)    0000000000000001 0000000020000000 0000000040000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<000000000024a77c>] smmu.c#find_smmu_master+0x8/0x3c (PC)
(XEN)    [<000000000024a8a4>] smmu.c#find_smmu_for_device+0x48/0x94 (LR)
(XEN)    [<000000000024ae80>] smmu.c#arm_smmu_assign_dev+0x58/0xb48
(XEN)    [<000000000024c6e8>] iommu_assign_dt_device+0x64/0xc0
(XEN)    [<00000000002c6274>] domain_build.c#handle_node+0x310/0x9ec
(XEN)    [<00000000002c6704>] domain_build.c#handle_node+0x7a0/0x9ec
(XEN)    [<00000000002c6704>] domain_build.c#handle_node+0x7a0/0x9ec
(XEN)    [<00000000002c6f78>] construct_dom0+0x410/0x4bc
(XEN)    [<00000000002cb1fc>] start_xen+0xb9c/0xca4
(XEN)    [<00000000002001a0>] arm64/head.o#primary_switched+0xc/0x1c

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Rahul Singh]

[-- Attachment #1.1: Type: text/plain, Size: 8324 bytes --]

Hi Stefano,

> On 10 Apr 2021, at 1:27 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Tue, 6 Apr 2021, Stefano Stabellini wrote:
>> On Mon, 5 Apr 2021, Julien Grall wrote:
>>> On 26/01/2021 22:58, Stefano Stabellini wrote:
>>>> Hi all,
>>>
>>> Hi Stefano,
>>>
>>>> This series introduces support for the generic SMMU bindings to
>>>> xen/drivers/passthrough/arm/smmu.c.
>>>>
>>>> The last version of the series was
>>>> https://marc.info/?l=xen-devel&m=159539053406643
>>> Some changes in the SMMU drivers went in recently. I believe this touched
>>> similar area to this series. Would you be able to check that this series still
>>> work as intented?
>>
>> Thanks for the heads up, no, unfortunately they don't work :-(
>>
>> They badly clash. I did the forward port of the three patches but they
>> fail at runtime in my tests. I ran out of time to figure out what is the
>> problem, and I'll have to pick it up at some point in the future (it
>> might not be any time soon unfortunately).
>>
>> Rahul, if you have any ideas about what the problem is please let me
>> know. This is the branch with the forward port:
>>
>> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic
>
> I did some more investigation and spotted a minor error in my forward
> port. This an updated branch based on staging:
>
> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic-2
>
> However, the real issue is that Rahul's patches break SMMU support on
> ZynqMP even without my changes. I ran a bisection and found out that
> patch #2 is the culprit:
>
> 5ee3fa0b21ea xen/arm: smmuv1: Consolidate stream map entry state
>
> It causes the abort appended below. The problem doesn't seem
> particularly ZynqMP specific. Rahul, can you reproduce it on your side?

Yes. I can reproduce the issue on Xilinx QEMU as we don’t have access to physical ZynqMP and found out that
associating an iommu group pointer with the S2CR causing the issue.

Associating the group pointer with S2CR is part of the patch "xen/arm: smmuv1: Intelligent SMR allocation”.

I just revert that part of the code from the patch and it works fine for me. Please find the attached patch for the same.

As per your analysis "5ee3fa0b21ea xen/arm: smmuv1: Consolidate stream map entry state” is causing the issue but what I found out that
"xen/arm: smmuv1: Intelligent SMR allocation” is causing the issue.
Can you please test it on the physical device and let me know if it works for you also to make sure we both observing the same issue.



Regards,
Rahul

>
>
> (XEN) smmu: /amba/smmu@fd800000: d0: p2maddr 0x000000087bfa2000
> (XEN) Data Abort Trap. Syndrome=0x5
> (XEN) Walking Hypervisor VA 0x114ebfff8 on CPU0 via TTBR 0x0000000000f38000
> (XEN) 0TH[0x0] = 0x0000000000f3bf7f
> (XEN) 1ST[0x4] = 0x0000000000000000
> (XEN) CPU0: Unexpected Trap: Data Abort
> (XEN) ----[ Xen-4.14.0  arm64  debug=y   Not tainted ]----
> (XEN) CPU:    0
> (XEN) PC:     000000000024a77c smmu.c#find_smmu_master+0x8/0x3c
> (XEN) LR:     000000000024a8a4
> (XEN) SP:     00000000002ff1f0
> (XEN) CPSR:   80000249 MODE:64-bit EL2h (Hypervisor, handler)
> (XEN)      X0: 0000000114ec0000  X1: 00008000fbfc7478  X2: 00008000fbfc71e0
> (XEN)      X3: 00000000002af840  X4: 0000000000000000  X5: 0000000000000001
> (XEN)      X6: 0000000000000000  X7: 0000000000000000  X8: 00008000fbf8b9e0
> (XEN)      X9: 0000000000000004 X10: 0101010101010101 X11: 0000000000000020
> (XEN)     X12: 0000000000000018 X13: ff00000000000000 X14: 0400000084000000
> (XEN)     X15: 0000000000000000 X16: 00000000002b1000 X17: 00000000002b1000
> (XEN)     X18: 00000000002b2000 X19: 00008000fbffcb70 X20: 00000000002af848
> (XEN)     X21: 00008000fbfc7478 X22: 00008000fbfc74d8 X23: 00008000fbfc7508
> (XEN)     X24: 0000000000000000 X25: 0000000000000001 X26: 00008000fbfa7c20
> (XEN)     X27: 0000000000000000 X28: 0000000000000000  FP: 00000000002ff1f0
> (XEN)
> (XEN)   VTCR_EL2: 80023558
> (XEN)  VTTBR_EL2: 000000087bf54000
> (XEN)
> (XEN)  SCTLR_EL2: 30cd183d
> (XEN)    HCR_EL2: 000000000000003a
> (XEN)  TTBR0_EL2: 0000000000f38000
> (XEN)
> (XEN)    ESR_EL2: 96000005
> (XEN)  HPFAR_EL2: 0000000000220000
> (XEN)    FAR_EL2: 0000000114ebfff8
> (XEN)
> (XEN) Xen stack trace from sp=00000000002ff1f0:
> (XEN)    00000000002ff220 000000000024ae80 00008000fbfa5000 00008000fbfc74e8
> (XEN)    00008000fbfa5000 0000000800000001 00000000002ff2a0 000000000024c6e8
> (XEN)    00008000fbfa5000 00000000fffffff0 00008000fbfc7478 00008000fbfc74d8
> (XEN)    00008000fbfc7508 0000000000000000 0000000000000001 0000000000000001
> (XEN)    0000000000000000 0000000000000000 00000000002ff2a0 000000000024c6b8
> (XEN)    00008000fbfa5000 00000000002ff550 00000000002ff2d0 00000000002c6274
> (XEN)    00008000fbfc7478 00000000002ff550 00008000fbfa5000 0000000000000005
> (XEN)    00000000002ff390 00000000002c6704 00008000fbfc3ce8 00000000002ff550
> (XEN)    00008000fbfa5000 0000000000000005 00008000fbfc7478 0000000000000000
> (XEN)    00008000fbff1100 0000000000000000 0000000000000000 0000000000000000
> (XEN)    00000000002d28e8 00000000fbf78090 00000000002d28d8 00000000002d1b80
> (XEN)    00000000002ff380 00000000ff0b0000 00008000fbfc3ce8 0000000000001000
> (XEN)    00008000fbfa5000 00008000fbfa5000 0000000000000005 00000000002c6600
> (XEN)    00000000002ff450 00000000002c6704 00008000fbfc0000 00000000002ff550
> (XEN)    00008000fbfa5000 0000000000000005 00008000fbfc3ce8 0000000000000000
> (XEN)    00008000fbff00a8 0000000000000013 0000000000000000 0000000000000000
> (XEN)    00000000002d28e8 00000000fbf78090 00000000002d28d8 00000000002d1b80
> (XEN)    00000000002ff440 00000000ff990000 00008000fbfc0000 0000000000001000
> (XEN)    00008000fbfa5000 00008000fbfa5000 0000000000000005 00000000002c6600
> (XEN)    00000000002ff510 00000000002c6f78 0000000000008090 0000000000e00000
> (XEN)    00000000002d2ae8 00008000fbfa5000 000000000000000f 0000000000000004
> (XEN)    00000000002e05e0 0000000000000000 0000000880000000 0000000000000002
> (XEN)    00000000002d28e8 000000000022d1e4 00000000002d28d8 00000000002d1b80
> (XEN)    00000000002ff500 00000000002b7da0 0000000000008090 0000000000e00000
> (XEN)    00000000002d2ae8 00008000fbfa5000 0000000000000005 00000000002c6f60
> (XEN)    00000000002ffdf0 00000000002cb1fc 00008000fbfa5000 00000000002b0600
> (XEN)    0000000000340430 0000000000000004 00000000002a3810 0000000000000000
> (XEN)    0000000000000001 00008000fbfa5000 00008000fbf70000 0000000000000000
> (XEN)    0000000000000001 0000000020000000 0000000040000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) Xen call trace:
> (XEN)    [<000000000024a77c>] smmu.c#find_smmu_master+0x8/0x3c (PC)
> (XEN)    [<000000000024a8a4>] smmu.c#find_smmu_for_device+0x48/0x94 (LR)
> (XEN)    [<000000000024ae80>] smmu.c#arm_smmu_assign_dev+0x58/0xb48
> (XEN)    [<000000000024c6e8>] iommu_assign_dt_device+0x64/0xc0
> (XEN)    [<00000000002c6274>] domain_build.c#handle_node+0x310/0x9ec
> (XEN)    [<00000000002c6704>] domain_build.c#handle_node+0x7a0/0x9ec
> (XEN)    [<00000000002c6704>] domain_build.c#handle_node+0x7a0/0x9ec
> (XEN)    [<00000000002c6f78>] construct_dom0+0x410/0x4bc
> (XEN)    [<00000000002cb1fc>] start_xen+0xb9c/0xca4
> (XEN)    [<00000000002001a0>] arm64/head.o#primary_switched+0xc/0x1c


[-- Attachment #1.2: Type: text/html, Size: 11373 bytes --]

[-- Attachment #2: 0001-xen-arm-smmuv1-Revert-associating-the-group-pointer-.patch --]
[-- Type: application/octet-stream, Size: 2959 bytes --]

From 02278fa0e4abd41e5c5e253fe23b604a4f081105 Mon Sep 17 00:00:00 2001
Message-Id: <02278fa0e4abd41e5c5e253fe23b604a4f081105.1618222589.git.rahul.singh@arm.com>
From: Rahul Singh <rahul.singh@arm.com>
Date: Mon, 12 Apr 2021 09:50:05 +0100
Subject: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with
 the S2CR

Revert the code that associate the group pointer with the S2CR as this
causing an issue when SMMU device has more that one master device.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 44 +++---------------------------
 1 file changed, 4 insertions(+), 40 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 20ac672e91..3456daa03f 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -597,7 +597,6 @@ enum arm_smmu_arch_version {
 };
 
 struct arm_smmu_s2cr {
-	struct iommu_group		*group;
 	int				count;
 	enum arm_smmu_s2cr_type		type;
 	enum arm_smmu_s2cr_privcfg	privcfg;
@@ -1498,7 +1497,6 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
 	struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
 	struct arm_smmu_device *smmu = cfg->smmu;
 	struct arm_smmu_smr *smrs = smmu->smrs;
-	struct iommu_group *group;
 	int i, idx, ret;
 
 	spin_lock(&smmu->stream_map_lock);
@@ -1523,19 +1521,9 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
 		cfg->smendx[i] = (s16)idx;
 	}
 
-	group = iommu_group_get(dev);
-	if (!group)
-		group = ERR_PTR(-ENOMEM);
-	if (IS_ERR(group)) {
-		ret = PTR_ERR(group);
-		goto out_err;
-	}
-	iommu_group_put(group);
-
 	/* It worked! Now, poke the actual hardware */
 	for_each_cfg_sme(cfg, i, idx) {
 		arm_smmu_write_sme(smmu, idx);
-		smmu->s2crs[idx].group = group;
 	}
 
 	spin_unlock(&smmu->stream_map_lock);
@@ -1966,27 +1954,6 @@ static void __arm_smmu_release_pci_iommudata(void *data)
 	kfree(data);
 }
 
-static struct iommu_group *arm_smmu_device_group(struct
-						arm_smmu_master_cfg *cfg)
-{
-	struct arm_smmu_device *smmu = cfg->smmu;
-	struct iommu_group *group = NULL;
-	int i, idx;
-
-	for_each_cfg_sme(cfg, i, idx) {
-		if (group && smmu->s2crs[idx].group &&
-		    group != smmu->s2crs[idx].group)
-			return ERR_PTR(-EINVAL);
-
-		group = smmu->s2crs[idx].group;
-	}
-
-	if (group)
-		return group;
-
-	return NULL;
-}
-
 static int arm_smmu_add_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
@@ -2027,13 +1994,10 @@ static int arm_smmu_add_device(struct device *dev)
 		cfg->smmu = smmu;
 	}
 
-	group = arm_smmu_device_group(cfg);
-	if (!group) {
-		group = iommu_group_alloc();
-		if (IS_ERR(group)) {
-			dev_err(dev, "Failed to allocate IOMMU group\n");
-			return PTR_ERR(group);
-		}
+	group = iommu_group_alloc();
+	if (IS_ERR(group)) {
+		dev_err(dev, "Failed to allocate IOMMU group\n");
+		return PTR_ERR(group);
 	}
 
 	iommu_group_set_iommudata(group, cfg, releasefn);
-- 
2.17.1

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3226 bytes --]

On Mon, 12 Apr 2021, Rahul Singh wrote:
> Hi Stefano,
> 
> > On 10 Apr 2021, at 1:27 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >
> > On Tue, 6 Apr 2021, Stefano Stabellini wrote:
> >> On Mon, 5 Apr 2021, Julien Grall wrote:
> >>> On 26/01/2021 22:58, Stefano Stabellini wrote:
> >>>> Hi all,
> >>>
> >>> Hi Stefano,
> >>>
> >>>> This series introduces support for the generic SMMU bindings to
> >>>> xen/drivers/passthrough/arm/smmu.c.
> >>>>
> >>>> The last version of the series was
> >>>> https://marc.info/?l=xen-devel&m=159539053406643
> >>> Some changes in the SMMU drivers went in recently. I believe this touched
> >>> similar area to this series. Would you be able to check that this series still
> >>> work as intented?
> >>
> >> Thanks for the heads up, no, unfortunately they don't work :-(
> >>
> >> They badly clash. I did the forward port of the three patches but they
> >> fail at runtime in my tests. I ran out of time to figure out what is the
> >> problem, and I'll have to pick it up at some point in the future (it
> >> might not be any time soon unfortunately).
> >>
> >> Rahul, if you have any ideas about what the problem is please let me
> >> know. This is the branch with the forward port:
> >>
> >> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic
> >
> > I did some more investigation and spotted a minor error in my forward
> > port. This an updated branch based on staging:
> >
> > http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic-2
> >
> > However, the real issue is that Rahul's patches break SMMU support on
> > ZynqMP even without my changes. I ran a bisection and found out that
> > patch #2 is the culprit:
> >
> > 5ee3fa0b21ea xen/arm: smmuv1: Consolidate stream map entry state
> >
> > It causes the abort appended below. The problem doesn't seem
> > particularly ZynqMP specific. Rahul, can you reproduce it on your side?
> 
> Yes. I can reproduce the issue on Xilinx QEMU as we don’t have access to physical ZynqMP and found out that
> associating an iommu group pointer with the S2CR causing the issue.
> 
> Associating the group pointer with S2CR is part of the patch "xen/arm: smmuv1: Intelligent SMR allocation”.
> 
> I just revert that part of the code from the patch and it works fine for me. Please find the attached patch for the same.
> 
> As per your analysis "5ee3fa0b21ea xen/arm: smmuv1: Consolidate stream map entry state” is causing the issue but what I found out that
> "xen/arm: smmuv1: Intelligent SMR allocation” is causing the issue.
> Can you please test it on the physical device and let me know if it works for you also to make sure we both observing the same issue.

Great! Yes, I can confirm that your patch fixed the issue, now I can
boot staging on ZynqMP without errors and I can do device assignment
too. Thank you so much!

The other good news is that the three "Generic SMMU Bindings" patches
work too on top of yours with the fix!

Is the patch you submitted the valid fix for the problem? In other words,
should we go ahead, review, and commit the patch you attached or do you
want to send a different version of the patch for inclusion in Xen
staging?

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Rahul Singh]

HI Stefano,

> On 13 Apr 2021, at 1:27 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Mon, 12 Apr 2021, Rahul Singh wrote:
>> Hi Stefano,
>> 
>>> On 10 Apr 2021, at 1:27 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Tue, 6 Apr 2021, Stefano Stabellini wrote:
>>>> On Mon, 5 Apr 2021, Julien Grall wrote:
>>>>> On 26/01/2021 22:58, Stefano Stabellini wrote:
>>>>>> Hi all,
>>>>> 
>>>>> Hi Stefano,
>>>>> 
>>>>>> This series introduces support for the generic SMMU bindings to
>>>>>> xen/drivers/passthrough/arm/smmu.c.
>>>>>> 
>>>>>> The last version of the series was
>>>>>> https://marc.info/?l=xen-devel&m=159539053406643
>>>>> Some changes in the SMMU drivers went in recently. I believe this touched
>>>>> similar area to this series. Would you be able to check that this series still
>>>>> work as intented?
>>>> 
>>>> Thanks for the heads up, no, unfortunately they don't work :-(
>>>> 
>>>> They badly clash. I did the forward port of the three patches but they
>>>> fail at runtime in my tests. I ran out of time to figure out what is the
>>>> problem, and I'll have to pick it up at some point in the future (it
>>>> might not be any time soon unfortunately).
>>>> 
>>>> Rahul, if you have any ideas about what the problem is please let me
>>>> know. This is the branch with the forward port:
>>>> 
>>>> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic
>>> 
>>> I did some more investigation and spotted a minor error in my forward
>>> port. This an updated branch based on staging:
>>> 
>>> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic-2
>>> 
>>> However, the real issue is that Rahul's patches break SMMU support on
>>> ZynqMP even without my changes. I ran a bisection and found out that
>>> patch #2 is the culprit:
>>> 
>>> 5ee3fa0b21ea xen/arm: smmuv1: Consolidate stream map entry state
>>> 
>>> It causes the abort appended below. The problem doesn't seem
>>> particularly ZynqMP specific. Rahul, can you reproduce it on your side?
>> 
>> Yes. I can reproduce the issue on Xilinx QEMU as we don’t have access to physical ZynqMP and found out that
>> associating an iommu group pointer with the S2CR causing the issue.
>> 
>> Associating the group pointer with S2CR is part of the patch "xen/arm: smmuv1: Intelligent SMR allocation”.
>> 
>> I just revert that part of the code from the patch and it works fine for me. Please find the attached patch for the same.
>> 
>> As per your analysis "5ee3fa0b21ea xen/arm: smmuv1: Consolidate stream map entry state” is causing the issue but what I found out that
>> "xen/arm: smmuv1: Intelligent SMR allocation” is causing the issue.
>> Can you please test it on the physical device and let me know if it works for you also to make sure we both observing the same issue.
> 
> Great! Yes, I can confirm that your patch fixed the issue, now I can
> boot staging on ZynqMP without errors and I can do device assignment
> too. Thank you so much!
> 
> The other good news is that the three "Generic SMMU Bindings" patches
> work too on top of yours with the fix!
> 
> Is the patch you submitted the valid fix for the problem? In other words,
> should we go ahead, review, and commit the patch you attached or do you
> want to send a different version of the patch for inclusion in Xen
> staging?

Thank you for testing the patch. Patch as it is ok for review. I will send the patch for review.

Regards,
Rahul