Re: [Xen-devel] [RFC v2 7/7] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

2017-11-21 Thread Goel, Sameer 


On 11/20/2017 7:25 AM, Julien Grall wrote:
> Hi Sameer,
> 
> On 19/11/17 07:45, Goel, Sameer wrote:
>> On 10/12/2017 10:36 AM, Julien Grall wrote:
 +
 +typedef paddr_t phys_addr_t;
 +typedef paddr_t dma_addr_t;
 +
 +/* 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
 +#define mutex spinlock_t
 +#define mutex_init spin_lock_init
 +#define mutex_lock spin_lock
 +#define mutex_unlock spin_unlock
>>>
>>> mutex and spinlock are not the same. The former is sleeping whilst the 
>>> later is not.
>>>
>>> Can you please explain why this is fine and possibly add that in a comment?
>>>
>> Mutex is used to protect the access to smmu device internal data structure 
>> when setting up the s2 config and installing stes for a given device in 
>> Linux. The ste programming  operation can be competitively long but in the 
>> current testing, I did not see this blocking for too long. I will put in a 
>> comment.
> 
> Well, I don't think that this is a justification. You tested on one platform 
> and does not explain how you perform them.
> 
> If I understand correctly, that mutex is only used when assigning device. So 
> it might be ok to switch to spinlock. But that's not because the operation is 
> not too long, it just because it would be only perform by the toolstack 
> (domctl) and will not be issued by guest.
Ok. I agree ans will update the comment. 
> 
>>
 +
 +/* Xen: Helpers to get device MMIO and IRQs */
 +struct resource {
 +    u64 addr;
 +    u64 size;
 +    unsigned int type;
 +};
>>>
>>> Likely we want a compat header for defining Linux helpers. This would avoid 
>>> replicating it everywhere.
>> Agreed.
>>
> That should be
>>>
 +
 +#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)
 +{
 +    /*
 + * The resource is only used between 2 calls of platform_get_resource.
 + * It's quite ugly but it's avoid to add too much code in the part
 + * imported from Linux
 + */
 +    static struct resource res;
 +    struct acpi_iort_node *iort_node;
 +    struct acpi_iort_smmu_v3 *node_smmu_data;
 +    int ret = 0;
 +
 +    res.type = type;
 +
 +    switch (type) {
 +    case IORESOURCE_MEM:
 +    if (pdev->type == DEV_ACPI) {
 +    ret = 1;
 +    iort_node = pdev->acpi_node;
 +    node_smmu_data =
 +    (struct acpi_iort_smmu_v3 *)iort_node->node_data;
 +
 +    if (node_smmu_data != NULL) {
 +    res.addr = node_smmu_data->base_address;
 +    res.size = SZ_128K;
 +    ret = 0;
 +    }
 +    } else {
 +    ret = dt_device_get_address(dev_to_dt(pdev), num,
 +    , );
 +    }
 +
 +    return ((ret) ? NULL : );
 +
 +    case IORESOURCE_IRQ:
 +    ret = platform_get_irq(dev_to_dt(pdev), num);
>>>
>>> No IRQ for ACPI?
>> For IRQs the code calls platform_get_irq_byname. So, the IORESOURCE_IRQ 
>> implementation is not needed at all. (DT or ACPI)
> 
> Please document it then.
Ok.
> 
> [...]
> 
>>>
 +    udelay(sleep_us); \
 +    } \
 +    (cond) ? 0 : -ETIMEDOUT; \
 +})
 +
 +#define readl_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) 
 \
 +    readx_poll_timeout(readl_relaxed, addr, val, cond, delay_us, 
 timeout_us)
 +
 +/* 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;
 +
 +/* Device logger functions */
 +#define dev_print(dev, lvl, fmt, ...)    \
 + printk(lvl "smmu: " fmt, ## __VA_ARGS__)
 +
 +#define dev_dbg(dev, fmt, ...) dev_print(dev, XENLOG_DEBUG, fmt, ## 
 __VA_ARGS__)
 +#define dev_notice(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## 
 __VA_ARGS__)
 +#define dev_warn(dev, fmt, ...) dev_print(dev, XENLOG_WARNING, fmt, ## 
 __VA_ARGS__)
 +#define dev_err(dev, fmt, ...)

