Re: [Xen-devel] [PATCH 6/7] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-05-18 Thread Sameer Goel 


On 2/9/2018 4:02 AM, Roger Pau Monné wrote:
> On Fri, Feb 09, 2018 at 10:51:01AM +, Julien Grall wrote:
>> Hi,
>>
>> On 02/09/2018 10:43 AM, Roger Pau Monné wrote:
 +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)
 +
 +#define dma_set_mask_and_coherent(d, b)   0
 +#define of_dma_is_coherent(n) 0
 +
 +static void __iomem *devm_ioremap_resource(struct device *dev,
 + struct resource *res)
>>> Aligment, please use spaces.
>>>
>>> Also, is __iomem needed here at all?
>> On Arm, we tend to add keep __iomem on pointer dealing with MMIO.
> I understand that you keep it when directly importing code from Linux,
> but this is Xen code, so unless this is done merely for consistency it
> seems quite pointless (__iomem is defined to nothing AFAICT).
>
>> [...]
>>
 +
 +#endif /* __ARM_SMMU_H__ */
 +
 diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
 b/xen/drivers/passthrough/arm/smmu-v3.c
 index f43485fe6e..f0a61521fb 100644
 --- a/xen/drivers/passthrough/arm/smmu-v3.c
 +++ b/xen/drivers/passthrough/arm/smmu-v3.c
 @@ -49,28 +49,7 @@
   #include 
   #include 
 -/* Alias to Xen device tree helpers */
 -#define device_node dt_device_node
 -#define of_phandle_args dt_phandle_args
 -#define of_device_id dt_device_match
 -#define of_match_node dt_match_node
 -#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, 
 pname, out))
 -#define of_property_read_bool dt_property_read_bool
 -#define of_parse_phandle_with_args dt_parse_phandle_with_args
 -
 -/* 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
>>> You introduce the above code in patch 5, and remove it in patch 6, is
>>> this really needed?
>>>
>>> Ie: why not simply introduce this code directly in this patch?
>> See https://lists.xen.org/archives/html/xen-devel/2018-01/msg02066.html
> Hm, OK, I'm not sure I follow that.
>
> AFAICT the above code is added in patch 5 so that the driver can be
> hooked up into the build system. Could we just hold off hooking the
> driver to the build system until patch 6, in order to avoid such
> addition and removal of code?
I just wanted this patch to be the unifying change between the SMMUv2 and 
SMMUv3 jargon. This allows me to keep some variable names as is from Linux 
kernel for the first checkin.

I agree that I can shuffle around some variables but since I was introducing 
this patch I refrained from it.

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


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

Re: [Xen-devel] [PATCH 6/7] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-03-01 Thread Julien Grall 

Hi,

On 09/02/18 03:10, Sameer Goel wrote:

Pull common defines for SMMU drives in a local header.


s/drivers/



Signed-off-by: Sameer Goel 
---
  xen/drivers/passthrough/arm/arm_smmu.h | 125 +
  xen/drivers/passthrough/arm/smmu-v3.c  |  96 +
  xen/drivers/passthrough/arm/smmu.c | 104 +--
  3 files changed, 128 insertions(+), 197 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..f49dceb5b4
--- /dev/null
+++ b/xen/drivers/passthrough/arm/arm_smmu.h
@@ -0,0 +1,125 @@
+/**
+ * ./arm_smmu.h


xen/drivers/passthrough/arm/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__
+
+


No need for 2 newline


+/* Alias to Xen device tree helpers */
+#define device_node dt_device_node
+#define of_phandle_args dt_phandle_args
+#define of_device_id dt_device_match
+#define of_match_node dt_match_node
+#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, pname, 
out))
+#define of_property_read_bool dt_property_read_bool
+#define of_parse_phandle_with_args dt_parse_phandle_with_args
+
+/* Helpers to get device MMIO and IRQs */
+struct resource {
+u64 addr;
+u64 size;


Please take the oppoturnity to switch to uint64_t.


+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)


Please avoid to implement empty macro. Use do { } while (0)


+
+#define VA_BITS0 /* Only used for configuring stage-1 input 
size */


This seems to only be dropped in the SMMUv2 driver.


+
+#define MODULE_DEVICE_TABLE(type, name)
+#define module_param_named(name, value, type, perm)
+#define MODULE_PARM_DESC(_parm, desc)


