[PATCH v5 0/4] Generic SMMU Bindings

[PATCH v5 0/4] Generic SMMU Bindings

[Stefano Stabellini]

Hi all,

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

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.

Stefano Stabellini (1):
      xen: do not return -EEXIST if iommu_add_dt_device is called twice

 xen/drivers/passthrough/arm/smmu.c    | 141 +++++++++++++++++++++++++---------
 xen/drivers/passthrough/device_tree.c |  26 ++-----
 2 files changed, 114 insertions(+), 53 deletions(-)

[PATCH v5 1/4] 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>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v5:
- moved iommu_add_dt_device change to a separate patch
---
 xen/drivers/passthrough/arm/smmu.c | 75 ++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 24 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 658c40433c..09773702c3 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 */
@@ -617,13 +621,11 @@ struct arm_smmu_smr {
 
 struct arm_smmu_master_cfg {
 	struct arm_smmu_device		*smmu;
-	int				num_streamids;
-	u16				streamids[MAX_MASTER_STREAMIDS];
 	s16				smendx[MAX_MASTER_STREAMIDS];
 };
 #define INVALID_SMENDX			-1
-#define for_each_cfg_sme(cfg, i, idx) \
-	for (i = 0; idx = cfg->smendx[i], i < cfg->num_streamids; ++i)
+#define for_each_cfg_sme(cfg, i, idx, num) \
+	for (i = 0; idx = cfg->smendx[i], i < num; ++i)
 
 struct arm_smmu_master {
 	struct device_node		*of_node;
@@ -713,6 +715,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;
@@ -806,8 +816,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) {
@@ -817,24 +828,30 @@ 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;
+	}
+
+	/* adding the ids here */
+	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
+				   masterspec->args,
+				   masterspec->args_count);
+	if (ret)
+		return ret;
+
+	fwspec = dev_iommu_fwspec_get(dev);
 
 	/* 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) {
+	for (i = 0; i < fwspec->num_ids; ++i) {
 		u16 streamid = masterspec->args[i];
 
 		if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
@@ -844,7 +861,6 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 				masterspec->np->name, smmu->num_mapping_groups);
 			return -ERANGE;
 		}
-		master->cfg.streamids[i] = streamid;
 		master->cfg.smendx[i] = INVALID_SMENDX;
 	}
 	return insert_smmu_master(smmu, master);
@@ -1500,22 +1516,23 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
 	struct arm_smmu_device *smmu = cfg->smmu;
 	struct arm_smmu_smr *smrs = smmu->smrs;
 	int i, idx, ret;
+	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
 
 	spin_lock(&smmu->stream_map_lock);
 	/* Figure out a viable stream map entry allocation */
-	for_each_cfg_sme(cfg, i, idx) {
+	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
 		if (idx != INVALID_SMENDX) {
 			ret = -EEXIST;
 			goto out_err;
 		}
 
-		ret = arm_smmu_find_sme(smmu, cfg->streamids[i], 0);
+		ret = arm_smmu_find_sme(smmu, fwspec->ids[i], 0);
 		if (ret < 0)
 			goto out_err;
 
 		idx = ret;
 		if (smrs && smmu->s2crs[idx].count == 0) {
-			smrs[idx].id = cfg->streamids[i];
+			smrs[idx].id = fwspec->ids[i];
 			smrs[idx].mask = 0; /* We don't currently share SMRs */
 			smrs[idx].valid = true;
 		}
@@ -1524,7 +1541,7 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
 	}
 
 	/* It worked! Now, poke the actual hardware */
-	for_each_cfg_sme(cfg, i, idx) {
+	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
 		arm_smmu_write_sme(smmu, idx);
 	}
 
