[Xen-devel,RFC,06/11] fwnode xen spacific changes

Message ID 20180102092809.1841-7-manish.jaggi@linaro.org
State New
Headers show
Series
  • acpi: arm: IORT Support for Xen
Related show

Commit Message

Manish Jaggi Jan. 2, 2018, 9:28 a.m.
From: Manish Jaggi <manish.jaggi@linaro.org>

Merge few more changes from linux kernel code (v4.14) into iommu.c
Modify code specifc to xen.

Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
 xen/drivers/passthrough/iommu.c | 75 +++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/device.h    | 11 ++++--
 xen/include/xen/iommu.h         | 22 ++++++++++++
 3 files changed, 106 insertions(+), 2 deletions(-)

Comments

Julien Grall Jan. 18, 2018, 6:51 p.m. | #1
Hi Manish,

Please use scripts/get_maintainers.pl to CC relevant maintainers. I have 
done it for you this time.


Title: s/spacific/specific/

On 02/01/18 09:28, manish.jaggi@linaro.org wrote:
> From: Manish Jaggi <manish.jaggi@linaro.org>
> 
> Merge few more changes from linux kernel code (v4.14) into iommu.c
> Modify code specifc to xen.

I appreciate you pick-up the series from Sameer. I would also have 
appreciated if you have addressed my remarks from there.

Sameer explain why he imported fwnode. This has been dropped here. Also,
I think you probably want a bit more context in the commit message about 
implement fwnode.h in common code.

Within this series, fwnode seems to only be used by Arm. So what would 
be the advantage to get that in xen/? Is it going to be used by x86 or 
taken advantage in common code?

> 
> Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
> ---
>   xen/drivers/passthrough/iommu.c | 75 +++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/device.h    | 11 ++++--
>   xen/include/xen/iommu.h         | 22 ++++++++++++
>   3 files changed, 106 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 1aecf7cf34..408f44106d 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -13,6 +13,7 @@
>    */
>   
>   #include <xen/sched.h>
> +#include <xen/fwnode.h>
>   #include <xen/iommu.h>
>   #include <xen/paging.h>
>   #include <xen/guest_access.h>
> @@ -507,6 +508,80 @@ static void iommu_dump_p2m_table(unsigned char key)
>       }
>   }
>   
> +/**
> + * fwnode_handle_put - Drop reference to a device node
> + * @fwnode: Pointer to the device node to drop the reference to.
> + *
> + * This has to be used when terminating device_for_each_child_node() iteration
> + * with break or return to prevent stale device node references from being left
> + * behind.
> + */
> +void fwnode_handle_put(struct fwnode_handle *fwnode)
> +{
> +        fwnode_call_void_op(fwnode, put);

This file is following Xen coding style. And therefore you should use 
Xen coding.

> +}
> +
> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
> +{
> +       return iommu_get_ops();
> +}
> +
> +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 = kzalloc(sizeof(*fwspec), GFP_KERNEL);

You define kzalloc in a later patch and hence break the build. *All* the 
patches should build one by one to help bisectability.

But given the side of the code and the fact you are going to fix the 
coding style. It might be easier to use Xen name here.

> +       if (!fwspec)
> +               return -ENOMEM;
> +#if 0
> +       of_node_get(to_of_node(iommu_fwnode));
> +#endif
> +       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);
> +               kfree(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)) {
> +                //TBD: fwspec = krealloc(dev->iommu_fwspec, size, GFP_KERNEL);

Hmmm?

> +                if (!fwspec)
> +                        return -ENOMEM;
> +
> +                dev->iommu_fwspec = fwspec;
> +        }
> +
> +        for (i = 0; i < num_ids; i++)
> +                fwspec->ids[fwspec->num_ids + i] = ids[i];
> +
> +        fwspec->num_ids += num_ids;
> +        return 0;
> +
> +}
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 6734ae8efd..f78482ca0c 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -6,6 +6,8 @@
>   enum device_type
>   {
>       DEV_DT,
> +    DEV_ACPI,

You don't use DEV_ACPI in this patch. So why is there?

> +    DEV_PCI,
>   };
>   
>   struct dev_archdata {
> @@ -18,8 +20,13 @@ struct device
>       enum device_type type;
>   #ifdef CONFIG_HAS_DEVICE_TREE
>       struct dt_device_node *of_node; /* Used by drivers imported from Linux */

As said on Sameer's patches, I was expecting a todo in the code after 
the discussion about leave of_node here.

> +#endif
> +#ifdef CONFIG_ACPI
> +    void *acpi_node;

You don't use acpi_node in this patch. So why is it there?

>   #endif
>       struct dev_archdata archdata;
> +    struct fwnode_handle *fwnode; /* firmware device node */

Ditto.

> +    struct iommu_fwspec *iommu_fwspec;
>   };
>   
>   typedef struct device device_t;
> @@ -27,8 +34,8 @@ typedef struct device device_t;
>   #include <xen/device_tree.h>
>   
>   /* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */
> -#define dev_is_pci(dev) ((void)(dev), 0)
> -#define dev_is_dt(dev)  ((dev->type == DEV_DT)
> +#define dev_is_pci(dev) (dev->type == DEV_PCI)
> +#define dev_is_dt(dev)  (dev->type == DEV_DT)

