[PATCH 0/2] Add support parse PCI iommu bindings for Xen SMMU

[PATCH 0/2] Add support parse PCI iommu bindings for Xen SMMU

[Wei Chen]

The legacy IOMMU bindings will be deprecated in future. Instead, device
tree provide generic IOMMU bindings and PCI IOMMU bindings. Currently,
the PCI support hasn't been enabled on ARM Xen. So in this series, we
just add the support of parsing PCI IOMMU bindings.

Wei Chen (2):
  xen: devicetree: Introduce a helper to translate PCI requester ID
  xen/arm: smmu: Parse generic iommu binding for PCI devices

 xen/common/device_tree.c           | 89 ++++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/arm/smmu.c | 73 ++++++++++++++++++++++++++++++-
 xen/include/xen/device_tree.h      | 23 ++++++++++
 3 files changed, 183 insertions(+), 2 deletions(-)

-- 
2.7.4


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

[PATCH 1/2] xen: devicetree: Introduce a helper to translate PCI requester ID

[Wei Chen]

Each PCI(e) device under a root complex is uniquely identified by its
Requester ID (AKA RID). A Requester ID is a triplet of a Bus number,
Device number, and Function number. IOMMUs may distinguish PCI devices
through sideband data derived from the Requester ID. While a given PCI
device can only master through one IOMMU, a root complex may split
masters across a set of IOMMUs.

The generic 'iommus' property is using to describe this relationship.
This helper will be used to parse and map PCI Requester ID to IOMMU
match ID in later patches.

This patch is based on Linux of_pci.c:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/of_pci.c
The commit id is: 987068fcbdb7a085bb11151b91dc6f4c956c4a1b

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/common/device_tree.c      | 89 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h | 23 +++++++++++
 2 files changed, 112 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 7b009ea..bf95cda 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1663,6 +1663,95 @@ int dt_parse_phandle_with_args(const struct dt_device_node *np,
                                         index, out_args);
 }
 