@@ -1544,9 +1561,10 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg)
 {
     struct arm_smmu_device *smmu = cfg->smmu;
 	int i, idx;
+	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
 
 	spin_lock(&smmu->stream_map_lock);
-	for_each_cfg_sme(cfg, i, idx) {
+	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
 		if (arm_smmu_free_sme(smmu, idx))
 			arm_smmu_write_sme(smmu, idx);
 		cfg->smendx[i] = INVALID_SMENDX;
@@ -1562,8 +1580,9 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	enum arm_smmu_s2cr_type type = S2CR_TYPE_TRANS;
 	u8 cbndx = smmu_domain->cfg.cbndx;
 	int i, idx;
+	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
 
-	for_each_cfg_sme(cfg, i, idx) {
+	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
 		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
 			continue;
 
@@ -1962,6 +1981,7 @@ static int arm_smmu_add_device(struct device *dev)
 	struct arm_smmu_master_cfg *cfg;
 	struct iommu_group *group;
 	void (*releasefn)(void *) = NULL;
+	int ret;
 
 	smmu = find_smmu_for_device(dev);
 	if (!smmu)
@@ -1969,19 +1989,26 @@ static int arm_smmu_add_device(struct device *dev)
 
 	if (dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(dev);
+		struct iommu_fwspec *fwspec;
 
 		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
 		if (!cfg) {
 			return -ENOMEM;
 		}
 
-		cfg->num_streamids = 1;
+		ret = iommu_fwspec_init(dev, smmu->dev);
+		if (ret) {
+			kfree(cfg);
+			return ret;
+		}
+		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;
 		cfg->smmu = smmu;
 	} else {
-- 
2.17.1

[PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Stefano Stabellini]

If both legacy IOMMU bindings and generic bindings are present,
iommu_add_dt_device can be called twice. Do not return error in that
case, that way there is no need to check for -EEXIST at the call sites.
Remove the one existing -EEXIT check, now unneeded.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v5:
- new patch
---
 xen/drivers/passthrough/device_tree.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 999b831d90..32526ecabb 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
     if ( !ops )
         return -EINVAL;
 
+    /*
+     * Some Device Trees may expose both legacy SMMU and generic
+     * IOMMU bindings together. If both are present, the device
+     * can be already added.
+     */
     if ( dev_iommu_fwspec_get(dev) )
-        return -EEXIST;
+        return 0;
 
     /*
      * According to the Documentation/devicetree/bindings/iommu/iommu.txt
@@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
          * already added to the IOMMU (positive result). Such happens after
          * re-creating guest domain.
          */
-        if ( ret < 0 && ret != -EEXIST )
+        if ( ret < 0 )
         {
             printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n",
                    dt_node_full_name(dev));
-- 
2.17.1

[PATCH v5 3/4] 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>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 xen/drivers/passthrough/arm/smmu.c | 62 ++++++++++++++++--------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 09773702c3..4aa3ecec57 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -812,53 +812,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;
-	}
-
-	/* adding the ids here */
-	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
-				   masterspec->args,
-				   masterspec->args_count);
-	if (ret)
-		return ret;
-
-	fwspec = dev_iommu_fwspec_get(dev);
+	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);
 
 	for (i = 0; i < fwspec->num_ids; ++i) {
-		u16 streamid = masterspec->args[i];
-
 		if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
-		     (streamid >= smmu->num_mapping_groups)) {
+		     (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;
 		}
 		master->cfg.smendx[i] = INVALID_SMENDX;
@@ -866,6 +849,29 @@ 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;
+
+	ret = iommu_fwspec_add_ids(&masterspec->np->dev,
+				   masterspec->args,
+				   masterspec->args_count);
+	if (ret)
+		return ret;
+
+	fwspec = dev_iommu_fwspec_get(&masterspec->np->dev);
+	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 v5 4/4] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.

[Stefano Stabellini]

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

For the legacy path, arm_smmu_dt_add_device_legacy is called by
register_smmu_master scanning mmu-masters (a fwspec entry is also
created.) For the generic path, arm_smmu_dt_add_device_generic gets
called instead. Then, arm_smmu_dt_add_device_generic calls
arm_smmu_dt_add_device_legacy afterwards, shared with the legacy path.
This way most of the low level implementation is shared between the two
paths.

If both legacy bindings and generic bindings are present in device tree,
the legacy bindings are the ones that are used. That's because
mmu-masters is parsed by
xen/drivers/passthrough/arm/smmu.c:arm_smmu_device_dt_probe which is
called by arm_smmu_dt_init. It happens very early. iommus is parsed by
xen/drivers/passthrough/device_tree.c:iommu_add_dt_device which is
called by xen/arch/arm/domain_build.c:handle_device and happens
afterwards.

arm_smmu_dt_xlate_generic is a verbatim copy from Linux
(drivers/iommu/arm/arm-smmu/arm-smmu.c:arm_smmu_of_xlate, version
v5.10).

A workaround was introduced by cf4af9d6d6c (xen/arm: boot with device
trees with "mmu-masters" and "iommus") because the SMMU driver only
supported the legacy bindings. Remove it now.

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>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes in v5:
- add Acked-by
---
 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 4aa3ecec57..c234ad9c7f 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -256,6 +256,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);
@@ -444,6 +446,8 @@ static struct iommu_group *iommu_group_get(struct device *dev)
 #define SMR_VALID			(1U << 31)
 #define SMR_MASK_SHIFT			16
 #define SMR_ID_SHIFT			0
+#define SMR_ID_MASK			0x7fff
+#define SMR_MASK_MASK			0x7fff
 
 #define ARM_SMMU_GR0_S2CR(n)		(0xc00 + ((n) << 2))
 #define S2CR_CBNDX_SHIFT		0
@@ -872,6 +876,38 @@ 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;
@@ -2837,6 +2873,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,
@@ -2844,9 +2881,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 32526ecabb..46ce726521 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -160,22 +160,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 v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Jan Beulich]

On 23.07.2021 01:36, Stefano Stabellini wrote:
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>      if ( !ops )
>          return -EINVAL;
>  
> +    /*
> +     * Some Device Trees may expose both legacy SMMU and generic
> +     * IOMMU bindings together. If both are present, the device
> +     * can be already added.
> +     */
>      if ( dev_iommu_fwspec_get(dev) )
> -        return -EEXIST;
> +        return 0;

