Re: [Xen-devel] [RFC v4 8/8] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-01-23 Thread Julien Grall 

Hi Sameer,

On 19/12/17 03:17, Sameer Goel wrote:

Pull common defines for SMMU drivers in a local header.


I was expected to see this patch before SMMUv3 is added. But I am ok 
with that too.


However, if you want to do some renaming this should be done separately. 
So it will be easier to know this patch is only consolidation.


But why do you need to do renaming at first in the SMMUv3? The code is new.

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC v4 8/8] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-01-16 Thread Manish Jaggi 



On 01/16/2018 07:10 PM, Julien Grall wrote:

Hi Manish,

On 16/01/18 13:27, Manish Jaggi wrote:



On 01/16/2018 06:44 PM, Julien Grall wrote:



On 16/01/18 12:40, Manish Jaggi wrote:

Hi Julien,


Hi,


On 01/16/2018 02:11 AM, Julien Grall wrote:



On 01/03/2018 05:34 AM, Manish Jaggi wrote:

Hi Sameer,


Hi Manish,


+    unsigned int type;
+
+    /* Dummy compatibility defines */
+    unsigned long pgsize_bitmap;
+    struct iommu_domain_geometry geometry;
+
+    atomic_t ref;
+    /* Used to link iommu_domain contexts for a same domain.
+ * There is at least one per-SMMU to used by the domain.
+ */
+    struct list_head    list;
+};
+
+/* Xen: Describes information required for a Xen domain */
+struct arm_smmu_xen_domain {
+    spinlock_t  lock;
+    /* List of iommu domains associated to this domain */
+    struct list_head    contexts;
Could we use a more verbose name, How about 
%s/contexts/iommu_domain_contexts/g ?


How about a much more verbose name...? This name is 21 letters and 
that's only for the field. Just imagine with the variable name 
before and a couple of indentation.
How about io_context? anything which makes it more verbose is ok 
with me.


I much prefer to stick with "contexts".
I am not able to understand why to use contexts for a list which has 
iommu_domain pointers.


Because the comment should have been read "iommu_domain context". As 
it is in other description.


If you are ok with this logic, we can rename all iommu_domain pointer 
variables in this file as context.


Why do you keep asking renaming when I clearly said multiple time that 
we *should not* rename any code coming from Linux. This is breaking 
the idea of keeping Xen and Linux SMMU driver close.


If you don't want to get SMMUv3 close to Linux. Then fine, but it 
means you have to use Xen coding style and dropping all necessary code.


But I am afraid this is not the solution I (and AFAIK Stefano) prefer 
because it adds burden on maintenance on Xen community.


I agree with the point that code imported from linux should be touched 
minimally, but If you look closely the problem is in naming of xen 
specific code that is added later.
Not in code imported from linux. So why we cant fix xen specific code 
naming?


Below are the few examples
a. static struct iommu_domain *arm_smmu_get_domain(struct domain *d,...)
This function use of domain is confusing.
This is not linux function,

a function named arm_smmu_get_domain takes a parameter of domain which 
represents a virtual machine and returns an iommu_domain pointer.

while the file still has a structure arm_smmu_domain.
In what way this function naming explains itself?

If you want to take a cue from linux see this code below
structarm_smmu_domain *smmu_domain = to_smmu_domain(domain); => The 
naming is quite clear and not confusing.
Also If you see there are few functions in xen specifc smmu code which 
are named correctly

Look at arm_smmu_iommu_domain_init(struct domain *d)

But arm_smmu_iommu_domain_init(struct domain *d) and arm_smmu_get_domain 
naming dont match,  they are both referring to iommu_domain


So my _two cents_ on the current use of structures / functions and 
variable names which use domain in xen specific code.


b. Similarly contexts in arm_smmu_xen_domain structure is not coming 
from linux code

c. arm_smmu_assign_dev is a xen specific function so it can be changed
d. Why id arm_smmu_xen_domain named the way it is. Does linux code 
include _linux_ in any structure name?



Cheers,




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC v4 8/8] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-01-16 Thread Julien Grall 

Hi Manish,

On 16/01/18 13:27, Manish Jaggi wrote:



On 01/16/2018 06:44 PM, Julien Grall wrote:



On 16/01/18 12:40, Manish Jaggi wrote:

Hi Julien,


Hi,


On 01/16/2018 02:11 AM, Julien Grall wrote:



On 01/03/2018 05:34 AM, Manish Jaggi wrote:

Hi Sameer,


Hi Manish,