Should not this belong to the linux-compat.h?


+
+#define dma_set_mask_and_coherent(d, b)0
+#define of_dma_is_coherent(n)  0


Those 2 don't exist in the SMMUv2 driver.


+
+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 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;
+};
+
+#endif /* __ARM_SMMU_H__ */
+
diff --git a/xen/drivers/passthrou

Re: [Xen-devel] [PATCH 6/7] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-02-09 Thread Sameer Goel 


On 2/9/2018 3:51 AM, Julien Grall wrote:
>
>>
>>> diff --git a/xen/drivers/passthrough/arm/smmu.c 
>>> b/xen/drivers/passthrough/arm/smmu.c
>>> index ad956d5b8d..4c04391e21 100644
>>> --- a/xen/drivers/passthrough/arm/smmu.c
>>> +++ b/xen/drivers/passthrough/arm/smmu.c
>>> @@ -41,6 +41,7 @@
>>>   #include 
>>>   #include 
>>>   #include 
>>> +#include 
The above header included for the first time.
>>>   #include 
>>>   #include 
>>>   #include 
>>> @@ -51,36 +52,13 @@
>>>   #include 
>>>   #include 
>>>   +#include "arm_smmu.h" /* Not a self contained header. So last in the 
>>> list */
>>>   /* Xen: The below defines are redefined within the file. Undef it */
>>>   #undef SCTLR_AFE
>>>   #undef SCTLR_TRE
>>>   #undef SCTLR_M
>>>   #undef TTBCR_EAE
>>>   -/* Alias to Xen device tree helpers */
>>> -#define device_node dt_device_node
>>> -#define of_phandle_args dt_phandle_args
>>> -#define of_device_id dt_device_match
>>> -#define of_match_node dt_match_node
>>> -#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, 
>>> pname, out))
>>> -#define of_property_read_bool dt_property_read_bool
>>> -#define of_parse_phandle_with_args dt_parse_phandle_with_args
>>> -
>>> -/* 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
>>> -
>>>   static struct resource *platform_get_resource(struct platform_device 
>>> *pdev,
>>>     unsigned int type,
>>>     unsigned int num)
>>> @@ -118,58 +96,6 @@ static struct resource *platform_get_resource(struct 
>>> platform_device *pdev,
>>>     /* Xen: Helpers for IRQ functions */
>>>   #define request_irq(irq, func, flags, name, dev) request_irq(irq, flags, 
>>> func, name, dev)
>>> -#define free_irq release_irq
>>> -
>>> -enum irqreturn {
>>> -    IRQ_NONE    = (0 << 0),
>>> -    IRQ_HANDLED    = (1 << 0),
>>> -};
>>> -
>>> -typedef enum irqreturn irqreturn_t;
>>
>> You remove the irqreturn enum without adding any replacement, is this
>> really unused?
>
> It is used, so looks like the SMMU driver has not been build test it. Sameer, 
> please at least build test the changes you made in the SMMU driver.

It is build tested. The above defined now come from linux_compat.h. I 
introduced this with the smmu-v3 code changes as recommended by Roger on the 
RFC.
>
> Cheers,
>
Thanks,
Sameer

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

Re: [Xen-devel] [PATCH 6/7] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-02-09 Thread Julien Grall 

Hi,

On 02/09/2018 11:02 AM, Roger Pau Monné wrote:

On Fri, Feb 09, 2018 at 10:51:01AM +, Julien Grall wrote:

Hi,

On 02/09/2018 10:43 AM, Roger Pau Monné wrote:

+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_BITS0 /* 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)
+
+#define dma_set_mask_and_coherent(d, b)0
+#define of_dma_is_coherent(n)  0
+
+static void __iomem *devm_ioremap_resource(struct device *dev,
+  struct resource *res)


Aligment, please use spaces.

Also, is __iomem needed here at all?


On Arm, we tend to add keep __iomem on pointer dealing with MMIO.


I understand that you keep it when directly importing code from Linux,
but this is Xen code, so unless this is done merely for consistency it
seems quite pointless (__iomem is defined to nothing AFAICT).


We do it quite consistenly in Xen Arm code :). Have a look at the return 
of ioremap_* or read*/write* parameters.


While it might be defined to nothing today, I'd like to keep it because 
it could easily be defined to check the address space used.






[...]


