[Xen-devel] [RFC 0/2] Generic DT Support for SMMU

[Xen-devel] [RFC 0/2] Generic DT Support for SMMU

[Brian Woods]

I'd like some feedback on these patches.  Parts I particularly want
feedback on are listed below and each patch will have a copy of the
relevant parts requesting feedback.  Any feedback is welcomed though.
Also, the commit messages are a little rough.

Patch 1:
  - Check in device_tree.c.  This is needed, otherwise it won't boot due
    to dev_iommu_fwspec_get(dev) being true and returning EEXIST.  I'm
    not completely sure what type of check is best here.

Patch 2:
   - Checks for the: arm_smmu_dt_add_device* and arm_smmu_dt_xlate*
     functions.

These patches have been tested with device passthrough on a Xilinx
ZCU102 with passing the on board ethernet to a guest via the SMMU.

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

 xen/drivers/passthrough/arm/smmu.c    | 156 +++++++++++++++++++++++++---------
 xen/drivers/passthrough/device_tree.c |  20 +----
 2 files changed, 118 insertions(+), 58 deletions(-)

-- 
2.7.4


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

[Xen-devel] [RFC 1/2] arm, smmu: add support for 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, making enabling generic device tree
bindings in the SMMU driver much cleaner.

Signed-off-by: Brian Woods <brian.woods@xilinx.com>
---
RFC especially wanted on:
  - Check in device_tree.c.  This is needed, otherwise it won't boot due
    to dev_iommu_fwspec_get(dev) being true and returning EEXIST.  I'm
    not completely sure what type of check is best here.

 xen/drivers/passthrough/arm/smmu.c    | 74 ++++++++++++++++++++++-------------
 xen/drivers/passthrough/device_tree.c |  3 ++
 2 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 94662a8..c5db5be 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 */
@@ -597,8 +598,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 +779,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);
@@ -798,26 +798,37 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	}
 
 	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
-	if (!master)
+	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);
 
-	master->of_node			= masterspec->np;
-	master->cfg.num_streamids	= masterspec->args_count;
+	/* 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 +1408,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 +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	= 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 +1459,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 +1482,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 +1510,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 +1935,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


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

[Xen-devel] [RFC 2/2] 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.  The normal add_device and
dt_xlate functions are wrappers of the legacy functions due to legacy
calls needing more arguments because the find_smmu can't a smmu that
isn't initialized.

Signed-off-by: Brian Woods <brian.woods@xilinx.com>
---
RFC especially on:
   - Checks for the: arm_smmu_dt_add_device* and arm_smmu_dt_xlate*
     functions.

 xen/drivers/passthrough/arm/smmu.c    | 118 +++++++++++++++++++++++++---------
 xen/drivers/passthrough/device_tree.c |  17 +----
 2 files changed, 87 insertions(+), 48 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index c5db5be..08787cd 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);
@@ -775,64 +777,114 @@ 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)
+/*
+ * Since smmu isn't done initializing before this is run in the legacy
+ * case, create a function where that's passed and then have the generic
+ * function just be a simple wrapper.
+ */
+static int arm_smmu_dt_xlate_legacy(struct device *dev,
+				    const struct of_phandle_args *spec,
+				    struct iommu_fwspec *fwspec)
+{
+	if ((spec->args_count + fwspec->num_ids) > MAX_MASTER_STREAMIDS) {
+		dev_err(dev,
+			"reached maximum number (%d) of stream IDs for master device %s\n",
+			MAX_MASTER_STREAMIDS, spec->np->name);
+		return -ENOSPC;
+	}
+
+	/* adding the ids here */
+	return iommu_fwspec_add_ids(dev,
+				    spec->args,
+				    spec->args_count);
+}
+
+static int arm_smmu_dt_xlate(struct device *dev,
+			     const struct dt_phandle_args *spec)
+{
+	return arm_smmu_dt_xlate_legacy(dev,
+					spec,
+					dev_iommu_fwspec_get(dev));
+}
+
+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);
+
+	BUG_ON(dev == NULL);
+	BUG_ON(dev_node == NULL);
 