+    unsigned int    type;
+
+    /* Dummy compatibility defines */
+    unsigned long pgsize_bitmap;
+    struct iommu_domain_geometry geometry;
+
+    atomic_t ref;
+    /* Used to link iommu_domain contexts for a same domain.
+ * There is at least one per-SMMU to used by the domain.
+ */
+    struct list_head    list;
+};
+
+/* Xen: Describes information required for a Xen domain */
+struct arm_smmu_xen_domain {
+    spinlock_t  lock;
+    /* List of iommu domains associated to this domain */
+    struct list_head    contexts;
Could we use a more verbose name, How about 
%s/contexts/iommu_domain_contexts/g ?


How about a much more verbose name...? This name is 21 letters and 
that's only for the field. Just imagine with the variable name 
before and a couple of indentation.
How about io_context? anything which makes it more verbose is ok with 
me.


I much prefer to stick with "contexts".
I am not able to understand why to use contexts for a list which has 
iommu_domain pointers.


Because the comment should have been read "iommu_domain context". As it 
is in other description.


If you are ok with this logic, we can rename all iommu_domain pointer 
variables in this file as context.


Why do you keep asking renaming when I clearly said multiple time that 
we *should not* rename any code coming from Linux. This is breaking the 
idea of keeping Xen and Linux SMMU driver close.


If you don't want to get SMMUv3 close to Linux. Then fine, but it means 
you have to use Xen coding style and dropping all necessary code.


But I am afraid this is not the solution I (and AFAIK Stefano) prefer 
because it adds burden on maintenance on Xen community.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC v4 8/8] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-01-16 Thread Manish Jaggi 



On 01/16/2018 06:44 PM, Julien Grall wrote:



On 16/01/18 12:40, Manish Jaggi wrote:

Hi Julien,


Hi,


On 01/16/2018 02:11 AM, Julien Grall wrote:



On 01/03/2018 05:34 AM, Manish Jaggi wrote:

Hi Sameer,


Hi Manish,


+    unsigned int    type;
+
+    /* Dummy compatibility defines */
+    unsigned long pgsize_bitmap;
+    struct iommu_domain_geometry geometry;
+
+    atomic_t ref;
+    /* Used to link iommu_domain contexts for a same domain.
+ * There is at least one per-SMMU to used by the domain.
+ */
+    struct list_head    list;
+};
+
+/* Xen: Describes information required for a Xen domain */
+struct arm_smmu_xen_domain {
+    spinlock_t  lock;
+    /* List of iommu domains associated to this domain */
+    struct list_head    contexts;
Could we use a more verbose name, How about 
%s/contexts/iommu_domain_contexts/g ?


How about a much more verbose name...? This name is 21 letters and 
that's only for the field. Just imagine with the variable name 
before and a couple of indentation.
How about io_context? anything which makes it more verbose is ok with 
me.


I much prefer to stick with "contexts".
I am not able to understand why to use contexts for a list which has 
iommu_domain pointers.
If you are ok with this logic, we can rename all iommu_domain pointer 
variables in this file as context.
It is not intuitive to use context for iommu_domain in the same file it 
is confusing.


smmu code in xen is not easy to read.
There are other places which confuse...

|struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv; What 
is the first impression from this variable xen_domain, does it refer to 
a VM ? It is confusing. |




Cheers,




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC v4 8/8] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-01-16 Thread Manish Jaggi 

Hi Julien,


On 01/16/2018 02:11 AM, Julien Grall wrote:



On 01/03/2018 05:34 AM, Manish Jaggi wrote:

Hi Sameer,


Hi Manish,


+    unsigned int    type;
+
+    /* Dummy compatibility defines */
+    unsigned long pgsize_bitmap;
+    struct iommu_domain_geometry geometry;
+
+    atomic_t ref;
+    /* Used to link iommu_domain contexts for a same domain.
+ * There is at least one per-SMMU to used by the domain.
+ */
+    struct list_head    list;
+};
+
+/* Xen: Describes information required for a Xen domain */
+struct arm_smmu_xen_domain {
+    spinlock_t  lock;
+    /* List of iommu domains associated to this domain */
+    struct list_head    contexts;
Could we use a more verbose name, How about 
%s/contexts/iommu_domain_contexts/g ?


How about a much more verbose name...? This name is 21 letters and 
that's only for the field. Just imagine with the variable name before 
and a couple of indentation.

How about io_context? anything which makes it more verbose is ok with me.


This is where comment are helpful, a developer can easily look for the 
structure/field to see a description of the field. So let's not make 
the code more horrible because you would have to split the line.


Cheers,




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC v4 8/8] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-01-02 Thread Manish Jaggi 

Hi Sameer,


On 12/19/2017 08:47 AM, Sameer Goel wrote:

Pull common defines for SMMU drivers in a local header.