+
+#endif /* __ARM_SMMU_H__ */
+
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
b/xen/drivers/passthrough/arm/smmu-v3.c
index f43485fe6e..f0a61521fb 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -49,28 +49,7 @@
   #include 
   #include 
-/* Alias to Xen device tree helpers */
-#define device_node dt_device_node
-#define of_phandle_args dt_phandle_args
-#define of_device_id dt_device_match
-#define of_match_node dt_match_node
-#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, pname, 
out))
-#define of_property_read_bool dt_property_read_bool
-#define of_parse_phandle_with_args dt_parse_phandle_with_args
-
-/* 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


You introduce the above code in patch 5, and remove it in patch 6, is
this really needed?

Ie: why not simply introduce this code directly in this patch?


See https://lists.xen.org/archives/html/xen-devel/2018-01/msg02066.html


Hm, OK, I'm not sure I follow that.

AFAICT the above code is added in patch 5 so that the driver can be
hooked up into the build system. Could we just hold off hooking the
driver to the build system until patch 6, in order to avoid such
addition and removal of code?


What matters is to know what is common between SMMUv2 and SMMUv3 driver. 
So it can be pulled in a separate headers.


IHMO, the both yours and his way are valid. TBH, I would have done a 
third way and move that patch before #5. But at this stage, this is a 
matter of taste, hence way I didn't push to reshuffle the series.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 6/7] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-02-09 Thread Roger Pau Monné 
On Fri, Feb 09, 2018 at 10:51:01AM +, Julien Grall wrote:
> Hi,
> 
> On 02/09/2018 10:43 AM, Roger Pau Monné wrote:
> > > +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)
> > > +
> > > +#define dma_set_mask_and_coherent(d, b)  0
> > > +#define of_dma_is_coherent(n)0
> > > +
> > > +static void __iomem *devm_ioremap_resource(struct device *dev,
> > > +struct resource *res)
> > 
> > Aligment, please use spaces.
> > 
> > Also, is __iomem needed here at all?
> 
> On Arm, we tend to add keep __iomem on pointer dealing with MMIO.

I understand that you keep it when directly importing code from Linux,
but this is Xen code, so unless this is done merely for consistency it
seems quite pointless (__iomem is defined to nothing AFAICT).

> 
> [...]
> 
> > > +
> > > +#endif /* __ARM_SMMU_H__ */
> > > +
> > > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> > > b/xen/drivers/passthrough/arm/smmu-v3.c
> > > index f43485fe6e..f0a61521fb 100644
> > > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > > @@ -49,28 +49,7 @@
> > >   #include 
> > >   #include 
> > > -/* Alias to Xen device tree helpers */
> > > -#define device_node dt_device_node
> > > -#define of_phandle_args dt_phandle_args
> > > -#define of_device_id dt_device_match
> > > -#define of_match_node dt_match_node
> > > -#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, 
> > > pname, out))
> > > -#define of_property_read_bool dt_property_read_bool
> > > -#define of_parse_phandle_with_args dt_parse_phandle_with_args
> > > -
> > > -/* 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
> > 
> > You introduce the above code in patch 5, and remove it in patch 6, is
> > this really needed?
> > 
> > Ie: why not simply introduce this code directly in this patch?
> 
> See https://lists.xen.org/archives/html/xen-devel/2018-01/msg02066.html

Hm, OK, I'm not sure I follow that.

AFAICT the above code is added in patch 5 so that the driver can be
hooked up into the build system. Could we just hold off hooking the
driver to the build system until patch 6, in order to avoid such
addition and removal of code?

Thanks, Roger.

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

Re: [Xen-devel] [PATCH 6/7] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-02-09 Thread Julien Grall 

Hi,

On 02/09/2018 10:43 AM, Roger Pau Monné wrote:

+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_BITS0 /* 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)
+
+#define dma_set_mask_and_coherent(d, b)0
+#define of_dma_is_coherent(n)  0
+
+static void __iomem *devm_ioremap_resource(struct device *dev,
+  struct resource *res)


Aligment, please use spaces.

Also, is __iomem needed here at all?


On Arm, we tend to add keep __iomem on pointer dealing with MMIO.

[...]


+
+#endif /* __ARM_SMMU_H__ */
+
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
b/xen/drivers/passthrough/arm/smmu-v3.c
index f43485fe6e..f0a61521fb 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -49,28 +49,7 @@
  #include 
  #include 
  
