[RFC v2 0/2] Generic SMMU Bindings

[RFC v2 0/2] Generic SMMU Bindings

[Brian Woods]

Finally had time to do a v2.  Changes are in each patch.

Brian Woods (2):
  arm,smmu: switch to using iommu_fwspec functions
  arm,smmu: add support for generic DT bindings

 xen/drivers/passthrough/arm/smmu.c    | 147 ++++++++++++++++++++++++----------
 xen/drivers/passthrough/device_tree.c |  20 +----
 2 files changed, 107 insertions(+), 60 deletions(-)

-- 
2.7.4

[RFC v2 1/2] arm,smmu: switch to using iommu_fwspec functions

[Brian Woods]

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.

Signed-off-by: Brian Woods <brian.woods@xilinx.com>
---

Interested in if combining the legacy and generic bindings paths are
worth or if Xen plans to depreicate legacy bindings at some point.

v1 -> v2
    - removed MAX_MASTER_STREAMIDS
    - removed unneeded curly brackets

 xen/drivers/passthrough/arm/smmu.c    | 81 +++++++++++++++++++----------------
 xen/drivers/passthrough/device_tree.c |  3 ++
 2 files changed, 47 insertions(+), 37 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 94662a8..7a5c6cd 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -49,6 +49,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 +303,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 +595,7 @@ struct arm_smmu_smr {
 };
 
 struct arm_smmu_master_cfg {
-	int				num_streamids;
-	u16				streamids[MAX_MASTER_STREAMIDS];
+	struct iommu_fwspec		*fwspec;
 	struct arm_smmu_smr		*smrs;
 };
 
@@ -779,7 +776,7 @@ 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;
 
 	master = find_smmu_master(smmu, masterspec->np);
@@ -790,34 +787,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;
+	}
+	master->cfg.fwspec = dev_iommu_fwspec_get(&master->of_node->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 < master->cfg.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);
 }
@@ -1397,15 +1397,15 @@ 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(cfg->fwspec->num_ids, sizeof(*smrs), GFP_KERNEL);
 	if (!smrs) {
 		dev_err(smmu->dev, "failed to allocate %d SMRs\n",
-			cfg->num_streamids);
+			cfg->fwspec->num_ids);
 		return -ENOMEM;
 	}
 
 	/* Allocate the SMRs on the SMMU */
-	for (i = 0; i < cfg->num_streamids; ++i) {
+	for (i = 0; i < cfg->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 +1416,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	= cfg->fwspec->ids[i],
 		};
 	}
 
 	/* It worked! Now, poke the actual hardware */