Signed-off-by: Sameer Goel 
---
  xen/drivers/passthrough/arm/arm_smmu.h | 113 +
  xen/drivers/passthrough/arm/smmu-v3.c  |  96 ++--
  xen/drivers/passthrough/arm/smmu.c | 104 +-
  3 files changed, 121 insertions(+), 192 deletions(-)
  create mode 100644 xen/drivers/passthrough/arm/arm_smmu.h

diff --git a/xen/drivers/passthrough/arm/arm_smmu.h 
b/xen/drivers/passthrough/arm/arm_smmu.h
new file mode 100644
index 00..70f97e7d50
--- /dev/null
+++ b/xen/drivers/passthrough/arm/arm_smmu.h
@@ -0,0 +1,113 @@
+/**
+ * arm_smmu.h
+ *
+ * Common compatibility defines and data_structures for porting arm smmu
+ * drivers from Linux.
+ *
+ * Copyright (c) 2017 Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see .
+ */
+
+#ifndef __ARM_SMMU_H__
+#define __ARM_SMMU_H__
+
+/* Helpers to get device MMIO and IRQs */
+struct resource {
+u64 addr;
+u64 size;
+unsigned int type;
+};
+
+#define resource_size(res) ((res)->size)
+
+#define platform_device device
+
+#define IORESOURCE_MEM 0
+#define IORESOURCE_IRQ 1
+
+/* Stub out DMA domain related functions */
+#define iommu_get_dma_cookie(dom) 0
+#define iommu_put_dma_cookie(dom)
+
+#define VA_BITS 0   /* Only used for configuring stage-1 input size */
+
+#define MODULE_DEVICE_TABLE(type, name)
+#define module_param_named(name, value, type, perm)
+#define MODULE_PARM_DESC(_parm, desc)
+
+static void __iomem *devm_ioremap_resource(struct device *dev,
+   struct resource *res)
+{
+void __iomem *ptr;
+
+if ( !res || res->type != IORESOURCE_MEM )
+{
+dev_err(dev, "Invalid resource\n");
+return ERR_PTR(-EINVAL);
+}
+
+ptr = ioremap_nocache(res->addr, res->size);
+if ( !ptr )
+{
+dev_err(dev,
+"ioremap failed (addr 0x%"PRIx64" size 0x%"PRIx64")\n",
+res->addr, res->size);
+return ERR_PTR(-ENOMEM);
+}
+
+return ptr;
+}
+
+/*
+ * Domain type definitions. Not really needed for Xen, defining to port
+ * Linux code as-is
+ */
+#define IOMMU_DOMAIN_UNMANAGED 0
+#define IOMMU_DOMAIN_DMA 1
+#define IOMMU_DOMAIN_IDENTITY 2
+
+/* Xen: Compatibility define for iommu_domain_geometry.*/
+struct iommu_domain_geometry {
+dma_addr_t aperture_start; /* First address that can be mapped*/
+dma_addr_t aperture_end;   /* Last address that can be mapped */
+bool force_aperture;   /* DMA only allowed in mappable range? */
+};
+
+/* Xen: Dummy iommu_domain */
+struct iommu_domain {
+/* Runtime SMMU configuration for this iommu_domain */
+struct arm_smmu_domain  *priv;
Do we need to call it priv? IMHO as there are too many structures wtih 
_domain and in the bigger goal of making the code intuitive

can we remove priv to something more verbose as smmu_domain.


+unsigned inttype;
+
+/* Dummy compatibility defines */
+unsigned long pgsize_bitmap;
+struct iommu_domain_geometry geometry;
+
+atomic_t ref;
+/* Used to link iommu_domain contexts for a same domain.
+ * There is at least one per-SMMU to used by the domain.
+ */
+struct list_headlist;
+};
+
+/* Xen: Describes information required for a Xen domain */
+struct arm_smmu_xen_domain {
+spinlock_t  lock;
+/* List of iommu domains associated to this domain */
+struct list_headcontexts;
Could we use a more verbose name, How about 
%s/contexts/iommu_domain_contexts/g ?

+};
+
+#endif /* __ARM_SMMU_H__ */
+
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
b/xen/drivers/passthrough/arm/smmu-v3.c
index 3488184ad4..6e705f63a3 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -49,20 +49,7 @@
  #include 
  #include 
  
-

-/* Xen: Helpers to get device MMIO and IRQs */
-struct resource {
-   u64 addr;
-   u64 size;
-   unsigned int type;
-};
-
-#define resource_size(res) ((res)->size)
-
-#define platform_device device
-
-#define IORESOURCE_MEM 0
-#define IORESOURCE_IRQ 1
+#include "arm_smmu.h" /* Not a self contained header. So last in the list

[Xen-devel] [RFC v4 8/8] drivers/passthrough/arm: Refactor code for arm smmu drivers

2017-12-18 Thread Sameer Goel 
Pull common defines for SMMU drivers in a local header.