Re: [Xen-devel] [RFC v2 7/7] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

2017-11-20 Thread Julien Grall 

Hi,

On 20/11/17 15:19, Robin Murphy wrote:

On 20/11/17 14:25, Julien Grall wrote:
[...]



+    else {
   cpu_relax();


Hmmm I now see why you added cpu_relax() at the top. Well, on Xen 
cpu_relax is just a barrier. On Linux it is used to yield.


And that bit is worrying me. The Linux code will allow context 
switching to another tasks if the code is taking too much time.


Xen is not preemptible, so is it fine?
This is used when consuming the command queue and could be a 
potential performance issue if the queue is large. (This is never the 
case).

I am wondering if we should define a yeild in long run?


As I said before, Xen is not preemptible. In this particular case, 
there are spinlock taken by the callers (e.g any function assigning 
device). So yield would just make it worst.


The arguments here don't make much sense - the "yield" instruction has 
nothing to do with software-level concepts of preemption. It is a hint 
to SMT *hardware* that this logical processor is doing nothing useful in 
the short term, so it might be a good idea to let other logical 
processor(s) have priority over shared execution resources if applicable.


Oh, sorry I thought this could also be used by the software to switch 
between thread. Please disregard my comment then.




Until SMT CPUs become commonly available, though, it's somewhat of a 
moot point and mostly just a future-proofing consideration.


Robin.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC v2 7/7] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

2017-11-20 Thread Robin Murphy 

On 20/11/17 14:25, Julien Grall wrote:
[...]