+#define pr_err(fmt, ...) printk(XENLOG_ERR fmt, ## __VA_ARGS__)
+#define pr_info(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
+#define pr_debug(fmt, ...) printk(XENLOG_DEBUG fmt, ## __VA_ARGS__)
+
+int dt_pci_map_rid(struct dt_device_node *np, u32 rid,
+           const char *map_name, const char *map_mask_name,
+           struct dt_device_node **target, u32 *id_out)
+{
+    u32 map_mask, masked_rid, map_len;
+    const __be32 *map = NULL;
+
+    if ( !np || !map_name || (!target && !id_out) )
+        return -EINVAL;
+
+    map = dt_get_property(np, map_name, &map_len);
+    if ( !map )
+    {
+        if (target)
+            return -ENODEV;
+        /* Otherwise, no map implies no translation */
+        *id_out = rid;
+        return 0;
+    }
+
+    if ( !map_len || map_len % (4 * sizeof(*map)) )
+    {
+        pr_err("%s: Error: Bad %s length: %d\n", np->full_name,
+               map_name, map_len);
+        return -EINVAL;
+    }
+
+    /*
+     * Can be overridden by "{iommu,msi}-map-mask" property.
+     * If of_property_read_u32() fails, the default is used.
+     */
+    if ( !map_mask_name ||
+         !dt_property_read_u32(np, map_mask_name, &map_mask) )
+        /* The default is to select all bits. */
+        map_mask = 0xffffffff;
+
+    masked_rid = map_mask & rid;
+    for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4 )
+    {
+        struct dt_device_node *phandle_node;
+        u32 rid_base = be32_to_cpup(map + 0);
+        u32 phandle = be32_to_cpup(map + 1);
+        u32 out_base = be32_to_cpup(map + 2);
+        u32 rid_len = be32_to_cpup(map + 3);
+
+        if ( rid_base & ~map_mask )
+        {
+            pr_err("%s: Invalid %s translation - %s-mask (0x%x) ignores rid-base (0x%x)\n",
+                    np->full_name, map_name, map_name,
+                    map_mask, rid_base);
+            return -EFAULT;
+        }
+
+        if ( masked_rid < rid_base || masked_rid >= rid_base + rid_len )
+            continue;
+
+        phandle_node = dt_find_node_by_phandle(phandle);
+        if ( !phandle_node )
+            return -ENODEV;
+
+        if ( target )
+        {
+            if ( *target == NULL )
+                *target = phandle_node;
+
+            if ( *target != phandle_node )
+                continue;
+        }
+
+        if ( id_out )
+            *id_out = masked_rid - rid_base + out_base;
+
+        pr_info("%s: %s, using mask %08x, rid-base: %08x, out-base: %08x, length: %08x, rid: %08x -> %08x\n",
+                np->full_name, map_name, map_mask, rid_base, out_base,
+                rid_len, rid, *id_out);
+        return 0;
+    }
+
+    pr_err("%s: Invalid %s translation - no match for rid 0x%x on %s\n",
+           np->full_name, map_name, rid,
+           target && *target ? (*target)->full_name : "any target");
+
+    return -EFAULT;
+}
+
 /**
  * unflatten_dt_node - Alloc and populate a device_node from the flat tree
  * @fdt: The parent device tree blob
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 0aecbe0..0bddd7f 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -486,6 +486,29 @@ int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, uint32_t u_plen,
                           struct dt_device_node **node);
 
 /**
+ * dt_pci_map_rid - Translate a requester ID through a downstream mapping.
+ * @np: root complex device node.
+ * @rid: PCI requester ID to map.
+ * @map_name: property name of the map to use.
+ * @map_mask_name: optional property name of the mask to use.
+ * @target: optional pointer to a target device node.
+ * @id_out: optional pointer to receive the translated ID.
+ *
+ * Given a PCI requester ID, look up the appropriate implementation-defined
+ * platform ID and/or the target device which receives transactions on that
+ * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
+ * @id_out may be NULL if only the other is required. If @target points to
+ * a non-NULL device node pointer, only entries targeting that node will be
+ * matched; if it points to a NULL value, it will receive the device node of
+ * the first matching target phandle, with a reference held.
+ *
+ * Return: 0 on success or a standard error code on failure.
+ */
+int dt_pci_map_rid(struct dt_device_node *np, u32 rid,
+           const char *map_name, const char *map_mask_name,
+           struct dt_device_node **target, u32 *id_out);
+
+/**
  * dt_get_parent - Get a node's parent if any
  * @node: Node to get parent
  *
-- 
2.7.4


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

[PATCH 2/2] xen/arm: smmu: Parse generic iommu binding for PCI devices

[Wei Chen]

The legacy smmu binding will be deprecated in favour of the generic
"iommus" binding. So we need a new helper to parse generic iommu
bindings. When the system detects the SMMU is using generic iommu
binding, this helper will be called when this platform device is
assiged to a guest.

This patch is based on Linux of_iommu.c:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/of_iommu.c
The commit id is:
2a0c57545a291f257cd231b1c4b18285b84608d8

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 73 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 71 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 25f2207..50ff997 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2750,7 +2750,72 @@ static int arm_smmu_platform_iommu_init(struct device *dev)
 	return 0;
 }
 
-static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
+/*
+ * Currently, we haven't supported PCI device on ARM. So this is the
+ * temporary function to get device node of pci bridge device for
+ * function verification only
+ */
+static struct device_node *pci_get_bridge_device_node(struct device *dev)
+{
+	struct device_node *dt_dev;
+	const char *type_str;
+
+	dt_for_each_device_node(dt_host, dt_dev) {
+		/* Return the first pci bridge device node simply */
+		if (!dt_property_read_string(dt_dev, "device_type", &type_str) &&
+			!strcmp(type_str, "pci"))
+			return dt_dev;
+	}
+
+	return NULL;
+}
+
+#define PCI_DEVID(bus, devfn)  ((((u16)(bus)) << 8) | (devfn))
+
+static int arm_smmu_pci_iommu_init(struct device *dev, u8 devfn)
+{
+	struct device_node *bridge_np;
+	struct of_phandle_args iommu_spec;
+	struct pci_dev *pdev = to_pci_dev(dev);
+	int ret;
+
+	bridge_np = pci_get_bridge_device_node(dev);
+	if (!bridge_np) {
+		dev_err(dev, "Cloud not find the pci bridge device node!\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Start by tracing the RID alias down the PCI topology as
+	 * far as the host bridge whose OF node we have...
+	 * (we're not even attempting to handle multi-alias devices yet)
+	 */
+	iommu_spec.args_count = 1;
+	iommu_spec.np = bridge_np;
+	ret = __arm_smmu_get_pci_sid(pdev, PCI_DEVID(pdev->bus, devfn),
+					&iommu_spec.args[0]);
+	if (ret) {
+		dev_err(dev, "Get pci requester ID failed, err=%d!\n", ret);
+		return ret;
+	}
+
+	/*
+	 * ...then find out what that becomes once it escapes the PCI
+	 * bus into the system beyond, and which IOMMU it ends up at.
+	 */
+	iommu_spec.np = NULL;
+	ret = dt_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
+					"iommu-map-mask", &iommu_spec.np,
+					iommu_spec.args);
+	if (ret) {
+		dev_err(dev, "Do pci map rid failed, err=%d\n", ret);
+		return ret;
+	}
+
+	return arm_smmu_of_xlate(dev, &iommu_spec);
+}
+
+static int arm_smmu_xen_add_device(u8 devfn, struct device *dev)
 {
 	int ret;
 
@@ -2760,7 +2825,11 @@ static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
 	 * register Master IDs while this function had been invoked.
 	 */
 	if (using_generic_binding) {
-		ret = arm_smmu_platform_iommu_init(dev);
+		if (dev_is_pci(dev))
+			ret = arm_smmu_pci_iommu_init(dev, devfn);
+		else
+			ret = arm_smmu_platform_iommu_init(dev);
+
 		if (ret)
 			return ret;
 	}
-- 
2.7.4


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

Re: [PATCH 1/2] xen: devicetree: Introduce a helper to translate PCI requester ID

[Stefano Stabellini]

On Fri, 30 Jun 2017, Wei Chen wrote:
> Each PCI(e) device under a root complex is uniquely identified by its
> Requester ID (AKA RID). A Requester ID is a triplet of a Bus number,
> Device number, and Function number. IOMMUs may distinguish PCI devices
> through sideband data derived from the Requester ID. While a given PCI
> device can only master through one IOMMU, a root complex may split
> masters across a set of IOMMUs.
> 
> The generic 'iommus' property is using to describe this relationship.
> This helper will be used to parse and map PCI Requester ID to IOMMU
> match ID in later patches.
> 
> This patch is based on Linux of_pci.c:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/of_pci.c
> The commit id is: 987068fcbdb7a085bb11151b91dc6f4c956c4a1b
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>  xen/common/device_tree.c      | 89 +++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h | 23 +++++++++++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 7b009ea..bf95cda 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1663,6 +1663,95 @@ int dt_parse_phandle_with_args(const struct dt_device_node *np,
>                                          index, out_args);
>  }
>  
> +#define pr_err(fmt, ...) printk(XENLOG_ERR fmt, ## __VA_ARGS__)
> +#define pr_info(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
> +#define pr_debug(fmt, ...) printk(XENLOG_DEBUG fmt, ## __VA_ARGS__)

I wouldn't define pr_* in device_tree.c just for this function. I would
use printk(XENLOG_* directly and dt_dprintk.


> +int dt_pci_map_rid(struct dt_device_node *np, u32 rid,
> +           const char *map_name, const char *map_mask_name,
> +           struct dt_device_node **target, u32 *id_out)
> +{
> +    u32 map_mask, masked_rid, map_len;
> +    const __be32 *map = NULL;
> +
> +    if ( !np || !map_name || (!target && !id_out) )
> +        return -EINVAL;
> +
> +    map = dt_get_property(np, map_name, &map_len);
> +    if ( !map )
> +    {
> +        if (target)

if ( target )


> +            return -ENODEV;
> +        /* Otherwise, no map implies no translation */
> +        *id_out = rid;
> +        return 0;
> +    }
> +
> +    if ( !map_len || map_len % (4 * sizeof(*map)) )
> +    {
> +        pr_err("%s: Error: Bad %s length: %d\n", np->full_name,
> +               map_name, map_len);
> +        return -EINVAL;
> +    }
> +
> +    /*
> +     * Can be overridden by "{iommu,msi}-map-mask" property.
> +     * If of_property_read_u32() fails, the default is used.
> +     */
> +    if ( !map_mask_name ||
> +         !dt_property_read_u32(np, map_mask_name, &map_mask) )
> +        /* The default is to select all bits. */
> +        map_mask = 0xffffffff;
> +
> +    masked_rid = map_mask & rid;
> +    for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4 )
> +    {
> +        struct dt_device_node *phandle_node;
> +        u32 rid_base = be32_to_cpup(map + 0);
> +        u32 phandle = be32_to_cpup(map + 1);
> +        u32 out_base = be32_to_cpup(map + 2);
> +        u32 rid_len = be32_to_cpup(map + 3);
> +
> +        if ( rid_base & ~map_mask )
> +        {
> +            pr_err("%s: Invalid %s translation - %s-mask (0x%x) ignores rid-base (0x%x)\n",
> +                    np->full_name, map_name, map_name,
> +                    map_mask, rid_base);
> +            return -EFAULT;
> +        }
> +
> +        if ( masked_rid < rid_base || masked_rid >= rid_base + rid_len )
> +            continue;
> +
> +        phandle_node = dt_find_node_by_phandle(phandle);
> +        if ( !phandle_node )
> +            return -ENODEV;
> +
> +        if ( target )
> +        {
> +            if ( *target == NULL )
> +                *target = phandle_node;
> +
> +            if ( *target != phandle_node )
> +                continue;
> +        }
> +
> +        if ( id_out )
> +            *id_out = masked_rid - rid_base + out_base;
> +
> +        pr_info("%s: %s, using mask %08x, rid-base: %08x, out-base: %08x, length: %08x, rid: %08x -> %08x\n",
> +                np->full_name, map_name, map_mask, rid_base, out_base,
> +                rid_len, rid, *id_out);
> +        return 0;
> +    }
> +
> +    pr_err("%s: Invalid %s translation - no match for rid 0x%x on %s\n",
> +           np->full_name, map_name, rid,
> +           target && *target ? (*target)->full_name : "any target");
> +
> +    return -EFAULT;
> +}
> +
>  /**
>   * unflatten_dt_node - Alloc and populate a device_node from the flat tree
>   * @fdt: The parent device tree blob
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 0aecbe0..0bddd7f 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -486,6 +486,29 @@ int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, uint32_t u_plen,
>                            struct dt_device_node **node);
>  
>  /**
> + * dt_pci_map_rid - Translate a requester ID through a downstream mapping.
> + * @np: root complex device node.
> + * @rid: PCI requester ID to map.
> + * @map_name: property name of the map to use.
> + * @map_mask_name: optional property name of the mask to use.
> + * @target: optional pointer to a target device node.
> + * @id_out: optional pointer to receive the translated ID.
> + *
> + * Given a PCI requester ID, look up the appropriate implementation-defined
> + * platform ID and/or the target device which receives transactions on that
> + * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
> + * @id_out may be NULL if only the other is required. If @target points to
> + * a non-NULL device node pointer, only entries targeting that node will be
> + * matched; if it points to a NULL value, it will receive the device node of
> + * the first matching target phandle, with a reference held.
> + *
> + * Return: 0 on success or a standard error code on failure.
> + */
> +int dt_pci_map_rid(struct dt_device_node *np, u32 rid,
> +           const char *map_name, const char *map_mask_name,
> +           struct dt_device_node **target, u32 *id_out);
> +
> +/**
>   * dt_get_parent - Get a node's parent if any
>   * @node: Node to get parent
>   *
> -- 
> 2.7.4
> 

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

Re: [PATCH 2/2] xen/arm: smmu: Parse generic iommu binding for PCI devices

[Stefano Stabellini]

On Fri, 30 Jun 2017, Wei Chen wrote:
> The legacy smmu binding will be deprecated in favour of the generic
> "iommus" binding. So we need a new helper to parse generic iommu
> bindings. When the system detects the SMMU is using generic iommu
> binding, this helper will be called when this platform device is
> assiged to a guest.
> 
> This patch is based on Linux of_iommu.c:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/of_iommu.c
> The commit id is:
> 2a0c57545a291f257cd231b1c4b18285b84608d8
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 73 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 71 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 25f2207..50ff997 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2750,7 +2750,72 @@ static int arm_smmu_platform_iommu_init(struct device *dev)
>  	return 0;
>  }
>  
> -static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> +/*
> + * Currently, we haven't supported PCI device on ARM. So this is the

"Currently, we don't support PCI devices on ARM."


> + * temporary function to get device node of pci bridge device for
> + * function verification only

What do you mean by "for function verification only"?


> + */
> +static struct device_node *pci_get_bridge_device_node(struct device *dev)
> +{
> +	struct device_node *dt_dev;
> +	const char *type_str;
> +
> +	dt_for_each_device_node(dt_host, dt_dev) {
> +		/* Return the first pci bridge device node simply */
> +		if (!dt_property_read_string(dt_dev, "device_type", &type_str) &&
> +			!strcmp(type_str, "pci"))
> +			return dt_dev;

Not only this is very expensive, but also it doesn't seem to be even
checking that it found the right pci bridge before returning it. We
cannot just return the first one we find. If we need this function for
testing, then please separate it out to a different patch (that won't get
applied).


> +	}
> +
> +	return NULL;
> +}
> +
> +#define PCI_DEVID(bus, devfn)  ((((u16)(bus)) << 8) | (devfn))
> +
> +static int arm_smmu_pci_iommu_init(struct device *dev, u8 devfn)
> +{
> +	struct device_node *bridge_np;
> +	struct of_phandle_args iommu_spec;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int ret;
> +
> +	bridge_np = pci_get_bridge_device_node(dev);
> +	if (!bridge_np) {
> +		dev_err(dev, "Cloud not find the pci bridge device node!\n");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Start by tracing the RID alias down the PCI topology as
> +	 * far as the host bridge whose OF node we have...
> +	 * (we're not even attempting to handle multi-alias devices yet)
> +	 */
> +	iommu_spec.args_count = 1;
> +	iommu_spec.np = bridge_np;
> +	ret = __arm_smmu_get_pci_sid(pdev, PCI_DEVID(pdev->bus, devfn),
> +					&iommu_spec.args[0]);
> +	if (ret) {
> +		dev_err(dev, "Get pci requester ID failed, err=%d!\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * ...then find out what that becomes once it escapes the PCI
> +	 * bus into the system beyond, and which IOMMU it ends up at.
> +	 */
> +	iommu_spec.np = NULL;
> +	ret = dt_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
> +					"iommu-map-mask", &iommu_spec.np,
> +					iommu_spec.args);
> +	if (ret) {
> +		dev_err(dev, "Do pci map rid failed, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	return arm_smmu_of_xlate(dev, &iommu_spec);
> +}
> +
> +static int arm_smmu_xen_add_device(u8 devfn, struct device *dev)
>  {
>  	int ret;
>  
> @@ -2760,7 +2825,11 @@ static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
>  	 * register Master IDs while this function had been invoked.
>  	 */
>  	if (using_generic_binding) {
> -		ret = arm_smmu_platform_iommu_init(dev);
> +		if (dev_is_pci(dev))
> +			ret = arm_smmu_pci_iommu_init(dev, devfn);
> +		else
> +			ret = arm_smmu_platform_iommu_init(dev);
> +
>  		if (ret)
>  			return ret;
>  	}
> -- 
> 2.7.4
> 

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

Re: [PATCH 1/2] xen: devicetree: Introduce a helper to translate PCI requester ID

[Wei Chen]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月7日 4:26
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Julien Grall
> <Julien.Grall@arm.com>; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH 1/2] xen: devicetree: Introduce a helper to translate PCI
> requester ID
> 
> On Fri, 30 Jun 2017, Wei Chen wrote:
> > Each PCI(e) device under a root complex is uniquely identified by its
> > Requester ID (AKA RID). A Requester ID is a triplet of a Bus number,
> > Device number, and Function number. IOMMUs may distinguish PCI devices
> > through sideband data derived from the Requester ID. While a given PCI
> > device can only master through one IOMMU, a root complex may split
> > masters across a set of IOMMUs.
> >
> > The generic 'iommus' property is using to describe this relationship.
> > This helper will be used to parse and map PCI Requester ID to IOMMU
> > match ID in later patches.
> >
> > This patch is based on Linux of_pci.c:
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driver
> s/of/of_pci.c
> > The commit id is: 987068fcbdb7a085bb11151b91dc6f4c956c4a1b
> >
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > ---
> >  xen/common/device_tree.c      | 89
> +++++++++++++++++++++++++++++++++++++++++++
> >  xen/include/xen/device_tree.h | 23 +++++++++++
> >  2 files changed, 112 insertions(+)
> >
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 7b009ea..bf95cda 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -1663,6 +1663,95 @@ int dt_parse_phandle_with_args(const struct
> dt_device_node *np,
> >                                          index, out_args);
> >  }
> >
> > +#define pr_err(fmt, ...) printk(XENLOG_ERR fmt, ## __VA_ARGS__)
> > +#define pr_info(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
> > +#define pr_debug(fmt, ...) printk(XENLOG_DEBUG fmt, ## __VA_ARGS__)
> 
> I wouldn't define pr_* in device_tree.c just for this function. I would
> use printk(XENLOG_* directly and dt_dprintk.
> 

Ok, I will cleanup it.

> 
> > +int dt_pci_map_rid(struct dt_device_node *np, u32 rid,
> > +           const char *map_name, const char *map_mask_name,
> > +           struct dt_device_node **target, u32 *id_out)
> > +{
> > +    u32 map_mask, masked_rid, map_len;
> > +    const __be32 *map = NULL;
> > +
> > +    if ( !np || !map_name || (!target && !id_out) )
> > +        return -EINVAL;
> > +
> > +    map = dt_get_property(np, map_name, &map_len);
> > +    if ( !map )
> > +    {
> > +        if (target)
> 
> if ( target )

Oh, it's code-style mistake, I will fix it.

> 
> 
> > +            return -ENODEV;
> > +        /* Otherwise, no map implies no translation */
> > +        *id_out = rid;
> > +        return 0;
> > +    }
> > +
> > +    if ( !map_len || map_len % (4 * sizeof(*map)) )
> > +    {
> > +        pr_err("%s: Error: Bad %s length: %d\n", np->full_name,
> > +               map_name, map_len);
> > +        return -EINVAL;
> > +    }
> > +
> > +    /*
> > +     * Can be overridden by "{iommu,msi}-map-mask" property.
> > +     * If of_property_read_u32() fails, the default is used.
> > +     */
> > +    if ( !map_mask_name ||
> > +         !dt_property_read_u32(np, map_mask_name, &map_mask) )
> > +        /* The default is to select all bits. */
> > +        map_mask = 0xffffffff;
> > +
> > +    masked_rid = map_mask & rid;
> > +    for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4 )
> > +    {
> > +        struct dt_device_node *phandle_node;
> > +        u32 rid_base = be32_to_cpup(map + 0);
> > +        u32 phandle = be32_to_cpup(map + 1);
> > +        u32 out_base = be32_to_cpup(map + 2);
> > +        u32 rid_len = be32_to_cpup(map + 3);
> > +
> > +        if ( rid_base & ~map_mask )
> > +        {
> > +            pr_err("%s: Invalid %s translation - %s-mask (0x%x) ignores
> rid-base (0x%x)\n",
> > +                    np->full_name, map_name, map_name,
> > +                    map_mask, rid_base);
> > +            return -EFAULT;
> > +        }
> > +
> > +        if ( masked_rid < rid_base || masked_rid >= rid_base + rid_len )
> > +            continue;
> > +
> > +        phandle_node = dt_find_node_by_phandle(phandle);
> > +        if ( !phandle_node )
> > +            return -ENODEV;
> > +
> > +        if ( target )
> > +        {
> > +            if ( *target == NULL )
> > +                *target = phandle_node;
> > +
> > +            if ( *target != phandle_node )
> > +                continue;
> > +        }
> > +
> > +        if ( id_out )
> > +            *id_out = masked_rid - rid_base + out_base;
> > +
> > +        pr_info("%s: %s, using mask %08x, rid-base: %08x, out-base: %08x,
> length: %08x, rid: %08x -> %08x\n",
> > +                np->full_name, map_name, map_mask, rid_base, out_base,
> > +                rid_len, rid, *id_out);
> > +        return 0;
> > +    }
> > +
> > +    pr_err("%s: Invalid %s translation - no match for rid 0x%x on %s\n",
> > +           np->full_name, map_name, rid,
> > +           target && *target ? (*target)->full_name : "any target");
> > +
> > +    return -EFAULT;
> > +}
> > +
> >  /**
> >   * unflatten_dt_node - Alloc and populate a device_node from the flat tree
> >   * @fdt: The parent device tree blob
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 0aecbe0..0bddd7f 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -486,6 +486,29 @@ int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path,
> uint32_t u_plen,
> >                            struct dt_device_node **node);
> >
> >  /**
> > + * dt_pci_map_rid - Translate a requester ID through a downstream mapping.
> > + * @np: root complex device node.
> > + * @rid: PCI requester ID to map.
> > + * @map_name: property name of the map to use.
> > + * @map_mask_name: optional property name of the mask to use.
> > + * @target: optional pointer to a target device node.
> > + * @id_out: optional pointer to receive the translated ID.
> > + *
> > + * Given a PCI requester ID, look up the appropriate implementation-defined
> > + * platform ID and/or the target device which receives transactions on that
> > + * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
> > + * @id_out may be NULL if only the other is required. If @target points to
> > + * a non-NULL device node pointer, only entries targeting that node will be
> > + * matched; if it points to a NULL value, it will receive the device node
> of
> > + * the first matching target phandle, with a reference held.
> > + *
> > + * Return: 0 on success or a standard error code on failure.
> > + */
> > +int dt_pci_map_rid(struct dt_device_node *np, u32 rid,
> > +           const char *map_name, const char *map_mask_name,
> > +           struct dt_device_node **target, u32 *id_out);
> > +
> > +/**
> >   * dt_get_parent - Get a node's parent if any
> >   * @node: Node to get parent
> >   *
> > --
> > 2.7.4
> >

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

Re: [PATCH 2/2] xen/arm: smmu: Parse generic iommu binding for PCI devices

[Wei Chen]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 2017年7月7日 5:05
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Julien Grall
> <Julien.Grall@arm.com>; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH 2/2] xen/arm: smmu: Parse generic iommu binding for PCI
> devices
> 
> On Fri, 30 Jun 2017, Wei Chen wrote:
> > The legacy smmu binding will be deprecated in favour of the generic
> > "iommus" binding. So we need a new helper to parse generic iommu
> > bindings. When the system detects the SMMU is using generic iommu
> > binding, this helper will be called when this platform device is
> > assiged to a guest.
> >
> > This patch is based on Linux of_iommu.c:
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driver
> s/iommu/of_iommu.c
> > The commit id is:
> > 2a0c57545a291f257cd231b1c4b18285b84608d8
> >
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > ---
> >  xen/drivers/passthrough/arm/smmu.c | 73
> ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 71 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> b/xen/drivers/passthrough/arm/smmu.c
> > index 25f2207..50ff997 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -2750,7 +2750,72 @@ static int arm_smmu_platform_iommu_init(struct device
> *dev)
> >  	return 0;
> >  }
> >
> > -static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> > +/*
> > + * Currently, we haven't supported PCI device on ARM. So this is the
> 
> "Currently, we don't support PCI devices on ARM."
> 

Ok.

> 
> > + * temporary function to get device node of pci bridge device for
> > + * function verification only
> 
> What do you mean by "for function verification only"?
> 

See comment below.

> 
> > + */
> > +static struct device_node *pci_get_bridge_device_node(struct device *dev)
> > +{
> > +	struct device_node *dt_dev;
> > +	const char *type_str;
> > +
> > +	dt_for_each_device_node(dt_host, dt_dev) {
> > +		/* Return the first pci bridge device node simply */
> > +		if (!dt_property_read_string(dt_dev, "device_type", &type_str) &&
> > +			!strcmp(type_str, "pci"))
> > +			return dt_dev;
> 
> Not only this is very expensive, but also it doesn't seem to be even
> checking that it found the right pci bridge before returning it. We
> cannot just return the first one we find. If we need this function for
> testing, then please separate it out to a different patch (that won't get
> applied).
> 

I just used it to verify the translation of PCI RID on my board. The original
intention of placing this patch to this series is to do a reminder:
After we support PCI devices on ARM, we should fix this function to return
correct PCI bridge.

But now, it seems the worry is needless, I will separate this patch from this
Series.

> 
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +#define PCI_DEVID(bus, devfn)  ((((u16)(bus)) << 8) | (devfn))
> > +
> > +static int arm_smmu_pci_iommu_init(struct device *dev, u8 devfn)
> > +{
> > +	struct device_node *bridge_np;
> > +	struct of_phandle_args iommu_spec;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	int ret;
> > +
> > +	bridge_np = pci_get_bridge_device_node(dev);
> > +	if (!bridge_np) {
> > +		dev_err(dev, "Cloud not find the pci bridge device node!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/*
> > +	 * Start by tracing the RID alias down the PCI topology as
> > +	 * far as the host bridge whose OF node we have...
> > +	 * (we're not even attempting to handle multi-alias devices yet)
> > +	 */
> > +	iommu_spec.args_count = 1;
> > +	iommu_spec.np = bridge_np;
> > +	ret = __arm_smmu_get_pci_sid(pdev, PCI_DEVID(pdev->bus, devfn),
> > +					&iommu_spec.args[0]);
> > +	if (ret) {
> > +		dev_err(dev, "Get pci requester ID failed, err=%d!\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * ...then find out what that becomes once it escapes the PCI
> > +	 * bus into the system beyond, and which IOMMU it ends up at.
> > +	 */
> > +	iommu_spec.np = NULL;
> > +	ret = dt_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
> > +					"iommu-map-mask", &iommu_spec.np,
> > +					iommu_spec.args);
> > +	if (ret) {
> > +		dev_err(dev, "Do pci map rid failed, err=%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return arm_smmu_of_xlate(dev, &iommu_spec);
> > +}
> > +
> > +static int arm_smmu_xen_add_device(u8 devfn, struct device *dev)
> >  {
> >  	int ret;
> >
> > @@ -2760,7 +2825,11 @@ static int arm_smmu_xen_add_device(u8 devfn, struct
> device*dev)
> >  	 * register Master IDs while this function had been invoked.
> >  	 */
> >  	if (using_generic_binding) {
> > -		ret = arm_smmu_platform_iommu_init(dev);
> > +		if (dev_is_pci(dev))
> > +			ret = arm_smmu_pci_iommu_init(dev, devfn);
> > +		else
> > +			ret = arm_smmu_platform_iommu_init(dev);
> > +
> >  		if (ret)
> >  			return ret;
> >  	}
> > --
> > 2.7.4
> >

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

Re: [PATCH 2/2] xen/arm: smmu: Parse generic iommu binding for PCI devices

[Julien Grall]

On 07/07/2017 07:06 AM, Wei Chen wrote:
> Hi Stefano,
> 
>> -----Original Message-----
>> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
>> Sent: 2017年7月7日 5:05
>> To: Wei Chen <Wei.Chen@arm.com>
>> Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Julien Grall
>> <Julien.Grall@arm.com>; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
>> <Kaly.Xin@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH 2/2] xen/arm: smmu: Parse generic iommu binding for PCI
>> devices
>> 
>> On Fri, 30 Jun 2017, Wei Chen wrote:
>> > The legacy smmu binding will be deprecated in favour of the generic
>> > "iommus" binding. So we need a new helper to parse generic iommu
>> > bindings. When the system detects the SMMU is using generic iommu
>> > binding, this helper will be called when this platform device is
>> > assiged to a guest.
>> >
>> > This patch is based on Linux of_iommu.c:
>> >
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driver
>> s/iommu/of_iommu.c
>> > The commit id is:
>> > 2a0c57545a291f257cd231b1c4b18285b84608d8
>> >
>> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>> > ---
>> >  xen/drivers/passthrough/arm/smmu.c | 73
>> ++++++++++++++++++++++++++++++++++++--
>> >  1 file changed, 71 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/xen/drivers/passthrough/arm/smmu.c
>> b/xen/drivers/passthrough/arm/smmu.c
>> > index 25f2207..50ff997 100644
>> > --- a/xen/drivers/passthrough/arm/smmu.c
>> > +++ b/xen/drivers/passthrough/arm/smmu.c
>> > @@ -2750,7 +2750,72 @@ static int arm_smmu_platform_iommu_init(struct device
>> *dev)
>> >      return 0;
>> >  }
>> >
>> > -static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
>> > +/*
>> > + * Currently, we haven't supported PCI device on ARM. So this is the
>> 
>> "Currently, we don't support PCI devices on ARM."
>> 
> 
> Ok.
> 
>> 
>> > + * temporary function to get device node of pci bridge device for
>> > + * function verification only
>> 
>> What do you mean by "for function verification only"?
>> 
> 
> See comment below.
> 
>> 
>> > + */
>> > +static struct device_node *pci_get_bridge_device_node(struct device *dev)
>> > +{
>> > +   struct device_node *dt_dev;
>> > +   const char *type_str;
>> > +
>> > +   dt_for_each_device_node(dt_host, dt_dev) {
>> > +           /* Return the first pci bridge device node simply */
>> > +           if (!dt_property_read_string(dt_dev, "device_type", &type_str) &&
>> > +                   !strcmp(type_str, "pci"))
>> > +                   return dt_dev;
>> 
>> Not only this is very expensive, but also it doesn't seem to be even
>> checking that it found the right pci bridge before returning it. We
>> cannot just return the first one we find. If we need this function for
>> testing, then please separate it out to a different patch (that won't get
>> applied).
>> 
> 
> I just used it to verify the translation of PCI RID on my board. The 
> original
> intention of placing this patch to this series is to do a reminder:
> After we support PCI devices on ARM, we should fix this function to return
> correct PCI bridge.
> 
> But now, it seems the worry is needless, I will separate this patch from 
> this
> Series.

Well, I am a little bit concerned to merge a code that we don't know  
whether it will work when PCI will be fully supported in Xen and until  
then would just be dead code.

I can see quite a few problems in this code, you assume BDF == RID. This  
is not true on all the platform. You should at least call  
pci_for_each_dma_alias. The function to_pci will return NULL, how can  
you get through the segfault?

But looking at the code, I am not even sure why this code is in ARM SMMU  
given that likely you will have to do for the same for all the SMMUs.  
Looking at Linux, they are calling of_iommu_xlate on each PCI devices.

Cheers,

-- 
Julien Grall

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