diff mbox

[RFC,4/6] xen/passthrough/arm: Introduce iommu_fwspec

Message ID 1496950247-8755-5-git-send-email-sgoel@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Goel, Sameer June 8, 2017, 7:30 p.m. UTC
Introduce a common structure to hold the fw (ACPI or DT) defined
configuration for SMMU hw. The current use case is for arm SMMUs. So,
making this architecture specific.

Based on Linux kernel commit 57f98d2f61e1: iommu: Introduce iommu_fwspec
Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
---
 xen/drivers/passthrough/arm/iommu.c | 57 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/device.h        |  1 +
 xen/include/xen/iommu.h             | 28 ++++++++++++++++++
 3 files changed, 86 insertions(+)

Comments

Julien Grall June 8, 2017, 8:02 p.m. UTC | #1
Hi,

On 08/06/2017 20:30, Sameer Goel wrote:
> Introduce a common structure to hold the fw (ACPI or DT) defined
> configuration for SMMU hw. The current use case is for arm SMMUs. So,
> making this architecture specific.
>
> Based on Linux kernel commit 57f98d2f61e1: iommu: Introduce iommu_fwspec
> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> ---
>  xen/drivers/passthrough/arm/iommu.c | 57 +++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/device.h        |  1 +
>  xen/include/xen/iommu.h             | 28 ++++++++++++++++++
>  3 files changed, 86 insertions(+)
>
> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index 95b1abb..edf70c2 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -73,3 +73,60 @@ int arch_iommu_populate_page_table(struct domain *d)
>      /* The IOMMU shares the p2m with the CPU */
>      return -ENOSYS;
>  }
> +
> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> +        const struct iommu_ops *ops)

If you put code in iommu.c then you need to respect the coding style of 
the file. So this should be correctly indented.

> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +    if (fwspec)

Coding style.

> +        return ops == fwspec->ops ? 0 : -EINVAL;
> +
> +    fwspec = xzalloc(struct iommu_fwspec);
> +    if (!fwspec)

Ditto.

> +        return -ENOMEM;
> +
> +    /* Ref counting for the dt device node is not needed */
> +
> +    /*of_node_get(to_of_node(iommu_fwnode));*/

I am a bit confused. You put this in a comment here and drop it in patch 
#5. So you probably want to drop here.

> +
> +    fwspec->iommu_fwnode = iommu_fwnode;
> +    fwspec->ops = ops;
> +    dev->iommu_fwspec = fwspec;

Newline here please.

> +    return 0;
> +}
> +
> +void iommu_fwspec_free(struct device *dev)
> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +    if (fwspec) {

Coding style.

> +        /*fwnode_handle_put(fwspec->iommu_fwnode);*/

And ditto for the comment.

> +        xfree(fwspec);
> +        dev->iommu_fwspec = NULL;
> +    }
> +}
> +
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)

As you don't respect the Linux coding style (you are using soft-tab). I 
see little point to stay close to Linux for this code as it would be 
difficult to port anyway.

So s/u32/uint32_t/

> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +    size_t size;
> +    int i;
> +
> +    if (!fwspec)

Coding style.

> +        return -EINVAL;
> +
> +    size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
> +    if (size > sizeof(*fwspec)) {

Coding style.

> +        fwspec = _xrealloc(dev->iommu_fwspec, size, sizeof(void *));

If you introduce _xrealloc (I will leave the REST maintainers decide 
whether they want it), then please introduce xrealloc macro.

> +        if (!fwspec)

Coding style.

> +            return -ENOMEM;
> +    }
> +
> +    for (i = 0; i < num_ids; i++)

Coding style.

> +        fwspec->ids[fwspec->num_ids + i] = ids[i];
> +
> +    fwspec->num_ids += num_ids;
> +    dev->iommu_fwspec = fwspec;

newline.

> +    return 0;
> +}
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 78c38fe..5027c87 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -21,6 +21,7 @@ struct device
>      struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>  #endif
>      struct fwnode_handle *fwnode; /*fw device node identifier */
> +    struct iommu_fwspec *iommu_fwspec;
>      struct dev_archdata archdata;
>  };
>
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 5803e3f..7ef9b93 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -224,4 +224,32 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>  extern struct spinlock iommu_pt_cleanup_lock;
>  extern struct page_list_head iommu_pt_cleanup_list;
>
> +/**
> + * Following block was ported from Linux to help with the implementation of
> + * arm64 iommu devices. Hence the architecture specific compile
> + */
> +
> +#if defined(CONFIG_ARM_64) || defined(CONFIG_ARM)