Those two changes does not belong to this patch. It is likely 2 separate 
patches:
     1# fixing dev_is_dt because of the missing parenthese
     2# implementing dev_is_dt

>   
>   enum device_class
>   {
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 33c8b221dc..56b169bae9 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -208,4 +208,26 @@ 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;
>   
> +/**
> + * 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);
> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
> +
>   #endif /* _IOMMU_H_ */
> 

Cheers,

Patch

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 1aecf7cf34..408f44106d 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -13,6 +13,7 @@ 
  */
 
 #include <xen/sched.h>
+#include <xen/fwnode.h>
 #include <xen/iommu.h>
 #include <xen/paging.h>
 #include <xen/guest_access.h>
@@ -507,6 +508,80 @@  static void iommu_dump_p2m_table(unsigned char key)
     }
 }
 
+/**
+ * fwnode_handle_put - Drop reference to a device node
+ * @fwnode: Pointer to the device node to drop the reference to.
+ *
+ * This has to be used when terminating device_for_each_child_node() iteration
+ * with break or return to prevent stale device node references from being left
+ * behind.
+ */
+void fwnode_handle_put(struct fwnode_handle *fwnode)
+{
+        fwnode_call_void_op(fwnode, put);
+}
+
+const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
+{
+       return iommu_get_ops();
+}
+
+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 = kzalloc(sizeof(*fwspec), GFP_KERNEL);
+       if (!fwspec)
+               return -ENOMEM;
+#if 0
+       of_node_get(to_of_node(iommu_fwnode));
+#endif
+       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);
+               kfree(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)) {
+                //TBD: fwspec = krealloc(dev->iommu_fwspec, size, GFP_KERNEL);
+                if (!fwspec)
+                        return -ENOMEM;
+
+                dev->iommu_fwspec = fwspec;
+        }
+
+        for (i = 0; i < num_ids; i++)
+                fwspec->ids[fwspec->num_ids + i] = ids[i];
+
+        fwspec->num_ids += num_ids;
+        return 0;
+
+}
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 6734ae8efd..f78482ca0c 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -6,6 +6,8 @@ 
 enum device_type
 {
     DEV_DT,
+    DEV_ACPI,
+    DEV_PCI,
 };
 
 struct dev_archdata {
@@ -18,8 +20,13 @@  struct device
     enum device_type type;
 #ifdef CONFIG_HAS_DEVICE_TREE
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
+#endif
+#ifdef CONFIG_ACPI
+    void *acpi_node;
 #endif
     struct dev_archdata archdata;
+    struct fwnode_handle *fwnode; /* firmware device node */
+    struct iommu_fwspec *iommu_fwspec;
 };
 
 typedef struct device device_t;
@@ -27,8 +34,8 @@  typedef struct device device_t;
 #include <xen/device_tree.h>
 
 /* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */
-#define dev_is_pci(dev) ((void)(dev), 0)
-#define dev_is_dt(dev)  ((dev->type == DEV_DT)
+#define dev_is_pci(dev) (dev->type == DEV_PCI)
+#define dev_is_dt(dev)  (dev->type == DEV_DT)
 
 enum device_class
 {
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 33c8b221dc..56b169bae9 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -208,4 +208,26 @@  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;
 
+/**
+ * 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);
+const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
+
 #endif /* _IOMMU_H_ */