-	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;
 	}
 
-	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;
-
-	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);
+	master->of_node = dev_node;
 
-	/* adding the ids here */
-	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
-				   masterspec->args,
-				   masterspec->args_count);
-	if (ret)
-		return ret;
+	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(u8 devfn, struct device *dev)
+{
+	struct arm_smmu_device *smmu;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+	smmu = (struct arm_smmu_device *) find_smmu(fwspec->iommu_dev);
+
+	return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
+}
+
+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 = arm_smmu_dt_xlate_legacy(&masterspec->np->dev,
+				       masterspec,
+				       fwspec);
+	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;
@@ -2754,6 +2806,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,
     .teardown = arm_smmu_iommu_domain_teardown,
     .iotlb_flush = arm_smmu_iotlb_flush,
     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
@@ -2761,9 +2814,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,
 };
 
-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


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

Re: [Xen-devel] [RFC 2/2] smmu: add support for generic DT bindings

[Julien Grall]

Hi Brian,

Thank you for the patch.

On 10/01/2020 01:26, Brian Woods wrote:
> Restructure some of the code and add supporting functions for adding
> generic device tree (DT) binding support.  The normal add_device and
> dt_xlate functions are wrappers of the legacy functions due to legacy
> calls needing more arguments because the find_smmu can't a smmu that
> isn't initialized.
> 
> Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> ---
> RFC especially on:
>     - Checks for the: arm_smmu_dt_add_device* and arm_smmu_dt_xlate*
>       functions.
> 
>   xen/drivers/passthrough/arm/smmu.c    | 118 +++++++++++++++++++++++++---------
>   xen/drivers/passthrough/device_tree.c |  17 +----
>   2 files changed, 87 insertions(+), 48 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index c5db5be..08787cd 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);
> @@ -775,64 +777,114 @@ 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)
> +/*
> + * Since smmu isn't done initializing before this is run in the legacy
> + * case, create a function where that's passed and then have the generic
> + * function just be a simple wrapper.
> + */
> +static int arm_smmu_dt_xlate_legacy(struct device *dev,
> +				    const struct of_phandle_args *spec,
> +				    struct iommu_fwspec *fwspec)
> +{
> +	if ((spec->args_count + fwspec->num_ids) > MAX_MASTER_STREAMIDS) {
> +		dev_err(dev,
> +			"reached maximum number (%d) of stream IDs for master device %s\n",
> +			MAX_MASTER_STREAMIDS, spec->np->name);
> +		return -ENOSPC;
> +	}
> +
> +	/* adding the ids here */
> +	return iommu_fwspec_add_ids(dev,
> +				    spec->args,
> +				    spec->args_count);
> +}
> +
> +static int arm_smmu_dt_xlate(struct device *dev,
> +			     const struct dt_phandle_args *spec)
> +{
> +	return arm_smmu_dt_xlate_legacy(dev,
> +					spec,
> +					dev_iommu_fwspec_get(dev));
> +}

The legacy and generic bindings are fundamentally different.

In the case of the legacy binding, a specifier will contains multiple 
streamids. But for the generic bindings, the interpretation of the 
specifier will depend on the number of arguments.

If you want to specify multiple streamID, you would either have to use 
one specifier per streamID or use stream matching. You also have an 
additional property to take care of (see "stream-match-mask").

Please have a look at the bindings in Linux ([1], [2]) for more details. 
I would also recommend to have a look at the SMMU driver in Linux as 
well [3].

I would expect this to change the way the patch is structure. So I am 
not going to review the rest. Although, let me know if you want me to 
look at a particular bits.

Cheers,


[1] Documentation/devicetree/bindings/iommu/iommu.txt
[2] Documentation/devicetree/bindings/iommu/arm,smmu.txt
[3] drivers/iommu/arm-smmu.c

-- 
Julien Grall

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

Re: [Xen-devel] [RFC 1/2] arm, smmu: add support for iommu_fwspec functions