Since the xen: prefix in the subject made me go look (I wouldn't have
if it had been e.g. dt: ), I may as well ask: Since previously there
was concern about bogus duplicate entries, does this concern go away
no altogether? It's one thing for there to be a legacy and a generic
binding, but another if you found two legacy or two generic ones, I
would think.

And what if legacy and generic representation differ in some way?
Shouldn't you limit processing to just one of the two categories,
such that no legitimate "already present" case could be encountered
here in the first place?

Jan

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Julien Grall]

Hi Stefano,

On 23/07/2021 00:36, Stefano Stabellini wrote:
> If both legacy IOMMU bindings and generic bindings are present,
> iommu_add_dt_device can be called twice. Do not return error in that
> case, that way there is no need to check for -EEXIST at the call sites.
> Remove the one existing -EEXIT check, now unneeded.

The commit message implies that we already support both legacy and 
generic bindings. However, this is not yet implemented.

So how about:

"
iommu_add_dt_device() will returns -EEXIST if the device was already 
registered.

At the moment, this can only happen if the device was already assigned 
to a domain (either dom0 at boot or via XEN_DOMCTL_assign_device).

In a follow-up patch, we will convert the SMMU driver to use the FW 
spec. When the legacy bindings are used, all the devices will be 
registered at probe. Therefore, iommu_add_dt_device() will always 
returns -EEXIST.

Currently, one caller (XEN_DOMCTL_assign_device) will check the return 
and ignore -EEXIST. All the other will fail because it was technically a 
programming error.

However, there is no harm to call iommu_add_dt_device() twice, so we can 
simply return 0.

With that in place the caller doesn't need to check -EEXIST anymore, so 
remove the check.
"

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> Changes in v5:
> - new patch
> ---
>   xen/drivers/passthrough/device_tree.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 999b831d90..32526ecabb 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>       if ( !ops )
>           return -EINVAL;
>   
> +    /*
> +     * Some Device Trees may expose both legacy SMMU and generic
> +     * IOMMU bindings together. If both are present, the device
> +     * can be already added.

Wouldn't this also happen when there is just generic bindings? If so, 
shouldn't this patch be first in the series to avoid breaking bisection?

> +     */

My point on the previous version is this is not the only reasons why 
dev_iommu_fwspec_get(). So either we want to write all the reasons 
(AFAICT, there is only two) or we want to write a generic message.

>       if ( dev_iommu_fwspec_get(dev) )
> -        return -EEXIST;
> +        return 0;
>   
>       /*
>        * According to the Documentation/devicetree/bindings/iommu/iommu.txt
> @@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>            * already added to the IOMMU (positive result). Such happens after
>            * re-creating guest domain.
>            */

This comment on top is now stale.

> -        if ( ret < 0 && ret != -EEXIST )
> +        if ( ret < 0 )
>           {
>               printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n",
>                      dt_node_full_name(dev));
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Julien Grall]

Hi Jan,

On 23/07/2021 07:31, Jan Beulich wrote:
> On 23.07.2021 01:36, Stefano Stabellini wrote:
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>       if ( !ops )
>>           return -EINVAL;
>>   
>> +    /*
>> +     * Some Device Trees may expose both legacy SMMU and generic
>> +     * IOMMU bindings together. If both are present, the device
>> +     * can be already added.
>> +     */
>>       if ( dev_iommu_fwspec_get(dev) )
>> -        return -EEXIST;
>> +        return 0;
> 
> Since the xen: prefix in the subject made me go look (I wouldn't have
> if it had been e.g. dt: ), I may as well ask: Since previously there
> was concern about bogus duplicate entries, does this concern go away
> no altogether?

The check wasn't originally added because of legacy vs generic binding.

It was added because in some circumstances iommu_add_dt_device() could 
genuinely be called twice (for instance if the device is re-assigned). 
This was returning -EEXIST rather than 0 so the caller can decide 
whether it is normal that the device is already added.

Calling iommu_add_dt_device() twice doesn't hurt but after patch #1 
(this patch should really be first), dev_iommu_fwspec_get() will return 
a non-NULL pointer as the legacy devices are added when the IOMMU is probed.

> It's one thing for there to be a legacy and a generic
> binding, but another if you found two legacy or two generic ones, I
> would think.

I am not quite too sure what you mean by "two legacy" and "two generic". 
Can you clarify it?

> 
> And what if legacy and generic representation differ in some way?

That would be a firmware table issue. It is not Xen business to check 
whether the two representation agree.

> Shouldn't you limit processing to just one of the two categories,
> such that no legitimate "already present" case could be encountered
> here in the first place?
There are legitimate "already present" case. This can happen when a 
device is re-assigned. Arguably the caller could check if the device was 
already added, however it would involve more code in each caller. So it 
is much easier to add in iommu_add_dt_device().