No ifdef ARCH in xen/* headers. If this is only used by ARM, then it 
should go in asm-arm/iommu.h.

> +/**
> + * struct iommu_fwspec - per-device IOMMU instance data
> + * @ops: ops for this device's IOMMU
> + * @iommu_fwnode: firmware handle for this device's IOMMU
> + * @iommu_priv: IOMMU driver private data for this device
> + * @num_ids: number of associated device IDs
> + * @ids: IDs which this device may present to the IOMMU
> + */
> +struct iommu_fwspec {
> +	const struct iommu_ops	*ops;
> +	struct fwnode_handle	*iommu_fwnode;
> +	void			*iommu_priv;
> +	unsigned int		num_ids;
> +	u32			ids[1];
> +};
> +
> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> +		      const struct iommu_ops *ops);
> +void iommu_fwspec_free(struct device *dev);
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> +
> +#endif
>  #endif /* _IOMMU_H_ */
>

Cheers,
diff mbox

Patch

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 95b1abb..edf70c2 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -73,3 +73,60 @@  int arch_iommu_populate_page_table(struct domain *d)
     /* The IOMMU shares the p2m with the CPU */
     return -ENOSYS;
 }
+
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
+        const struct iommu_ops *ops)
+{
+    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+    if (fwspec)
+        return ops == fwspec->ops ? 0 : -EINVAL;
+
+    fwspec = xzalloc(struct iommu_fwspec);
+    if (!fwspec)
+        return -ENOMEM;
+
+    /* Ref counting for the dt device node is not needed */
+
+    /*of_node_get(to_of_node(iommu_fwnode));*/
+
+    fwspec->iommu_fwnode = iommu_fwnode;
+    fwspec->ops = ops;
+    dev->iommu_fwspec = fwspec;
+    return 0;
+}
+
+void iommu_fwspec_free(struct device *dev)
+{
+    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+    if (fwspec) {
+        /*fwnode_handle_put(fwspec->iommu_fwnode);*/
+        xfree(fwspec);
+        dev->iommu_fwspec = NULL;
+    }
+}
+
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
+{
+    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+    size_t size;
+    int i;
+
+    if (!fwspec)
+        return -EINVAL;
+
+    size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
+    if (size > sizeof(*fwspec)) {
+        fwspec = _xrealloc(dev->iommu_fwspec, size, sizeof(void *));
+        if (!fwspec)
+            return -ENOMEM;
+    }
+
+    for (i = 0; i < num_ids; i++)
+        fwspec->ids[fwspec->num_ids + i] = ids[i];
+
+    fwspec->num_ids += num_ids;
+    dev->iommu_fwspec = fwspec;
+    return 0;
+}
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 78c38fe..5027c87 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -21,6 +21,7 @@  struct device
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
 #endif
     struct fwnode_handle *fwnode; /*fw device node identifier */
+    struct iommu_fwspec *iommu_fwspec;
     struct dev_archdata archdata;
 };
 
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5803e3f..7ef9b93 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -224,4 +224,32 @@  DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
 extern struct spinlock iommu_pt_cleanup_lock;
 extern struct page_list_head iommu_pt_cleanup_list;
 
+/**
+ * Following block was ported from Linux to help with the implementation of
+ * arm64 iommu devices. Hence the architecture specific compile
+ */
+
+#if defined(CONFIG_ARM_64) || defined(CONFIG_ARM)
+/**
+ * struct iommu_fwspec - per-device IOMMU instance data
+ * @ops: ops for this device's IOMMU
+ * @iommu_fwnode: firmware handle for this device's IOMMU
+ * @iommu_priv: IOMMU driver private data for this device
+ * @num_ids: number of associated device IDs
+ * @ids: IDs which this device may present to the IOMMU
+ */
+struct iommu_fwspec {
+	const struct iommu_ops	*ops;
+	struct fwnode_handle	*iommu_fwnode;
+	void			*iommu_priv;
+	unsigned int		num_ids;
+	u32			ids[1];
+};
+
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
+		      const struct iommu_ops *ops);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
+
+#endif
 #endif /* _IOMMU_H_ */