[Julien Grall]

Hi Brian,

On 10/01/2020 01:26, 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, making enabling generic device tree
> bindings in the SMMU driver much cleaner.
> 
> Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> ---
> RFC especially wanted on:
>    - Check in device_tree.c.  This is needed, otherwise it won't boot due
>      to dev_iommu_fwspec_get(dev) being true and returning EEXIST.  I'm
>      not completely sure what type of check is best here.

I guess this because the masters are registered during the 
initialization of the SMMU. Could we instead look at registering them 
from the add_device callback?

I understand this would mean to go through all the SMMU and require a 
bit more work. But I think it will help longer term as the workflow for 
registering a master would be similar whether legacy or generic bindings 
are used.

@Stefano any opinions?

> 
>   xen/drivers/passthrough/arm/smmu.c    | 74 ++++++++++++++++++++++-------------
>   xen/drivers/passthrough/device_tree.c |  3 ++ >   2 files changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 94662a8..c5db5be 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 */
> @@ -597,8 +598,7 @@ struct arm_smmu_smr {
>   };
>   
>   struct arm_smmu_master_cfg {
> -	int				num_streamids;
> -	u16				streamids[MAX_MASTER_STREAMIDS];

Now that we use fwspec, do we really need to keep the 
MAX_MASTER_STREAMIDS limit?

> +	struct iommu_fwspec		*fwspec;

NIT: Can the content be const?

>   	struct arm_smmu_smr		*smrs;
>   };
>   
> @@ -779,7 +779,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);
> @@ -798,26 +798,37 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>   	}
>   
>   	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
> -	if (!master)
> +	if (!master) {
>   		return -ENOMEM;
> +	}

NIT: May I ask why did you add the {} here?


Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC 1/2] arm, smmu: add support for iommu_fwspec functions

[Stefano Stabellini]

On Tue, 14 Jan 2020, Julien Grall wrote:
> Hi Brian,
> 
> On 10/01/2020 01:26, 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, making enabling generic device tree
> > bindings in the SMMU driver much cleaner.
> > 
> > Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> > ---
> > RFC especially wanted on:
> >    - Check in device_tree.c.  This is needed, otherwise it won't boot due
> >      to dev_iommu_fwspec_get(dev) being true and returning EEXIST.  I'm
> >      not completely sure what type of check is best here.
> 
> I guess this because the masters are registered during the initialization of
> the SMMU. Could we instead look at registering them from the add_device
> callback?
> 
> I understand this would mean to go through all the SMMU and require a bit more
> work. But I think it will help longer term as the workflow for registering a
> master would be similar whether legacy or generic bindings are used.
> 
> @Stefano any opinions?

Yeah, add_device looks like a better fit for the new bindings.


> >   xen/drivers/passthrough/arm/smmu.c    | 74
> > ++++++++++++++++++++++-------------
> >   xen/drivers/passthrough/device_tree.c |  3 ++ >   2 files changed, 49
> > insertions(+), 28 deletions(-)
> > 
> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > b/xen/drivers/passthrough/arm/smmu.c
> > index 94662a8..c5db5be 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 */
> > @@ -597,8 +598,7 @@ struct arm_smmu_smr {
> >   };
> >     struct arm_smmu_master_cfg {
> > -	int				num_streamids;
> > -	u16				streamids[MAX_MASTER_STREAMIDS];
> 
> Now that we use fwspec, do we really need to keep the MAX_MASTER_STREAMIDS
> limit?
> 
> > +	struct iommu_fwspec		*fwspec;
> 
> NIT: Can the content be const?
> 
> >   	struct arm_smmu_smr		*smrs;
> >   };
> >   @@ -779,7 +779,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);
> > @@ -798,26 +798,37 @@ static int register_smmu_master(struct arm_smmu_device
> > *smmu,
> >   	}
> >     	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
> > -	if (!master)
> > +	if (!master) {
> >   		return -ENOMEM;
> > +	}
> 
> NIT: May I ask why did you add the {} here?
> 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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