+    else {
   cpu_relax();


Hmmm I now see why you added cpu_relax() at the top. Well, on Xen 
cpu_relax is just a barrier. On Linux it is used to yield.


And that bit is worrying me. The Linux code will allow context 
switching to another tasks if the code is taking too much time.


Xen is not preemptible, so is it fine?
This is used when consuming the command queue and could be a potential 
performance issue if the queue is large. (This is never the case).

I am wondering if we should define a yeild in long run?


As I said before, Xen is not preemptible. In this particular case, there 
are spinlock taken by the callers (e.g any function assigning device). 
So yield would just make it worst.


The arguments here don't make much sense - the "yield" instruction has 
nothing to do with software-level concepts of preemption. It is a hint 
to SMT *hardware* that this logical processor is doing nothing useful in 
the short term, so it might be a good idea to let other logical 
processor(s) have priority over shared execution resources if applicable.


Until SMT CPUs become commonly available, though, it's somewhat of a 
moot point and mostly just a future-proofing consideration.


Robin.

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

Re: [Xen-devel] [RFC v2 7/7] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

2017-11-20 Thread Julien Grall 

Hi Sameer,

On 19/11/17 07:45, Goel, Sameer wrote:

On 10/12/2017 10:36 AM, Julien Grall wrote:

+
+typedef paddr_t phys_addr_t;
+typedef paddr_t dma_addr_t;
+
+/* 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
+#define mutex spinlock_t
+#define mutex_init spin_lock_init
+#define mutex_lock spin_lock
+#define mutex_unlock spin_unlock


mutex and spinlock are not the same. The former is sleeping whilst the later is 
not.

Can you please explain why this is fine and possibly add that in a comment?


Mutex is used to protect the access to smmu device internal data structure when 
setting up the s2 config and installing stes for a given device in Linux. The 
ste programming  operation can be competitively long but in the current 
testing, I did not see this blocking for too long. I will put in a comment.


Well, I don't think that this is a justification. You tested on one 
platform and does not explain how you perform them.


If I understand correctly, that mutex is only used when assigning 
device. So it might be ok to switch to spinlock. But that's not because 
the operation is not too long, it just because it would be only perform 
by the toolstack (domctl) and will not be issued by guest.





+
+/* Xen: Helpers to get device MMIO and IRQs */
+struct resource {
+    u64 addr;
+    u64 size;
+    unsigned int type;
+};


Likely we want a compat header for defining Linux helpers. This would avoid 
replicating it everywhere.

Agreed.


That should be



+
+#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)
+{
+    /*
+ * The resource is only used between 2 calls of platform_get_resource.
+ * It's quite ugly but it's avoid to add too much code in the part
+ * imported from Linux
+ */
+    static struct resource res;
+    struct acpi_iort_node *iort_node;
+    struct acpi_iort_smmu_v3 *node_smmu_data;
+    int ret = 0;
+
+    res.type = type;
+
+    switch (type) {
+    case IORESOURCE_MEM:
+    if (pdev->type == DEV_ACPI) {
+    ret = 1;
+    iort_node = pdev->acpi_node;
+    node_smmu_data =
+    (struct acpi_iort_smmu_v3 *)iort_node->node_data;
+
+    if (node_smmu_data != NULL) {
+    res.addr = node_smmu_data->base_address;
+    res.size = SZ_128K;
+    ret = 0;
+    }
+    } else {
+    ret = dt_device_get_address(dev_to_dt(pdev), num,
+    , );
+    }
+
+    return ((ret) ? NULL : );
+
+    case IORESOURCE_IRQ:
+    ret = platform_get_irq(dev_to_dt(pdev), num);


No IRQ for ACPI?

For IRQs the code calls platform_get_irq_byname. So, the IORESOURCE_IRQ 
implementation is not needed at all. (DT or ACPI)


Please document it then.

[...]




+    udelay(sleep_us); \
+    } \
+    (cond) ? 0 : -ETIMEDOUT; \
+})
+
+#define readl_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+    readx_poll_timeout(readl_relaxed, addr, val, cond, delay_us, timeout_us)
+
+/* 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;
+
+/* Device logger functions */
+#define dev_print(dev, lvl, fmt, ...)    \
+ printk(lvl "smmu: " fmt, ## __VA_ARGS__)
+
+#define dev_dbg(dev, fmt, ...) dev_print(dev, XENLOG_DEBUG, fmt, ## 
__VA_ARGS__)
+#define dev_notice(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## 
__VA_ARGS__)
+#define dev_warn(dev, fmt, ...) dev_print(dev, XENLOG_WARNING, fmt, ## 
__VA_ARGS__)
+#define dev_err(dev, fmt, ...) dev_print(dev, XENLOG_ERR, fmt, ## __VA_ARGS__)
+#define dev_info(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## 
__VA_ARGS__)
+
+#define dev_err_ratelimited(dev, fmt, ...)    \
+ dev_print(dev, XENLOG_ERR, fmt, ## __VA_ARGS__)
+
+#define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
+
+/* Alias to Xen allocation helpers */
+#define kfree xfree
+#define kmalloc(size, flags)    _xmalloc(size, sizeof(void *))
+#define kzalloc(size, flags)    _xzalloc(size, sizeof(void *))
+#define devm_kzalloc(dev, size, flags)    _xzalloc(size, sizeof(void *))
+#define kmalloc_array(size, n, flags)    _xmalloc_array(size, sizeof(void *), 
n)
+
+/* Compatibility defines */
+#undef WARN_ON

Re: [Xen-devel] [RFC v2 7/7] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

2017-11-18 Thread Goel, Sameer 


On 10/12/2017 10:36 AM, Julien Grall wrote:
> Hi Sameer,
> 
> Given this is all Arm specific. I am not sure why people like Andrew, Jan 
> have been added.
> 
> Please use scripts/get_maintainers to find the list of maintainers per 
> patches and avoid to CC all of them on each patches.
Ok.