Cheers,

-- 
Julien Grall

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Jan Beulich]

On 23.07.2021 11:28, Julien Grall wrote:
> Hi Jan,
> 
> On 23/07/2021 07:31, Jan Beulich wrote:
>> On 23.07.2021 01:36, Stefano Stabellini wrote:
>>> --- a/xen/drivers/passthrough/device_tree.c
>>> +++ b/xen/drivers/passthrough/device_tree.c
>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>       if ( !ops )
>>>           return -EINVAL;
>>>   
>>> +    /*
>>> +     * Some Device Trees may expose both legacy SMMU and generic
>>> +     * IOMMU bindings together. If both are present, the device
>>> +     * can be already added.
>>> +     */
>>>       if ( dev_iommu_fwspec_get(dev) )
>>> -        return -EEXIST;
>>> +        return 0;
>>
>> Since the xen: prefix in the subject made me go look (I wouldn't have
>> if it had been e.g. dt: ), I may as well ask: Since previously there
>> was concern about bogus duplicate entries, does this concern go away
>> no altogether?
> 
> The check wasn't originally added because of legacy vs generic binding.
> 
> It was added because in some circumstances iommu_add_dt_device() could 
> genuinely be called twice (for instance if the device is re-assigned). 
> This was returning -EEXIST rather than 0 so the caller can decide 
> whether it is normal that the device is already added.

Okay. If that distinction is of no interest anymore, then I can see
this wanting dropping.

> Calling iommu_add_dt_device() twice doesn't hurt but after patch #1 
> (this patch should really be first), dev_iommu_fwspec_get() will return 
> a non-NULL pointer as the legacy devices are added when the IOMMU is probed.
> 
>> It's one thing for there to be a legacy and a generic
>> binding, but another if you found two legacy or two generic ones, I
>> would think.
> 
> I am not quite too sure what you mean by "two legacy" and "two generic". 
> Can you clarify it?

Well, I'm having trouble describing it in different terms. I mean
two entries of the same kind (both legacy or both generic) referring
to the same device, thus leading to the function recognizing the 2nd
time round that the device is already there.

Jan

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Stefano Stabellini]

On Fri, 23 Jul 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/07/2021 00:36, Stefano Stabellini wrote:
> > If both legacy IOMMU bindings and generic bindings are present,
> > iommu_add_dt_device can be called twice. Do not return error in that
> > case, that way there is no need to check for -EEXIST at the call sites.
> > Remove the one existing -EEXIT check, now unneeded.
> 
> The commit message implies that we already support both legacy and generic
> bindings. However, this is not yet implemented.
> 
> So how about:
> 
> "
> iommu_add_dt_device() will returns -EEXIST if the device was already
> registered.
> 
> At the moment, this can only happen if the device was already assigned to a
> domain (either dom0 at boot or via XEN_DOMCTL_assign_device).
> 
> In a follow-up patch, we will convert the SMMU driver to use the FW spec. When
> the legacy bindings are used, all the devices will be registered at probe.
> Therefore, iommu_add_dt_device() will always returns -EEXIST.
> 
> Currently, one caller (XEN_DOMCTL_assign_device) will check the return and
> ignore -EEXIST. All the other will fail because it was technically a
> programming error.
> 
> However, there is no harm to call iommu_add_dt_device() twice, so we can
> simply return 0.
> 
> With that in place the caller doesn't need to check -EEXIST anymore, so remove
> the check.
> "

This is a lot better, thank you!


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > ---
> > Changes in v5:
> > - new patch
> > ---
> >   xen/drivers/passthrough/device_tree.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/drivers/passthrough/device_tree.c
> > b/xen/drivers/passthrough/device_tree.c
> > index 999b831d90..32526ecabb 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
> >       if ( !ops )
> >           return -EINVAL;
> >   +    /*
> > +     * Some Device Trees may expose both legacy SMMU and generic
> > +     * IOMMU bindings together. If both are present, the device
> > +     * can be already added.
> 
> Wouldn't this also happen when there is just generic bindings? If so,
> shouldn't this patch be first in the series to avoid breaking bisection?

No, both need to be present; if there is just the generic bindings we
don't need this change. I can still move it to the beginning of the
series anyway if you prefer. 


> > +     */
> 
> My point on the previous version is this is not the only reasons why
> dev_iommu_fwspec_get(). So either we want to write all the reasons (AFAICT,
> there is only two) or we want to write a generic message.

I see. Maybe:

  * In some circumstances iommu_add_dt_device() can genuinly be called
  * twice. As there is no harm in it just return success early.


> >       if ( dev_iommu_fwspec_get(dev) )
> > -        return -EEXIST;
> > +        return 0;
> >         /*
> >        * According to the Documentation/devicetree/bindings/iommu/iommu.txt
> > @@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct
> > domain *d,
> >            * already added to the IOMMU (positive result). Such happens
> > after
> >            * re-creating guest domain.
> >            */
> 
> This comment on top is now stale.

