1. Patchew
  2. Xen
  3. [Xen-devel] [RFC 0/2] Generic DT Support for SMMU
  4. View patch

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

Brian Woods posted 2 patches 6 years, 1 month ago
[Xen-devel] [RFC 1/2] arm, smmu: add support for iommu_fwspec functions
Posted by Brian Woods 6 years, 1 month ago 
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
Re: [Xen-devel] [RFC 1/2] arm, smmu: add support for iommu_fwspec functions
Posted by Julien Grall 6 years ago 
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
Posted by Stefano Stabellini 6 years ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.