> 
> On 21/09/17 01:37, Sameer Goel wrote:
>> This driver follows an approach similar to smmu driver. The intent here
>> is to reuse as much Linux code as possible.
>> - Glue code has been introduced to bridge the API calls.
>> - Called Linux functions from the Xen IOMMU function calls.
>> - Xen modifications are preceded by /*Xen: comment */
>>
>> Signed-off-by: Sameer Goel 
>> ---
>>   xen/drivers/passthrough/arm/Makefile  |   1 +
>>   xen/drivers/passthrough/arm/smmu-v3.c | 853 
>> +-
> 
> This is based on an old SMMUv3 version and I have been told there are some 
> changes may benefits Xen (such as increasing the timeout for sync) and some 
> optimisations also exist on the ML and will be queued soon.
> 
> So maybe you want to re-sync at least to master.
> 
>>   2 files changed, 738 insertions(+), 116 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/arm/Makefile 
>> b/xen/drivers/passthrough/arm/Makefile
>> index f4cd26e..57a6da6 100644
>> --- a/xen/drivers/passthrough/arm/Makefile
>> +++ b/xen/drivers/passthrough/arm/Makefile
>> @@ -1,2 +1,3 @@
>>   obj-y += iommu.o
>>   obj-y += smmu.o
>> +obj-y += smmu-v3.o
> 
> Do we want SMMUv3 to be built on Arm32? Maybe we should introduce a new 
> Kconfig to let the user select.
Agreed. I will define a CONFIG_ARM_SMMU_V3.

> 
>> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
>> b/xen/drivers/passthrough/arm/smmu-v3.c
>> index 380969a..8f3b43d 100644
>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>> @@ -18,28 +18,266 @@
>>    * Author: Will Deacon 
>>    *
>>    * This driver is powered by bad coffee and bombay mix.
>> + *
>> + *
>> + * Based on Linux drivers/iommu/arm-smmu-v3.c
>> + * => commit bdf95923086fb359ccb44c815724c3ace1611c90
>> + *
>> + * Xen modifications:
>> + * Sameer Goel 
>> + * Copyright (C) 2017, The Linux Foundation, All rights reserved.
>> + *
>>    */
>>   -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -
>> -#include 
>> -
>> -#include "io-pgtable.h"
>> +#include 
> 
> This is not necessary.
> 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> Please order the includes alphabetically with xen/* first then asm/*

Done.
> 
>> +
>> +typedef paddr_t phys_addr_t;
>> +typedef paddr_t dma_addr_t;
>> +
>> +/* 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
>> +#define mutex spinlock_t
>> +#define mutex_init spin_lock_init
>> +#define mutex_lock spin_lock
>> +#define mutex_unlock spin_unlock
> 
> mutex and spinlock are not the same. The former is sleeping whilst the later 
> is not.
> 
> Can you please explain why this is fine and possibly add that in a comment?
> 
Mutex is used to protect the access to smmu device internal data structure when 
setting up the s2 config and installing stes for a given device in Linux. The 
ste programming  operation can be competitively long but in the current 
testing, I did not see this blocking for too long. I will put in a comment. 

>> +
>> +/* Xen: Helpers to get device MMIO and IRQs */
>> +struct resource {
>> +    u64 addr;
>> +    u64 size;
>> +    unsigned int type;
>> +};
> 
> Likely we want a compat header for defining Linux helpers. This would avoid 
> replicating it everywhere.
Agreed.