I missed it somehow; yes definitely it should be removed. I can do it in
the next version of this patch.


> > -        if ( ret < 0 && ret != -EEXIST )
> > +        if ( ret < 0 )
> >           {
> >               printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n",
> >                      dt_node_full_name(dev));

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Julien Grall]

Hi Jan,

On 23/07/2021 14:02, Jan Beulich wrote:
> On 23.07.2021 11:28, Julien Grall wrote:
>> Hi Jan,
>>
>> On 23/07/2021 07:31, Jan Beulich wrote:
>>> On 23.07.2021 01:36, Stefano Stabellini wrote:
>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>>        if ( !ops )
>>>>            return -EINVAL;
>>>>    
>>>> +    /*
>>>> +     * Some Device Trees may expose both legacy SMMU and generic
>>>> +     * IOMMU bindings together. If both are present, the device
>>>> +     * can be already added.
>>>> +     */
>>>>        if ( dev_iommu_fwspec_get(dev) )
>>>> -        return -EEXIST;
>>>> +        return 0;
>>>
>>> Since the xen: prefix in the subject made me go look (I wouldn't have
>>> if it had been e.g. dt: ), I may as well ask: Since previously there
>>> was concern about bogus duplicate entries, does this concern go away
>>> no altogether?
>>
>> The check wasn't originally added because of legacy vs generic binding.
>>
>> It was added because in some circumstances iommu_add_dt_device() could
>> genuinely be called twice (for instance if the device is re-assigned).
>> This was returning -EEXIST rather than 0 so the caller can decide
>> whether it is normal that the device is already added.
> 
> Okay. If that distinction is of no interest anymore, then I can see
> this wanting dropping.
> 
>> Calling iommu_add_dt_device() twice doesn't hurt but after patch #1
>> (this patch should really be first), dev_iommu_fwspec_get() will return
>> a non-NULL pointer as the legacy devices are added when the IOMMU is probed.
>>
>>> It's one thing for there to be a legacy and a generic
>>> binding, but another if you found two legacy or two generic ones, I
>>> would think.
>>
>> I am not quite too sure what you mean by "two legacy" and "two generic".
>> Can you clarify it?
> 
> Well, I'm having trouble describing it in different terms. I mean
> two entries of the same kind (both legacy or both generic) referring
> to the same device, thus leading to the function recognizing the 2nd > time round that the device is already there.

I think you are misunderstanding the purpose of this function. It is 
called when we discover a new device rather than discovering a new entry 
in the IOMMU. The function will then sort out what to do for the device.

The legacy binding is somewhat specific because it bypass the function 
as the discovering is done per IOMMU rather than per device.

Cheers,

-- 
Julien Grall

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Julien Grall]

Hi Stefano,

On 23/07/2021 21:16, Stefano Stabellini wrote:
> On Fri, 23 Jul 2021, Julien Grall wrote:
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>> ---
>>> Changes in v5:
>>> - new patch
>>> ---
>>>    xen/drivers/passthrough/device_tree.c | 9 +++++++--
>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/device_tree.c
>>> b/xen/drivers/passthrough/device_tree.c
>>> index 999b831d90..32526ecabb 100644
>>> --- a/xen/drivers/passthrough/device_tree.c
>>> +++ b/xen/drivers/passthrough/device_tree.c
>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>        if ( !ops )
>>>            return -EINVAL;
>>>    +    /*
>>> +     * Some Device Trees may expose both legacy SMMU and generic
>>> +     * IOMMU bindings together. If both are present, the device
>>> +     * can be already added.
>>
>> Wouldn't this also happen when there is just generic bindings? If so,
>> shouldn't this patch be first in the series to avoid breaking bisection?
> 
> No, both need to be present; if there is just the generic bindings we
> don't need this change. I can still move it to the beginning of the
> series anyway if you prefer.

Sorry but I am having some trouble to understand why this is not a 
problem for just the legacy binding.

If I look at patch #1, the dev->iommu_fspec will be allocated in 
register_smmu_master(). If I am not mistaken, this is called when the 
SMMU is initialized.

So the call to iommu_add_dt_device() in handle_device() should return 
-EEXIST (dev_iommu_fwspec_get() will return a non-NULL pointer).

What did I miss?

> 
> 
>>> +     */
>>
>> My point on the previous version is this is not the only reasons why
>> dev_iommu_fwspec_get(). So either we want to write all the reasons (AFAICT,
>> there is only two) or we want to write a generic message.
> 
> I see. Maybe:
> 
>    * In some circumstances iommu_add_dt_device() can genuinly be called
>    * twice. As there is no harm in it just return success early.

Sound good to me.