-/* Alias to Xen device tree helpers */

-#define device_node dt_device_node
-#define of_phandle_args dt_phandle_args
-#define of_device_id dt_device_match
-#define of_match_node dt_match_node
-#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, pname, 
out))
-#define of_property_read_bool dt_property_read_bool
-#define of_parse_phandle_with_args dt_parse_phandle_with_args
-
-/* 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


You introduce the above code in patch 5, and remove it in patch 6, is
this really needed?

Ie: why not simply introduce this code directly in this patch?


See https://lists.xen.org/archives/html/xen-devel/2018-01/msg02066.html




diff --git a/xen/drivers/passthrough/arm/smmu.c 
b/xen/drivers/passthrough/arm/smmu.c
index ad956d5b8d..4c04391e21 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -41,6 +41,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -51,36 +52,13 @@
  #include 
  #include 
  
+#include "arm_smmu.h" /* Not a self contained header. So last in the list */

  /* Xen: The below defines are redefined within the file. Undef it */
  #undef SCTLR_AFE
  #undef SCTLR_TRE
  #undef SCTLR_M
  #undef TTBCR_EAE
  
-/* Alias to Xen device tree helpers */

-#define device_node dt_device_node
-#define of_phandle_args dt_phandle_args
-#define of_device_id dt_device_match
-#define of_match_node dt_match_node
-#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, pname, 
out))
-#define of_property_read_bool dt_property_read_bool
-#define of_parse_phandle_with_args dt_parse_phandle_with_args
-
-/* 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
-
  static struct resource *platform_get_resource(struct platform_device *pdev,
  unsigned int type,
  unsigned int num)
@@ -118,58 +96,6 @@ static struct resource *platform_get_resource(struct 
platform_device *pdev,
  
  /* Xen: Helpers for IRQ functions */

  #define request_irq(irq, func, flags, name, dev) request_irq(irq, flags, 
func, name, dev)
-#define free_irq release_irq
-
-enum irqreturn {
-   IRQ_NONE= (0 << 0),
-   IRQ_HANDLED = (1 << 0),
-};
-
-typedef enum irqreturn irqreturn_t;


You remove the irqreturn enum without adding any replacement, is this
really unused?


It is used, so looks like the SMMU driver has not been build test it. 
Sameer, please at least build test the changes you made in the SMMU driver.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 6/7] drivers/passthrough/arm: Refactor code for arm smmu drivers

2018-02-09 Thread Roger Pau Monné 
On Thu, Feb 08, 2018 at 08:10:54PM -0700, Sameer Goel wrote:
> Pull common defines for SMMU drives in a local header.
> 
> Signed-off-by: Sameer Goel 
> ---
>  xen/drivers/passthrough/arm/arm_smmu.h | 125 
> +
>  xen/drivers/passthrough/arm/smmu-v3.c  |  96 +
>  xen/drivers/passthrough/arm/smmu.c | 104 +--
>  3 files changed, 128 insertions(+), 197 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..f49dceb5b4
> --- /dev/null
> +++ b/xen/drivers/passthrough/arm/arm_smmu.h
> @@ -0,0 +1,125 @@
> +/**
> + * ./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__
> +
> +
> +/* Alias to Xen device tree helpers */
> +#define device_node dt_device_node
> +#define of_phandle_args dt_phandle_args
> +#define of_device_id dt_device_match
> +#define of_match_node dt_match_node
> +#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, 
> pname, out))

Break the line.

> +#define of_property_read_bool dt_property_read_bool
> +#define of_parse_phandle_with_args dt_parse_phandle_with_args
> +
> +/* Helpers to get device MMIO and IRQs */
> +struct resource {
> +u64 addr;
> +u64 size;

uint64_t is preferred rather than u64.

> +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)
> +
> +#define dma_set_mask_and_coherent(d, b)  0
> +#define of_dma_is_coherent(n)0
> +
> +static void __iomem *devm_ioremap_resource(struct device *dev,
> +struct resource *res)

Aligment, please use spaces.

Also, is __iomem needed here at all?

> +{
> +void __iomem *ptr;

Same question about __iomem attribute.

> +
> +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 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;

No tabs please. And if you want to align the fields, please do so
uniformly for all the structs in the file.

> +};
> +
> +/* Xen: Describes information required for a Xen domain */
> +struct arm_smmu_xen_domain {
> +