> 
>> +
>> +#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)
>> +{
>> +    /*
>> + * The resource is only used between 2 calls of platform_get_resource.
>> + * It's quite ugly but it's avoid to add too much code in the part
>> + * imported from Linux
>> + */
>> +    static struct resource res;
>> +    struct acpi_iort_node *iort_node;
>> +    struct acpi_iort_smmu_v3

Re: [Xen-devel] [RFC v2 7/7] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

2017-10-12 Thread Julien Grall 

Hi Sameer,

Given this is all Arm specific. I am not sure why people like Andrew, 
Jan have been added.


Please use scripts/get_maintainers to find the list of maintainers per 
patches and avoid to CC all of them on each patches.


On 21/09/17 01:37, Sameer Goel wrote:

This driver follows an approach similar to smmu driver. The intent here
is to reuse as much Linux code as possible.
- Glue code has been introduced to bridge the API calls.
- Called Linux functions from the Xen IOMMU function calls.
- Xen modifications are preceded by /*Xen: comment */

Signed-off-by: Sameer Goel 
---
  xen/drivers/passthrough/arm/Makefile  |   1 +
  xen/drivers/passthrough/arm/smmu-v3.c | 853 +-


This is based on an old SMMUv3 version and I have been told there are 
some changes may benefits Xen (such as increasing the timeout for sync) 
and some optimisations also exist on the ML and will be queued soon.


So maybe you want to re-sync at least to master.


  2 files changed, 738 insertions(+), 116 deletions(-)

diff --git a/xen/drivers/passthrough/arm/Makefile 
b/xen/drivers/passthrough/arm/Makefile
index f4cd26e..57a6da6 100644
--- a/xen/drivers/passthrough/arm/Makefile
+++ b/xen/drivers/passthrough/arm/Makefile
@@ -1,2 +1,3 @@
  obj-y += iommu.o
  obj-y += smmu.o
+obj-y += smmu-v3.o


Do we want SMMUv3 to be built on Arm32? Maybe we should introduce a new 
Kconfig to let the user select.



diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
b/xen/drivers/passthrough/arm/smmu-v3.c
index 380969a..8f3b43d 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -18,28 +18,266 @@
   * Author: Will Deacon 
   *
   * This driver is powered by bad coffee and bombay mix.
+ *
+ *
+ * Based on Linux drivers/iommu/arm-smmu-v3.c
+ * => commit bdf95923086fb359ccb44c815724c3ace1611c90
+ *
+ * Xen modifications:
+ * Sameer Goel 
+ * Copyright (C) 2017, The Linux Foundation, All rights reserved.
+ *
   */
  
-#include 

-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#include 
-
-#include "io-pgtable.h"
+#include 


This is not necessary.


+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


Please order the includes alphabetically with xen/* first then asm/*


+
+typedef paddr_t phys_addr_t;
+typedef paddr_t dma_addr_t;
+
+/* 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
+#define mutex spinlock_t
+#define mutex_init spin_lock_init
+#define mutex_lock spin_lock
+#define mutex_unlock spin_unlock


mutex and spinlock are not the same. The former is sleeping whilst the 
later is not.


Can you please explain why this is fine and possibly add that in a comment?


+
+/* Xen: Helpers to get device MMIO and IRQs */
+struct resource {
+   u64 addr;
+   u64 size;
+   unsigned int type;
+};


Likely we want a compat header for defining Linux helpers. This would 
avoid replicating it everywhere.



+
+#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)
+{
+   /*
+* The resource is only used between 2 calls of platform_get_resource.
+* It's quite ugly but it's avoid to add too much code in the part
+* imported from Linux
+*/
+   static struct resource res;
+   struct acpi_iort_node *iort_node;
+   struct acpi_iort_smmu_v3 *node_smmu_data;
+   int ret = 0;
+
+   res.type = type;
+
+   switch (type) {
+   case IORESOURCE_MEM:
+   if (pdev->type == DEV_ACPI) {
+   ret = 1;
+   iort_node = pdev->acpi_node;
+   node_smmu_data =
+   (struct acpi_iort_smmu_v3 
*)iort_node->node_data;
+
+   if (node_smmu_data != NULL) {
+   res.addr = node_smmu_data->base_address;
+   res.size = SZ_128K;
+   ret = 0;
+   }
+   } else {
+   ret = dt_device_get_address(dev_to_dt(pdev), num,
+   , );
+

Re: [Xen-devel] [RFC v2 7/7] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

2017-09-25 Thread Goel, Sameer 


On 9/20/2017 6:37 PM, Sameer Goel wrote:
> This driver follows an approach similar to smmu driver. The intent here
> is to reuse as much Linux code as possible.
> - Glue code has been introduced to bridge the API calls.
> - Called Linux functions from the Xen IOMMU function calls.
> - Xen modifications are preceded by /*Xen: comment */
> 
> Signed-off-by: Sameer Goel 
> ---
>  xen/drivers/passthrough/arm/Makefile  |   1 +
>  xen/drivers/passthrough/arm/smmu-v3.c | 853 
> +-
>  2 files changed, 738 insertions(+), 116 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/Makefile 
> b/xen/drivers/passthrough/arm/Makefile
> index f4cd26e..57a6da6 100644
> --- a/xen/drivers/passthrough/arm/Makefile
> +++ b/xen/drivers/passthrough/arm/Makefile
> @@ -1,2 +1,3 @@
>  obj-y += iommu.o
>  obj-y += smmu.o
> +obj-y += smmu-v3.o
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index 380969a..8f3b43d 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -18,28 +18,266 @@
>   * Author: Will Deacon 
>   *
>   * This driver is powered by bad coffee and bombay mix.
> + *
> + *
> + * Based on Linux drivers/iommu/arm-smmu-v3.c
> + * => commit bdf95923086fb359ccb44c815724c3ace1611c90
> + *
> + * Xen modifications:
> + * Sameer Goel 
> + * Copyright (C) 2017, The Linux Foundation, All rights reserved.
> + *
>   */
>  
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -
> -#include 
> -
> -#include "io-pgtable.h"
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +typedef paddr_t phys_addr_t;
> +typedef paddr_t dma_addr_t;
> +
> +/* 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
> +#define mutex spinlock_t
> +#define mutex_init spin_lock_init
> +#define mutex_lock spin_lock
> +#define mutex_unlock spin_unlock
> +
> +/* 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)
> +{
> + /*
> +  * The resource is only used between 2 calls of platform_get_resource.
> +  * It's quite ugly but it's avoid to add too much code in the part
> +  * imported from Linux
> +  */
> + static struct resource res;
> + struct acpi_iort_node *iort_node;
> + struct acpi_iort_smmu_v3 *node_smmu_data;
> + int ret = 0;
> +
> + res.type = type;
> +
> + switch (type) {
> + case IORESOURCE_MEM:
> + if (pdev->type == DEV_ACPI) {
> + ret = 1;
> + iort_node = pdev->acpi_node;
> + node_smmu_data =
> + (struct acpi_iort_smmu_v3 
> *)iort_node->node_data;
> +
> + if (node_smmu_data != NULL) {
> + res.addr = node_smmu_data->base_address;
> + res.size = SZ_128K;
> + ret = 0;
> + }
> + } else {
> + ret = dt_device_get_address(dev_to_dt(pdev), num,
> + , );
> + }
> +
> + return ((ret) ? NULL : );
> +
> + case IORESOURCE_IRQ:
> + ret = platform_get_irq(dev_to_dt(pdev), num);
> +
> + if (ret < 0)
> + return NULL;
> +
> + res.addr = ret;
> + res.size = 1;
> +
> + return 
> +
> + default:
> + return NULL;
> + }
> +}
> +
> +static int platform_get_irq_byname(struct platform_device *pdev, const char 
> *name)
> +{
> + const struct dt_property *dtprop;
> + struct acpi_iort_node *iort_node;
> + struct acpi_iort_smmu_v3 *node_smmu_data;
> + int ret = 0;
> +
> + if (pdev->type == DEV_ACPI) {
> + iort_node = pdev->acpi_node;
> + node_smmu_data = (struct

[Xen-devel] [RFC v2 7/7] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

2017-09-20 Thread Sameer Goel 
This driver follows an approach similar to smmu driver. The intent here
is to reuse as much Linux code as possible.
- Glue code has been introduced to bridge the API calls.
- Called Linux functions from the Xen IOMMU function calls.
- Xen modifications are preceded by /*Xen: comment */

Signed-off-by: Sameer Goel 
---
 xen/drivers/passthrough/arm/Makefile  |   1 +
 xen/drivers/passthrough/arm/smmu-v3.c | 853 +-
 2 files changed, 738 insertions(+), 116 deletions(-)

diff --git a/xen/drivers/passthrough/arm/Makefile 
b/xen/drivers/passthrough/arm/Makefile
index f4cd26e..57a6da6 100644
--- a/xen/drivers/passthrough/arm/Makefile
+++ b/xen/drivers/passthrough/arm/Makefile
@@ -1,2 +1,3 @@
 obj-y += iommu.o
 obj-y += smmu.o
+obj-y += smmu-v3.o
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
b/xen/drivers/passthrough/arm/smmu-v3.c
index 380969a..8f3b43d 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -18,28 +18,266 @@
  * Author: Will Deacon 
  *
  * This driver is powered by bad coffee and bombay mix.
+ *
+ *
+ * Based on Linux drivers/iommu/arm-smmu-v3.c
+ * => commit bdf95923086fb359ccb44c815724c3ace1611c90
+ *
+ * Xen modifications:
+ * Sameer Goel 
+ * Copyright (C) 2017, The Linux Foundation, All rights reserved.
+ *
  */
 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#include 
-
-#include "io-pgtable.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+typedef paddr_t phys_addr_t;
+typedef paddr_t dma_addr_t;
+
+/* 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
+#define mutex spinlock_t
+#define mutex_init spin_lock_init
+#define mutex_lock spin_lock
+#define mutex_unlock spin_unlock
+
+/* 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)
+{
+   /*
+* The resource is only used between 2 calls of platform_get_resource.
+* It's quite ugly but it's avoid to add too much code in the part
+* imported from Linux
+*/
+   static struct resource res;
+   struct acpi_iort_node *iort_node;
+   struct acpi_iort_smmu_v3 *node_smmu_data;
+   int ret = 0;
+
+   res.type = type;
+
+   switch (type) {
+   case IORESOURCE_MEM:
+   if (pdev->type == DEV_ACPI) {
+   ret = 1;
+   iort_node = pdev->acpi_node;
+   node_smmu_data =
+   (struct acpi_iort_smmu_v3 
*)iort_node->node_data;
+
+   if (node_smmu_data != NULL) {
+   res.addr = node_smmu_data->base_address;
+   res.size = SZ_128K;
+   ret = 0;
+   }
+   } else {
+   ret = dt_device_get_address(dev_to_dt(pdev), num,
+   , );
+   }
+
+   return ((ret) ? NULL : );
+
+   case IORESOURCE_IRQ:
+   ret = platform_get_irq(dev_to_dt(pdev), num);
+
+   if (ret < 0)
+   return NULL;
+
+   res.addr = ret;
+   res.size = 1;
+
+   return 
+
+   default:
+   return NULL;
+   }
+}
+
+static int platform_get_irq_byname(struct platform_device *pdev, const char 
*name)
+{
+   const struct dt_property *dtprop;
+   struct acpi_iort_node *iort_node;
+   struct acpi_iort_smmu_v3 *node_smmu_data;
+   int ret = 0;
+
+   if (pdev->type == DEV_ACPI) {
+   iort_node = pdev->acpi_node;
+   node_smmu_data = (struct acpi_iort_smmu_v3 
*)iort_node->node_data;
+
+   if (node_smmu_data != NULL) {
+   if (!strcmp(name, "eventq"))
+   ret = node_smmu_data->event_gsiv;
+   else if (!strcmp(name, "priq"))
+