> 
> 
>>>        if ( dev_iommu_fwspec_get(dev) )
>>> -        return -EEXIST;
>>> +        return 0;
>>>          /*
>>>         * According to the Documentation/devicetree/bindings/iommu/iommu.txt
>>> @@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct
>>> domain *d,
>>>             * already added to the IOMMU (positive result). Such happens
>>> after
>>>             * re-creating guest domain.
>>>             */
>>
>> This comment on top is now stale.
> 
> I missed it somehow; yes definitely it should be removed. I can do it in
> the next version of this patch.
> 
> 
>>> -        if ( ret < 0 && ret != -EEXIST )
>>> +        if ( ret < 0 )
>>>            {
>>>                printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n",
>>>                       dt_node_full_name(dev));

Cheers,

-- 
Julien Grall

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Stefano Stabellini]

On Mon, 26 Jul 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/07/2021 21:16, Stefano Stabellini wrote:
> > On Fri, 23 Jul 2021, Julien Grall wrote:
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > > ---
> > > > Changes in v5:
> > > > - new patch
> > > > ---
> > > >    xen/drivers/passthrough/device_tree.c | 9 +++++++--
> > > >    1 file changed, 7 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/xen/drivers/passthrough/device_tree.c
> > > > b/xen/drivers/passthrough/device_tree.c
> > > > index 999b831d90..32526ecabb 100644
> > > > --- a/xen/drivers/passthrough/device_tree.c
> > > > +++ b/xen/drivers/passthrough/device_tree.c
> > > > @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
> > > >        if ( !ops )
> > > >            return -EINVAL;
> > > >    +    /*
> > > > +     * Some Device Trees may expose both legacy SMMU and generic
> > > > +     * IOMMU bindings together. If both are present, the device
> > > > +     * can be already added.
> > > 
> > > Wouldn't this also happen when there is just generic bindings? If so,
> > > shouldn't this patch be first in the series to avoid breaking bisection?
> > 
> > No, both need to be present; if there is just the generic bindings we
> > don't need this change. I can still move it to the beginning of the
> > series anyway if you prefer.
> 
> Sorry but I am having some trouble to understand why this is not a problem for
> just the legacy binding.
> 
> If I look at patch #1, the dev->iommu_fspec will be allocated in
> register_smmu_master(). If I am not mistaken, this is called when the SMMU is
> initialized.
> 
> So the call to iommu_add_dt_device() in handle_device() should return -EEXIST
> (dev_iommu_fwspec_get() will return a non-NULL pointer).
> 
> What did I miss?

I checked. It is true that we need this check with the legacy bindings,
even when alone.

Like you said, dev->iommu_fspec is allocated early by
register_smmu_master. Then, handle_device, or handle_passthrough_prop
for dom0less guests, calls iommu_add_dt_device a second time.

On the other hand with only the generic bindings register_smmu_master
doesn't call iommu_add_dt_device, so iommu_add_dt_device is called only
once by handle_device or handle_passthrough_prop.


The comment I proposed is not correct. What about this one?

    /*
     * In case of legacy SMMU bindings, register_smmu_master might have
     * already initialized struct iommu_fwspec for this device.
     */

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Julien Grall]

Hi Stefano,

On 30/07/2021 22:57, Stefano Stabellini wrote:
> On Mon, 26 Jul 2021, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 23/07/2021 21:16, Stefano Stabellini wrote:
>>> On Fri, 23 Jul 2021, Julien Grall wrote:
>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>>> ---
>>>>> Changes in v5:
>>>>> - new patch
>>>>> ---
>>>>>     xen/drivers/passthrough/device_tree.c | 9 +++++++--
>>>>>     1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/xen/drivers/passthrough/device_tree.c
>>>>> b/xen/drivers/passthrough/device_tree.c
>>>>> index 999b831d90..32526ecabb 100644
>>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>>>         if ( !ops )
>>>>>             return -EINVAL;
>>>>>     +    /*
>>>>> +     * Some Device Trees may expose both legacy SMMU and generic
>>>>> +     * IOMMU bindings together. If both are present, the device
>>>>> +     * can be already added.
>>>>
>>>> Wouldn't this also happen when there is just generic bindings? If so,
>>>> shouldn't this patch be first in the series to avoid breaking bisection?
>>>
>>> No, both need to be present; if there is just the generic bindings we
>>> don't need this change. I can still move it to the beginning of the
>>> series anyway if you prefer.
>>
>> Sorry but I am having some trouble to understand why this is not a problem for
>> just the legacy binding.
>>
>> If I look at patch #1, the dev->iommu_fspec will be allocated in
>> register_smmu_master(). If I am not mistaken, this is called when the SMMU is
>> initialized.
>>
>> So the call to iommu_add_dt_device() in handle_device() should return -EEXIST
>> (dev_iommu_fwspec_get() will return a non-NULL pointer).
>>
>> What did I miss?
> 
> I checked. It is true that we need this check with the legacy bindings,
> even when alone.
> 
> Like you said, dev->iommu_fspec is allocated early by
> register_smmu_master. Then, handle_device, or handle_passthrough_prop
> for dom0less guests, calls iommu_add_dt_device a second time.
> 
> On the other hand with only the generic bindings register_smmu_master
> doesn't call iommu_add_dt_device, so iommu_add_dt_device is called only
> once by handle_device or handle_passthrough_prop. >
> 
> The comment I proposed is not correct. What about this one?
> 
>      /*
>       * In case of legacy SMMU bindings, register_smmu_master might have
>       * already initialized struct iommu_fwspec for this device.
>       */
As I may have mentionned in a previous version of the series, this check 
is not specific to the SMMU. We also need it for other cases (e.g. when 
the device is (re-)assigned to a guest).