-	for (i = 0; i < cfg->num_streamids; ++i) {
+	for (i = 0; i < cfg->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));
@@ -1448,7 +1448,7 @@ static void arm_smmu_master_free_smrs(struct arm_smmu_device *smmu,
 		return;
 
 	/* Invalidate the SMRs before freeing back to the allocator */
-	for (i = 0; i < cfg->num_streamids; ++i) {
+	for (i = 0; i < cfg->fwspec->num_ids; ++i) {
 		u8 idx = smrs[i].idx;
 
 		writel_relaxed(~SMR_VALID, gr0_base + ARM_SMMU_GR0_SMR(idx));
@@ -1471,10 +1471,10 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	if (ret)
 		return ret == -EEXIST ? 0 : ret;
 
-	for (i = 0; i < cfg->num_streamids; ++i) {
+	for (i = 0; i < cfg->fwspec->num_ids; ++i) {
 		u32 idx, s2cr;
 
-		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
+		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->fwspec->ids[i];
 		s2cr = S2CR_TYPE_TRANS |
 		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
@@ -1499,8 +1499,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 < cfg->fwspec->num_ids; ++i) {
+		u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->fwspec->ids[i];
 
 		writel_relaxed(S2CR_TYPE_FAULT,
 			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
@@ -1924,14 +1924,21 @@ static int arm_smmu_add_device(struct device *dev)
 			ret = -ENOMEM;
 			goto out_put_group;
 		}
+		cfg->fwspec = kzalloc(sizeof(struct iommu_fwspec), GFP_KERNEL);
+		if (!cfg->fwspec) {
+			kfree(cfg);
+			ret = -ENOMEM;
+			goto out_put_group;
+		}
+		iommu_fwspec_init(dev, smmu->dev);
 
-		cfg->num_streamids = 1;
+		cfg->fwspec->num_ids = 1;
 		/*
 		 * 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]);
+				       &cfg->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 999b831..acf6b62 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -140,6 +140,9 @@ int iommu_add_dt_device(struct dt_device_node *np)
     if ( !ops )
         return -EINVAL;
 
+    if ( dt_device_is_protected(np) )
+        return 0;
+
     if ( dev_iommu_fwspec_get(dev) )
         return -EEXIST;
 
-- 
2.7.4

[RFC v2 2/2] arm,smmu: add support for generic DT bindings

[Brian Woods]

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

Just realized that I'm fairly sure I need to do some work on the SMRs.
Other than that though, I think things should be okayish.

v1 -> v2
    - Corrected how reading of DT is done with generic bindings


 xen/drivers/passthrough/arm/smmu.c    | 102 +++++++++++++++++++++++++---------
 xen/drivers/passthrough/device_tree.c |  17 +-----
 2 files changed, 78 insertions(+), 41 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 7a5c6cd..25c090a 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -251,6 +251,8 @@ struct iommu_group
 	atomic_t ref;
 };
 
+static const 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);
@@ -772,56 +774,104 @@ 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 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;
-	}
-	master->cfg.fwspec = dev_iommu_fwspec_get(&master->of_node->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;
+	master->cfg.fwspec = fwspec;
 
 	/* 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 < master->cfg.fwspec->num_ids; ++i) {
-			if (masterspec->args[i] >= smmu->num_mapping_groups) {
+		for (i = 0; i < fwspec->num_ids; ++i) {
+			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;
 			}
 		}
 	}
+
 	return insert_smmu_master(smmu, master);
 }
 
+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 = (struct arm_smmu_device *) 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 of_phandle_args *spec)
+{
+	uint32_t mask, fwid = 0;
+
+	if (spec->args_count > 0)
+		fwid |= ((SMR_ID_MASK << SMR_ID_SHIFT) & spec->args[0]) >> SMR_ID_SHIFT;
+
+	if (spec->args_count > 1)
+		fwid |= ((SMR_MASK_MASK << SMR_MASK_SHIFT) & spec->args[1]) >> SMR_MASK_SHIFT;
+	else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask))
+		fwid |= ((SMR_MASK_MASK << SMR_MASK_SHIFT) & mask) >> SMR_MASK_SHIFT;
+
+	return iommu_fwspec_add_ids(dev,
+				    &fwid,
+				    1);
+}
+
+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;
@@ -2743,6 +2793,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,
@@ -2750,9 +2801,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 const 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 acf6b62..dd9cf65 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -158,22 +158,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.7.4

Re: [RFC v2 1/2] arm,smmu: switch to using iommu_fwspec functions

[Stefano Stabellini]

On Tue, 21 Jul 2020, Brian Woods wrote:
> 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.
> 
> Signed-off-by: Brian Woods <brian.woods@xilinx.com>

[...]

> @@ -1924,14 +1924,21 @@ static int arm_smmu_add_device(struct device *dev)
>  			ret = -ENOMEM;
>  			goto out_put_group;
>  		}
> +		cfg->fwspec = kzalloc(sizeof(struct iommu_fwspec), GFP_KERNEL);
> +		if (!cfg->fwspec) {
> +			kfree(cfg);
> +			ret = -ENOMEM;
> +			goto out_put_group;
> +		}
> +		iommu_fwspec_init(dev, smmu->dev);

Normally the fwspec structure is initialized in
xen/drivers/passthrough/device_tree.c:iommu_add_dt_device. However here
we are trying to use it with the legacy bindings, that of course don't
initialize in iommu_add_dt_device because they are different.

So I imagine this is the reason why we have to initialize iommu_fwspec here
indepdendently from iommu_add_dt_device.

However, why are we allocating the struct iommu_fwspec twice?

We are calling kzalloc, then iommu_fwspec_init is calling xzalloc.

Similarly, we are storing the pointer to struct iommu_fwspec in
cfg->fwspec but actually there is already a pointer to struct
iommu_fwspec in struct device (the one set by dev_iommu_fwspec_set.)

Do we actually need both?

Re: [RFC v2 1/2] arm,smmu: switch to using iommu_fwspec functions

[Brian Woods]

On Wed, Jul 22, 2020 at 09:47:43AM -0700, Stefano Stabellini wrote:
> On Tue, 21 Jul 2020, Brian Woods wrote:
> > 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.
> > 
> > Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> 
> [...]
> 
> > @@ -1924,14 +1924,21 @@ static int arm_smmu_add_device(struct device *dev)
> >  			ret = -ENOMEM;
> >  			goto out_put_group;
> >  		}
> > +		cfg->fwspec = kzalloc(sizeof(struct iommu_fwspec), GFP_KERNEL);
> > +		if (!cfg->fwspec) {
> > +			kfree(cfg);
> > +			ret = -ENOMEM;
> > +			goto out_put_group;
> > +		}
> > +		iommu_fwspec_init(dev, smmu->dev);
> 
> Normally the fwspec structure is initialized in
> xen/drivers/passthrough/device_tree.c:iommu_add_dt_device. However here
> we are trying to use it with the legacy bindings, that of course don't
> initialize in iommu_add_dt_device because they are different.
> 
> So I imagine this is the reason why we have to initialize iommu_fwspec here
> indepdendently from iommu_add_dt_device.
> 
> However, why are we allocating the struct iommu_fwspec twice?
> 
> We are calling kzalloc, then iommu_fwspec_init is calling xzalloc.
> 
> Similarly, we are storing the pointer to struct iommu_fwspec in
> cfg->fwspec but actually there is already a pointer to struct
> iommu_fwspec in struct device (the one set by dev_iommu_fwspec_set.)
> 
> Do we actually need both?

Sorry for taking so long.

Hrm, I've been looking for why I created two fwspecs and I'm not sure
why... It's pretty late, but later this morning I'll try some things
and try and remove it.

Brian

Re: [RFC v2 1/2] arm,smmu: switch to using iommu_fwspec functions

[Julien Grall]

Hi Brian,

On 22/07/2020 05:00, Brian Woods wrote:
> 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.
> 
> Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> ---
> 
> Interested in if combining the legacy and generic bindings paths are
> worth or if Xen plans to depreicate legacy bindings at some point.

The legacy binding is technically already deprecated in Linux. However, 
I don't think we can remove it completely until Linux does it.

Ideally, I would like the legacy master to be probed as part of 
add_device rather than when the IOMMU initialization (see more below).


> v1 -> v2
>      - removed MAX_MASTER_STREAMIDS
>      - removed unneeded curly brackets
> 
>   xen/drivers/passthrough/arm/smmu.c    | 81 +++++++++++++++++++----------------
>   xen/drivers/passthrough/device_tree.c |  3 ++
>   2 files changed, 47 insertions(+), 37 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 94662a8..7a5c6cd 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -49,6 +49,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 +303,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 +595,7 @@ struct arm_smmu_smr {
>   };
>   
>   struct arm_smmu_master_cfg {
> -	int				num_streamids;
> -	u16				streamids[MAX_MASTER_STREAMIDS];
> +	struct iommu_fwspec		*fwspec;
>   	struct arm_smmu_smr		*smrs;
>   };
>   
> @@ -779,7 +776,7 @@ static int register_smmu_master(struct arm_smmu_device *smmu,

We originally intended to keep the SMMU driver as close as possible to 
Linux. It turns out to be quite difficult as they reworked a fair bit 
the IOMMU subsystem. Although, I would still like to document the change 
we made in the code.

Could you add a comment maybe at the top of file to mention you added 
support for fwspec?

>   				struct device *dev,
>   				struct of_phandle_args *masterspec)
>   {
> -	int i;
> +	int i, ret = 0;
>   	struct arm_smmu_master *master;
>   
>   	master = find_smmu_master(smmu, masterspec->np);
> @@ -790,34 +787,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;
> +	}
> +	master->cfg.fwspec = dev_iommu_fwspec_get(&master->of_node->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 < master->cfg.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);
>   }
> @@ -1397,15 +1397,15 @@ 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(cfg->fwspec->num_ids, sizeof(*smrs), GFP_KERNEL);
>   	if (!smrs) {
>   		dev_err(smmu->dev, "failed to allocate %d SMRs\n",
> -			cfg->num_streamids);
> +			cfg->fwspec->num_ids);
>   		return -ENOMEM;
>   	}
>   
>   	/* Allocate the SMRs on the SMMU */
> -	for (i = 0; i < cfg->num_streamids; ++i) {
> +	for (i = 0; i < cfg->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 +1416,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	= cfg->fwspec->ids[i],
>   		};
>   	}
>   
>   	/* It worked! Now, poke the actual hardware */
> -	for (i = 0; i < cfg->num_streamids; ++i) {
> +	for (i = 0; i < cfg->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));
> @@ -1448,7 +1448,7 @@ static void arm_smmu_master_free_smrs(struct arm_smmu_device *smmu,
>   		return;
>   
>   	/* Invalidate the SMRs before freeing back to the allocator */
> -	for (i = 0; i < cfg->num_streamids; ++i) {
> +	for (i = 0; i < cfg->fwspec->num_ids; ++i) {
>   		u8 idx = smrs[i].idx;
>   
>   		writel_relaxed(~SMR_VALID, gr0_base + ARM_SMMU_GR0_SMR(idx));
> @@ -1471,10 +1471,10 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>   	if (ret)
>   		return ret == -EEXIST ? 0 : ret;
>   
> -	for (i = 0; i < cfg->num_streamids; ++i) {
> +	for (i = 0; i < cfg->fwspec->num_ids; ++i) {
>   		u32 idx, s2cr;
>   
> -		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
> +		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->fwspec->ids[i];
>   		s2cr = S2CR_TYPE_TRANS |
>   		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
>   		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
> @@ -1499,8 +1499,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 < cfg->fwspec->num_ids; ++i) {
> +		u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->fwspec->ids[i];
>   
>   		writel_relaxed(S2CR_TYPE_FAULT,
>   			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
> @@ -1924,14 +1924,21 @@ static int arm_smmu_add_device(struct device *dev)
>   			ret = -ENOMEM;
>   			goto out_put_group;
>   		}
> +		cfg->fwspec = kzalloc(sizeof(struct iommu_fwspec), GFP_KERNEL);
> +		if (!cfg->fwspec) {
> +			kfree(cfg);
> +			ret = -ENOMEM;
> +			goto out_put_group;
> +		}
> +		iommu_fwspec_init(dev, smmu->dev);
>   
> -		cfg->num_streamids = 1;
> +		cfg->fwspec->num_ids = 1;
>   		/*
>   		 * 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]);
> +				       &cfg->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 999b831..acf6b62 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -140,6 +140,9 @@ int iommu_add_dt_device(struct dt_device_node *np)
>       if ( !ops )
>           return -EINVAL;
>   
> +    if ( dt_device_is_protected(np) )
> +        return 0;

With the fwspec in place, I would have hoped we can remove an specific 
hack for the SMMU in the generic DT code. Could you instead explore 
whether it is possible to initialize the legacy master from add_device?

> +
>       if ( dev_iommu_fwspec_get(dev) )
>           return -EEXIST;
>   
> 

-- 
Julien Grall

Re: [RFC v2 2/2] arm,smmu: add support for generic DT bindings

[Julien Grall]

Hi Brian,

On 22/07/2020 05:00, Brian Woods wrote:
> Restructure some of the code and add supporting functions for adding
> generic device tree (DT) binding support.  

It feels to me you want to split the patch in two:
    1) Restructure the code
    2) Add support for DT bindings

> This will allow for using
> current Linux device trees with just modifying the chosen field to
> enable Xen.

So what happen if the legacy binding and generic bindings co-exist. 
Which one will be used?

> 
> Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> ---
> 
> Just realized that I'm fairly sure I need to do some work on the SMRs.
> Other than that though, I think things should be okayish.

The SMMU code in Xen is pretty awful (I know I adapted it for Xen). It 
would be hard to make it worse :).

> 
> v1 -> v2
>      - Corrected how reading of DT is done with generic bindings
> 
> 
>   xen/drivers/passthrough/arm/smmu.c    | 102 +++++++++++++++++++++++++---------
>   xen/drivers/passthrough/device_tree.c |  17 +-----
>   2 files changed, 78 insertions(+), 41 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 7a5c6cd..25c090a 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -251,6 +251,8 @@ struct iommu_group
>   	atomic_t ref;
>   };
>   
> +static const 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);
> @@ -772,56 +774,104 @@ 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 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;
> -	}
> -	master->cfg.fwspec = dev_iommu_fwspec_get(&master->of_node->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;
> +	master->cfg.fwspec = fwspec;
>   
>   	/* 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 < master->cfg.fwspec->num_ids; ++i) {
> -			if (masterspec->args[i] >= smmu->num_mapping_groups) {
> +		for (i = 0; i < fwspec->num_ids; ++i) {
> +			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;
>   			}
>   		}
>   	}
> +
>   	return insert_smmu_master(smmu, master);
>   }
>   
> +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 = (struct arm_smmu_device *) find_smmu(fwspec->iommu_dev)

Please don't use explicit cast to remove a const. If you need 
find_smmu() to return a non-const value, then you should drop the const 
from the return function.


> +	if (smmu == NULL)
> +		return -ENXIO;
> +
> +	return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);

This feels a bit odd to me to call a "legacy" function from a "generic" 
call. How about remove "legacy" from the function name?

> +}
> +
> +static int arm_smmu_dt_xlate_generic(struct device *dev,
> +				    const struct of_phandle_args *spec)

Please use dt_phandle_args to stay consistent with the naming and the 
fact the code is mostly Xen specific (though derived from Linux).

>   
> -static __init const struct arm_smmu_device *find_smmu(const struct device *dev)
> +static const 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 acf6b62..dd9cf65 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -158,22 +158,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;
> -        }

I would add a comment in the commit message explaining the hack in 
iommu_add_dt_device() can be removed.

> +            return -EINVAL;
>   
>           if ( !dt_device_is_available(iommu_spec.np) )
>               break;
> 

Cheers,


-- 
Julien Grall