Signed-off-by: Sameer Goel 
---
 xen/drivers/passthrough/arm/arm_smmu.h | 113 +
 xen/drivers/passthrough/arm/smmu-v3.c  |  96 ++--
 xen/drivers/passthrough/arm/smmu.c | 104 +-
 3 files changed, 121 insertions(+), 192 deletions(-)
 create mode 100644 xen/drivers/passthrough/arm/arm_smmu.h

diff --git a/xen/drivers/passthrough/arm/arm_smmu.h 
b/xen/drivers/passthrough/arm/arm_smmu.h
new file mode 100644
index 00..70f97e7d50
--- /dev/null
+++ b/xen/drivers/passthrough/arm/arm_smmu.h
@@ -0,0 +1,113 @@
+/**
+ * arm_smmu.h
+ *
+ * Common compatibility defines and data_structures for porting arm smmu
+ * drivers from Linux.
+ *
+ * Copyright (c) 2017 Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see .
+ */
+
+#ifndef __ARM_SMMU_H__
+#define __ARM_SMMU_H__
+
+/* Helpers to get device MMIO and IRQs */
+struct resource {
+u64 addr;
+u64 size;
+unsigned int type;
+};
+
+#define resource_size(res) ((res)->size)
+
+#define platform_device device
+
+#define IORESOURCE_MEM 0
+#define IORESOURCE_IRQ 1
+
+/* Stub out DMA domain related functions */
+#define iommu_get_dma_cookie(dom) 0
+#define iommu_put_dma_cookie(dom)
+
+#define VA_BITS 0   /* Only used for configuring stage-1 input size */
+
+#define MODULE_DEVICE_TABLE(type, name)
+#define module_param_named(name, value, type, perm)
+#define MODULE_PARM_DESC(_parm, desc)
+
+static void __iomem *devm_ioremap_resource(struct device *dev,
+   struct resource *res)
+{
+void __iomem *ptr;
+
+if ( !res || res->type != IORESOURCE_MEM )
+{
+dev_err(dev, "Invalid resource\n");
+return ERR_PTR(-EINVAL);
+}
+
+ptr = ioremap_nocache(res->addr, res->size);
+if ( !ptr )
+{
+dev_err(dev,
+"ioremap failed (addr 0x%"PRIx64" size 0x%"PRIx64")\n",
+res->addr, res->size);
+return ERR_PTR(-ENOMEM);
+}
+
+return ptr;
+}
+
+/*
+ * Domain type definitions. Not really needed for Xen, defining to port
+ * Linux code as-is
+ */
+#define IOMMU_DOMAIN_UNMANAGED 0
+#define IOMMU_DOMAIN_DMA 1
+#define IOMMU_DOMAIN_IDENTITY 2
+
+/* Xen: Compatibility define for iommu_domain_geometry.*/
+struct iommu_domain_geometry {
+dma_addr_t aperture_start; /* First address that can be mapped*/
+dma_addr_t aperture_end;   /* Last address that can be mapped */
+bool force_aperture;   /* DMA only allowed in mappable range? */
+};
+
+/* Xen: Dummy iommu_domain */
+struct iommu_domain {
+/* Runtime SMMU configuration for this iommu_domain */
+struct arm_smmu_domain  *priv;
+unsigned inttype;
+
+/* Dummy compatibility defines */
+unsigned long pgsize_bitmap;
+struct iommu_domain_geometry geometry;
+
+atomic_t ref;
+/* Used to link iommu_domain contexts for a same domain.
+ * There is at least one per-SMMU to used by the domain.
+ */
+struct list_headlist;
+};
+
+/* Xen: Describes information required for a Xen domain */
+struct arm_smmu_xen_domain {
+spinlock_t  lock;
+/* List of iommu domains associated to this domain */
+struct list_headcontexts;
+};
+
+#endif /* __ARM_SMMU_H__ */
+
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
b/xen/drivers/passthrough/arm/smmu-v3.c
index 3488184ad4..6e705f63a3 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -49,20 +49,7 @@
 #include 
 #include 
 
-
-/* Xen: Helpers to get device MMIO and IRQs */
-struct resource {
-   u64 addr;
-   u64 size;
-   unsigned int type;
-};
-
-#define resource_size(res) ((res)->size)
-
-#define platform_device device
-
-#define IORESOURCE_MEM 0
-#define IORESOURCE_IRQ 1
+#include "arm_smmu.h" /* Not a self contained header. So last in the list */
 
 static struct resource *platform_get_resource(struct platform_device *pdev,
  unsigned int type,
@@ -192,81 +179,10 @@ void dmam_free_coherent(struct device *dev, size_t size, 
void *vaddr,
xfree(vaddr);
 }
 
-/* Xen: Stub out DMA domain related functions */
-#define