So I think a specialized comment is unsuitable here. Instead, we should 
provide a generic comment. Something like:

/*
  * The device may already have been registered. As there is no harm in
  * it just return success early.
  */

Cheers,

-- 
Julien Grall

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Stefano Stabellini]

On Mon, 2 Aug 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 30/07/2021 22:57, Stefano Stabellini wrote:
> > On Mon, 26 Jul 2021, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 23/07/2021 21:16, Stefano Stabellini wrote:
> > > > On Fri, 23 Jul 2021, Julien Grall wrote:
> > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > > > > ---
> > > > > > Changes in v5:
> > > > > > - new patch
> > > > > > ---
> > > > > >     xen/drivers/passthrough/device_tree.c | 9 +++++++--
> > > > > >     1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/xen/drivers/passthrough/device_tree.c
> > > > > > b/xen/drivers/passthrough/device_tree.c
> > > > > > index 999b831d90..32526ecabb 100644
> > > > > > --- a/xen/drivers/passthrough/device_tree.c
> > > > > > +++ b/xen/drivers/passthrough/device_tree.c
> > > > > > @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node
> > > > > > *np)
> > > > > >         if ( !ops )
> > > > > >             return -EINVAL;
> > > > > >     +    /*
> > > > > > +     * Some Device Trees may expose both legacy SMMU and generic
> > > > > > +     * IOMMU bindings together. If both are present, the device
> > > > > > +     * can be already added.
> > > > > 
> > > > > Wouldn't this also happen when there is just generic bindings? If so,
> > > > > shouldn't this patch be first in the series to avoid breaking
> > > > > bisection?
> > > > 
> > > > No, both need to be present; if there is just the generic bindings we
> > > > don't need this change. I can still move it to the beginning of the
> > > > series anyway if you prefer.
> > > 
> > > Sorry but I am having some trouble to understand why this is not a problem
> > > for
> > > just the legacy binding.
> > > 
> > > If I look at patch #1, the dev->iommu_fspec will be allocated in
> > > register_smmu_master(). If I am not mistaken, this is called when the SMMU
> > > is
> > > initialized.
> > > 
> > > So the call to iommu_add_dt_device() in handle_device() should return
> > > -EEXIST
> > > (dev_iommu_fwspec_get() will return a non-NULL pointer).
> > > 
> > > What did I miss?
> > 
> > I checked. It is true that we need this check with the legacy bindings,
> > even when alone.
> > 
> > Like you said, dev->iommu_fspec is allocated early by
> > register_smmu_master. Then, handle_device, or handle_passthrough_prop
> > for dom0less guests, calls iommu_add_dt_device a second time.
> > 
> > On the other hand with only the generic bindings register_smmu_master
> > doesn't call iommu_add_dt_device, so iommu_add_dt_device is called only
> > once by handle_device or handle_passthrough_prop. >
> > 
> > The comment I proposed is not correct. What about this one?
> > 
> >      /*
> >       * In case of legacy SMMU bindings, register_smmu_master might have
> >       * already initialized struct iommu_fwspec for this device.
> >       */
> As I may have mentionned in a previous version of the series, this check is
> not specific to the SMMU. We also need it for other cases (e.g. when the
> device is (re-)assigned to a guest).
> 
> So I think a specialized comment is unsuitable here. Instead, we should
> provide a generic comment. Something like:
> 
> /*
>  * The device may already have been registered. As there is no harm in
>  * it just return success early.
>  */

Thanks, I'll use it in the next version

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Jan Beulich]

On 26.07.2021 17:45, Julien Grall wrote:
> On 23/07/2021 14:02, Jan Beulich wrote:
>> On 23.07.2021 11:28, Julien Grall wrote:
>>> On 23/07/2021 07:31, Jan Beulich wrote:
>>>> On 23.07.2021 01:36, Stefano Stabellini wrote:
>>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>>>        if ( !ops )
>>>>>            return -EINVAL;
>>>>>    
>>>>> +    /*
>>>>> +     * Some Device Trees may expose both legacy SMMU and generic
>>>>> +     * IOMMU bindings together. If both are present, the device
>>>>> +     * can be already added.
>>>>> +     */
>>>>>        if ( dev_iommu_fwspec_get(dev) )
>>>>> -        return -EEXIST;
>>>>> +        return 0;
>>>>
>>>> Since the xen: prefix in the subject made me go look (I wouldn't have
>>>> if it had been e.g. dt: ), I may as well ask: Since previously there
>>>> was concern about bogus duplicate entries, does this concern go away
>>>> no altogether?
>>>
>>> The check wasn't originally added because of legacy vs generic binding.
>>>
>>> It was added because in some circumstances iommu_add_dt_device() could
>>> genuinely be called twice (for instance if the device is re-assigned).
>>> This was returning -EEXIST rather than 0 so the caller can decide
>>> whether it is normal that the device is already added.
>>
>> Okay. If that distinction is of no interest anymore, then I can see
>> this wanting dropping.
>>
>>> Calling iommu_add_dt_device() twice doesn't hurt but after patch #1
>>> (this patch should really be first), dev_iommu_fwspec_get() will return
>>> a non-NULL pointer as the legacy devices are added when the IOMMU is probed.
>>>
>>>> It's one thing for there to be a legacy and a generic
>>>> binding, but another if you found two legacy or two generic ones, I
>>>> would think.
>>>
>>> I am not quite too sure what you mean by "two legacy" and "two generic".
>>> Can you clarify it?
>>
>> Well, I'm having trouble describing it in different terms. I mean
>> two entries of the same kind (both legacy or both generic) referring
>> to the same device, thus leading to the function recognizing the 2nd > time round that the device is already there.
> 
> I think you are misunderstanding the purpose of this function. It is 
> called when we discover a new device rather than discovering a new entry 
> in the IOMMU. The function will then sort out what to do for the device.

I'm struggling with assigning meaning to "discovering a new entry in the
IOMMU". Otoh to "discover a new device" means the device wasn't (supposed
to be) known before, which to me means -EEXIST is appropriate.

> The legacy binding is somewhat specific because it bypass the function 
> as the discovering is done per IOMMU rather than per device.

Well, I then guess I'm lacking too much context here.

Jan

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Julien Grall]

Hi Jan,

On 03/08/2021 07:57, Jan Beulich wrote:
> On 26.07.2021 17:45, Julien Grall wrote:
>> On 23/07/2021 14:02, Jan Beulich wrote:
>>> On 23.07.2021 11:28, Julien Grall wrote:
>>>> On 23/07/2021 07:31, Jan Beulich wrote:
>>>>> On 23.07.2021 01:36, Stefano Stabellini wrote:
>>>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>>>>         if ( !ops )
>>>>>>             return -EINVAL;
>>>>>>     
>>>>>> +    /*
>>>>>> +     * Some Device Trees may expose both legacy SMMU and generic
>>>>>> +     * IOMMU bindings together. If both are present, the device
>>>>>> +     * can be already added.
>>>>>> +     */
>>>>>>         if ( dev_iommu_fwspec_get(dev) )
>>>>>> -        return -EEXIST;
>>>>>> +        return 0;
>>>>>
>>>>> Since the xen: prefix in the subject made me go look (I wouldn't have
>>>>> if it had been e.g. dt: ), I may as well ask: Since previously there
>>>>> was concern about bogus duplicate entries, does this concern go away
>>>>> no altogether?
>>>>
>>>> The check wasn't originally added because of legacy vs generic binding.
>>>>
>>>> It was added because in some circumstances iommu_add_dt_device() could
>>>> genuinely be called twice (for instance if the device is re-assigned).
>>>> This was returning -EEXIST rather than 0 so the caller can decide
>>>> whether it is normal that the device is already added.
>>>
>>> Okay. If that distinction is of no interest anymore, then I can see
>>> this wanting dropping.
>>>
>>>> Calling iommu_add_dt_device() twice doesn't hurt but after patch #1
>>>> (this patch should really be first), dev_iommu_fwspec_get() will return
>>>> a non-NULL pointer as the legacy devices are added when the IOMMU is probed.
>>>>
>>>>> It's one thing for there to be a legacy and a generic
>>>>> binding, but another if you found two legacy or two generic ones, I
>>>>> would think.
>>>>
>>>> I am not quite too sure what you mean by "two legacy" and "two generic".
>>>> Can you clarify it?
>>>
>>> Well, I'm having trouble describing it in different terms. I mean
>>> two entries of the same kind (both legacy or both generic) referring
>>> to the same device, thus leading to the function recognizing the 2nd > time round that the device is already there.
>>
>> I think you are misunderstanding the purpose of this function. It is
>> called when we discover a new device rather than discovering a new entry
>> in the IOMMU. The function will then sort out what to do for the device.
> 
> I'm struggling with assigning meaning to "discovering a new entry in the
> IOMMU".

I meant in the IOMMU firmware table, sorry. IOW, when a new IOMMU is 
added we walk its configuration to figure out which device is attached 
to it.

>  Otoh to "discover a new device" means the device wasn't (supposed
> to be) known before, which to me means -EEXIST is appropriate.

Right. The problem is after patch #1 all callers would need to cope with 
-EEXIST because the legacy binding register the device up front.

That's why I think returning 0 here is better.

Cheers,

-- 
Julien Grall