[PATCH] xen/arm: Implement dynamic allocation of irq descriptors

[PATCH] xen/arm: Implement dynamic allocation of irq descriptors

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

For arm memory for 1024 irq descriptors are allocated
statically irrespective of number of interrupt supported
by the platform.

With this patch, irq descriptors are allocated at run time
and managed using red-black tree. Functions to insert, search
and delete irq descriptor are implemented in xen/common/irq.c.

Signed-off-by: Vijaya Kumar K<Vijaya.Kumar@caviumnetworks.com>
---
 xen/arch/arm/irq.c    |   83 +++++++++++++++++++++++++++++++++++++++----------
 xen/common/irq.c      |   58 ++++++++++++++++++++++++++++++++++
 xen/include/xen/irq.h |    6 ++++
 3 files changed, 130 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index cb9c99b..856aecf 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -30,6 +30,7 @@
 
 static unsigned int local_irqs_type[NR_LOCAL_IRQS];
 static DEFINE_SPINLOCK(local_irqs_type_lock);
+static DEFINE_SPINLOCK(rb_tree_desc_lock);
 
 static void ack_none(struct irq_desc *irq)
 {
@@ -48,33 +49,35 @@ hw_irq_controller no_irq_type = {
     .end = end_none
 };
 
-static irq_desc_t irq_desc[NR_IRQS];
+static struct rb_root desc_root;
+
 static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
 
 irq_desc_t *__irq_to_desc(int irq)
 {
+    struct irq_desc *desc;
+
     if (irq < NR_LOCAL_IRQS) return &this_cpu(local_irq_desc)[irq];
-    return &irq_desc[irq-NR_LOCAL_IRQS];
+
+    spin_lock(&rb_tree_desc_lock);
+    desc = find_irq_desc(&desc_root, irq);
+    spin_unlock(&rb_tree_desc_lock);
+
+    return desc;
 }
 
-int __init arch_init_one_irq_desc(struct irq_desc *desc)
+int arch_init_one_irq_desc(struct irq_desc *desc)
 {
     desc->arch.type = DT_IRQ_TYPE_INVALID;
     return 0;
 }
 
-
-static int __init init_irq_data(void)
+static int init_irq_data(int irq, struct irq_desc *desc)
 {
-    int irq;
-
-    for (irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++) {
-        struct irq_desc *desc = irq_to_desc(irq);
-        init_one_irq_desc(desc);
-        desc->irq = irq;
-        desc->action  = NULL;
-    }
-
+   init_one_irq_desc(desc);
+   desc->irq = irq;
+   desc->action  = NULL;
+ 
     return 0;
 }
 
@@ -114,7 +117,8 @@ void __init init_IRQ(void)
     spin_unlock(&local_irqs_type_lock);
 
     BUG_ON(init_local_irq_data() < 0);
-    BUG_ON(init_irq_data() < 0);
+
+    desc_root = RB_ROOT;
 }
 
 void __cpuinit init_secondary_IRQ(void)
@@ -265,6 +269,7 @@ void release_irq(unsigned int irq, const void *dev_id)
 
     desc = irq_to_desc(irq);
 
+    ASSERT(desc != NULL);
     spin_lock_irqsave(&desc->lock,flags);
 
     action_ptr = &desc->action;
@@ -301,6 +306,11 @@ void release_irq(unsigned int irq, const void *dev_id)
 
     if ( action->free_on_release )
         xfree(action);
+
+    spin_lock(&rb_tree_desc_lock);
+    delete_irq_desc(&desc_root, irq);
+    xfree(desc);
+    spin_unlock(&rb_tree_desc_lock);
 }
 
 static int __setup_irq(struct irq_desc *desc, unsigned int irqflags,
@@ -339,6 +349,8 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
 
     desc = irq_to_desc(irq);
 
+    ASSERT(desc != NULL);
+
     spin_lock_irqsave(&desc->lock, flags);
 
     if ( test_bit(_IRQ_GUEST, &desc->status) )
@@ -382,7 +394,7 @@ int route_irq_to_guest(struct domain *d, unsigned int irq,
                        const char * devname)
 {
     struct irqaction *action;
-    struct irq_desc *desc = irq_to_desc(irq);
+    struct irq_desc *desc = NULL;
     unsigned long flags;
     int retval = 0;
 
@@ -394,6 +406,23 @@ int route_irq_to_guest(struct domain *d, unsigned int irq,
     action->name = devname;
     action->free_on_release = 1;
 
+    spin_lock(&rb_tree_desc_lock);
+    desc = find_irq_desc(&desc_root, irq);
+
+    if ( desc == NULL )
+    {
+        /* Allocate descriptor and add to rb tree */
+        desc = xzalloc(struct irq_desc);
+        if ( desc == NULL )
+        {
+            spin_unlock(&rb_tree_desc_lock);
+            return -ENOMEM;
+        }
+        init_irq_data(irq, desc);
+        insert_irq_desc(&desc_root, irq, desc);
+    }
+    spin_unlock(&rb_tree_desc_lock);
+
     spin_lock_irqsave(&desc->lock, flags);
 
     /* If the IRQ is already used by someone
@@ -470,13 +499,16 @@ static bool_t irq_validate_new_type(unsigned int curr, unsigned new)
 int irq_set_spi_type(unsigned int spi, unsigned int type)
 {
     unsigned long flags;
-    struct irq_desc *desc = irq_to_desc(spi);
+    struct irq_desc *desc = NULL;
     int ret = -EBUSY;
 
     /* This function should not be used for other than SPIs */
     if ( spi < NR_LOCAL_IRQS )
         return -EINVAL;
 
+    desc = irq_to_desc(spi);
+    ASSERT(desc != NULL);
+
     spin_lock_irqsave(&desc->lock, flags);
 
     if ( !irq_validate_new_type(desc->arch.type, type) )
@@ -532,6 +564,7 @@ int platform_get_irq(const struct dt_device_node *device, int index)
 {
     struct dt_irq dt_irq;
     unsigned int type, irq;
+    struct irq_desc *desc;
     int res;
 
     res = dt_device_get_irq(device, index, &dt_irq);
@@ -541,6 +574,22 @@ int platform_get_irq(const struct dt_device_node *device, int index)
     irq = dt_irq.irq;
     type = dt_irq.type;
 
+    spin_lock(&rb_tree_desc_lock);
+    desc = find_irq_desc(&desc_root, irq);
+    if ( desc == NULL )
+    {
+        /* Allocate descriptor for this irq */
+        desc = xzalloc(struct irq_desc);
+        if ( desc == NULL )
+        {
+            spin_unlock(&rb_tree_desc_lock);
+            return -ENOMEM;
+        }
+        init_irq_data(irq, desc);
+        insert_irq_desc(&desc_root, irq, desc);
+    }
+    spin_unlock(&rb_tree_desc_lock);
+
     /* Setup the IRQ type */
     if ( irq < NR_LOCAL_IRQS )
         res = irq_local_set_type(irq, type);
diff --git a/xen/common/irq.c b/xen/common/irq.c
index 3e55dfa..ddf85c5 100644
--- a/xen/common/irq.c
+++ b/xen/common/irq.c
@@ -40,3 +40,61 @@ unsigned int irq_startup_none(struct irq_desc *desc)
 {
     return 0;
 }
+
+irq_desc_t * find_irq_desc( struct rb_root *root_node, int irq)
+{
+    struct rb_node *node = root_node->rb_node;
+
+    while ( node )
+    {
+        irq_desc_t *this;
+
+        this = container_of(node, irq_desc_t, node);
+ 
+        if ( irq < this->irq )
+            node = node->rb_left;
+        else if (irq > this->irq)
+            node = node->rb_right;
+        else
+            return this;
+    }
+ 
+    return NULL;
+}
+
+int insert_irq_desc(struct rb_root *root_node, int irq, struct irq_desc *desc)
+{
+    struct rb_node **new, *parent;
+
+    new = &root_node->rb_node;
+    parent = NULL;
+
+    while ( *new )
+    {
+        irq_desc_t *this;
+
+        this = container_of(*new, irq_desc_t, node);
+ 
+        parent = *new;
+        if ( desc->irq < this->irq )
+            new = &((*new)->rb_left);
+        else if (desc->irq > this->irq)
+            new = &((*new)->rb_right);
+        else
+            return -EEXIST;
+    }
+ 
+    rb_link_node(&desc->node, parent, new);
+    rb_insert_color(&desc->node, root_node);
+
+    return 0;
+}
+
+void delete_irq_desc(struct rb_root *root_node, int irq)
+{
+   struct irq_desc *desc;
+
+   desc = find_irq_desc(root_node, irq);
+   if ( desc )
+      rb_erase(&desc->node, root_node);
+}
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 9e0155c..d623ce3 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -6,6 +6,7 @@
 #include <xen/spinlock.h>
 #include <xen/time.h>
 #include <xen/list.h>
+#include <xen/rbtree.h>
 #include <asm/regs.h>
 #include <asm/hardirq.h>
 
@@ -83,6 +84,7 @@ struct msi_desc;
  * first field.
  */
 typedef struct irq_desc {
+    struct rb_node node;
     unsigned int status;        /* IRQ status */
     hw_irq_controller *handler;
     struct msi_desc   *msi_desc;
@@ -168,6 +170,10 @@ static inline void set_native_irq_info(unsigned int irq, const cpumask_t *mask)
 
 unsigned int set_desc_affinity(struct irq_desc *, const cpumask_t *);
 
+irq_desc_t * find_irq_desc(struct rb_root *root_node, int irq);
+int insert_irq_desc(struct rb_root *root_node, int irq, struct irq_desc *desc);
+void delete_irq_desc(struct rb_root *root_node, int irq);
+
 #ifndef arch_hwdom_irqs
 unsigned int arch_hwdom_irqs(domid_t);
 #endif
-- 
1.7.9.5

Re: [PATCH] xen/arm: Implement dynamic allocation of irq descriptors

[Julien Grall]

On 19/02/15 07:17, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> For arm memory for 1024 irq descriptors are allocated
> statically irrespective of number of interrupt supported
> by the platform.
> 
> With this patch, irq descriptors are allocated at run time
> and managed using red-black tree. Functions to insert, search
> and delete irq descriptor are implemented in xen/common/irq.c.

I think we may want to allocate SPIs/SGIs/PPIs at boot time. This number
will never change. We can avoid to always to allocate 1024 IRQs by using
the number provided by the GIC.

[..]

>  void __cpuinit init_secondary_IRQ(void)
> @@ -265,6 +269,7 @@ void release_irq(unsigned int irq, const void *dev_id)
>  
>      desc = irq_to_desc(irq);
>  
> +    ASSERT(desc != NULL);

What happen if the code decides to try to free a non-existing IRQ on
non-debug mode? It will just segfault... and bring a possible security
issue.

>      spin_lock_irqsave(&desc->lock,flags);
>  
>      action_ptr = &desc->action;
> @@ -301,6 +306,11 @@ void release_irq(unsigned int irq, const void *dev_id)
>  
>      if ( action->free_on_release )
>          xfree(action);
> +
> +    spin_lock(&rb_tree_desc_lock);
> +    delete_irq_desc(&desc_root, irq);
> +    xfree(desc);
> +    spin_unlock(&rb_tree_desc_lock);
>  }
>  
>  static int __setup_irq(struct irq_desc *desc, unsigned int irqflags,
> @@ -339,6 +349,8 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
>  
>      desc = irq_to_desc(irq);
>  
> +    ASSERT(desc != NULL);
> +
>      spin_lock_irqsave(&desc->lock, flags);
>  
>      if ( test_bit(_IRQ_GUEST, &desc->status) )
> @@ -382,7 +394,7 @@ int route_irq_to_guest(struct domain *d, unsigned int irq,
>                         const char * devname)
>  {
>      struct irqaction *action;
> -    struct irq_desc *desc = irq_to_desc(irq);
> +    struct irq_desc *desc = NULL;

Pointless initialization.

>      unsigned long flags;
>      int retval = 0;
>  
> @@ -394,6 +406,23 @@ int route_irq_to_guest(struct domain *d, unsigned int irq,
>      action->name = devname;
>      action->free_on_release = 1;
>  
> +    spin_lock(&rb_tree_desc_lock);
> +    desc = find_irq_desc(&desc_root, irq);
> +
> +    if ( desc == NULL )
> +    {
> +        /* Allocate descriptor and add to rb tree */
> +        desc = xzalloc(struct irq_desc);
> +        if ( desc == NULL )
> +        {
> +            spin_unlock(&rb_tree_desc_lock);
> +            return -ENOMEM;
> +        }
> +        init_irq_data(irq, desc);
> +        insert_irq_desc(&desc_root, irq, desc);
> +    }
> +    spin_unlock(&rb_tree_desc_lock);
> +

Why do you need this in route_irq_to_guest?

>      spin_lock_irqsave(&desc->lock, flags);
>  
>      /* If the IRQ is already used by someone
> @@ -470,13 +499,16 @@ static bool_t irq_validate_new_type(unsigned int curr, unsigned new)
>  int irq_set_spi_type(unsigned int spi, unsigned int type)
>  {
>      unsigned long flags;
> -    struct irq_desc *desc = irq_to_desc(spi);
> +    struct irq_desc *desc = NULL;
>      int ret = -EBUSY;
>  
>      /* This function should not be used for other than SPIs */
>      if ( spi < NR_LOCAL_IRQS )
>          return -EINVAL;
>  
> +    desc = irq_to_desc(spi);
> +    ASSERT(desc != NULL);
> +
>      spin_lock_irqsave(&desc->lock, flags);
>  
>      if ( !irq_validate_new_type(desc->arch.type, type) )
> @@ -532,6 +564,7 @@ int platform_get_irq(const struct dt_device_node *device, int index)
>  {
>      struct dt_irq dt_irq;
>      unsigned int type, irq;
> +    struct irq_desc *desc;
>      int res;
>  
>      res = dt_device_get_irq(device, index, &dt_irq);
> @@ -541,6 +574,22 @@ int platform_get_irq(const struct dt_device_node *device, int index)
>      irq = dt_irq.irq;
>      type = dt_irq.type;
>  
> +    spin_lock(&rb_tree_desc_lock);
> +    desc = find_irq_desc(&desc_root, irq);
> +    if ( desc == NULL )
> +    {
> +        /* Allocate descriptor for this irq */
> +        desc = xzalloc(struct irq_desc);
> +        if ( desc == NULL )
> +        {
> +            spin_unlock(&rb_tree_desc_lock);
> +            return -ENOMEM;
> +        }
> +        init_irq_data(irq, desc);
> +        insert_irq_desc(&desc_root, irq, desc);
> +    }
> +    spin_unlock(&rb_tree_desc_lock);
> +

>      /* Setup the IRQ type */
>      if ( irq < NR_LOCAL_IRQS )
>          res = irq_local_set_type(irq, type);
> diff --git a/xen/common/irq.c b/xen/common/irq.c
> index 3e55dfa..ddf85c5 100644
> --- a/xen/common/irq.c
> +++ b/xen/common/irq.c
> @@ -40,3 +40,61 @@ unsigned int irq_startup_none(struct irq_desc *desc)
>  {
>      return 0;
>  }
> +
> +irq_desc_t * find_irq_desc( struct rb_root *root_node, int irq)
> +{
> +    struct rb_node *node = root_node->rb_node;
> +
> +    while ( node )
> +    {
> +        irq_desc_t *this;
> +
> +        this = container_of(node, irq_desc_t, node);
> + 
> +        if ( irq < this->irq )
> +            node = node->rb_left;
> +        else if (irq > this->irq)
> +            node = node->rb_right;
> +        else
> +            return this;
> +    }
> + 
> +    return NULL;
> +}
> +
> +int insert_irq_desc(struct rb_root *root_node, int irq, struct irq_desc *desc)
> +{
> +    struct rb_node **new, *parent;
> +
> +    new = &root_node->rb_node;
> +    parent = NULL;
> +
> +    while ( *new )
> +    {
> +        irq_desc_t *this;
> +
> +        this = container_of(*new, irq_desc_t, node);
> + 
> +        parent = *new;
> +        if ( desc->irq < this->irq )
> +            new = &((*new)->rb_left);
> +        else if (desc->irq > this->irq)
> +            new = &((*new)->rb_right);
> +        else
> +            return -EEXIST;
> +    }
> + 
> +    rb_link_node(&desc->node, parent, new);
> +    rb_insert_color(&desc->node, root_node);
> +
> +    return 0;
> +}
> +
> +void delete_irq_desc(struct rb_root *root_node, int irq)
> +{
> +   struct irq_desc *desc;
> +
> +   desc = find_irq_desc(root_node, irq);
> +   if ( desc )
> +      rb_erase(&desc->node, root_node);
> +}

What's the matter to put this function on the common code when the
variable itself lives in the architecture code?

> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index 9e0155c..d623ce3 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -6,6 +6,7 @@
>  #include <xen/spinlock.h>
>  #include <xen/time.h>
>  #include <xen/list.h>
> +#include <xen/rbtree.h>
>  #include <asm/regs.h>
>  #include <asm/hardirq.h>
>  
> @@ -83,6 +84,7 @@ struct msi_desc;
>   * first field.
>   */
>  typedef struct irq_desc {
> +    struct rb_node node;

Please move this after the field status. So code on ARM rely on the
alignment to access this field without lock (via bit_* manipulation).

>      unsigned int status;        /* IRQ status */
>      hw_irq_controller *handler;
>      struct msi_desc   *msi_desc;
> @@ -168,6 +170,10 @@ static inline void set_native_irq_info(unsigned int irq, const cpumask_t *mask)
>  
>  unsigned int set_desc_affinity(struct irq_desc *, const cpumask_t *);
>  
> +irq_desc_t * find_irq_desc(struct rb_root *root_node, int irq);
> +int insert_irq_desc(struct rb_root *root_node, int irq, struct irq_desc *desc);
> +void delete_irq_desc(struct rb_root *root_node, int irq);
> +
>  #ifndef arch_hwdom_irqs
>  unsigned int arch_hwdom_irqs(domid_t);
>  #endif
> 

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Implement dynamic allocation of irq descriptors

[Vijay Kilari]

On Thu, Feb 19, 2015 at 7:33 PM, Julien Grall <julien.grall@linaro.org> wrote:
> On 19/02/15 07:17, vijay.kilari@gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> For arm memory for 1024 irq descriptors are allocated
>> statically irrespective of number of interrupt supported
>> by the platform.
>>
>> With this patch, irq descriptors are allocated at run time
>> and managed using red-black tree. Functions to insert, search
>> and delete irq descriptor are implemented in xen/common/irq.c.
>
> I think we may want to allocate SPIs/SGIs/PPIs at boot time. This number
> will never change. We can avoid to always to allocate 1024 IRQs by using
> the number provided by the GIC.

The irq descriptor is allocated when platform_get_irq() is called or
route_irq_to_guest()
only. So we are not allocating based on GIC.

BTW, I have a query regarding pending_irq[].

pending_irq[] structure is also allocated boot time. Shouldn't we
allocate similar to
irq descriptors?. In case of LPI's (ITS) interrupts we cannot pre-allocate the
GIC ITS supported number of pending_irq[] at boot time.

If so, I propose to allocate pending_irq[] along with irq_descritor and destroy
along with irq descriptor

>
> [..]
>
>>  void __cpuinit init_secondary_IRQ(void)
>> @@ -265,6 +269,7 @@ void release_irq(unsigned int irq, const void *dev_id)
>>
>>      desc = irq_to_desc(irq);
>>
>> +    ASSERT(desc != NULL);
>
> What happen if the code decides to try to free a non-existing IRQ on
> non-debug mode? It will just segfault... and bring a possible security
> issue.

 Should quietly return?

>
>>      spin_lock_irqsave(&desc->lock,flags);
>>
>>      action_ptr = &desc->action;
>> @@ -301,6 +306,11 @@ void release_irq(unsigned int irq, const void *dev_id)
>>
>>      if ( action->free_on_release )
>>          xfree(action);
>> +
>> +    spin_lock(&rb_tree_desc_lock);
>> +    delete_irq_desc(&desc_root, irq);
>> +    xfree(desc);
>> +    spin_unlock(&rb_tree_desc_lock);
>>  }
>>
>>  static int __setup_irq(struct irq_desc *desc, unsigned int irqflags,
>> @@ -339,6 +349,8 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
>>
>>      desc = irq_to_desc(irq);
>>
>> +    ASSERT(desc != NULL);
>> +
>>      spin_lock_irqsave(&desc->lock, flags);
>>
>>      if ( test_bit(_IRQ_GUEST, &desc->status) )
>> @@ -382,7 +394,7 @@ int route_irq_to_guest(struct domain *d, unsigned int irq,
>>                         const char * devname)
>>  {
>>      struct irqaction *action;
>> -    struct irq_desc *desc = irq_to_desc(irq);
>> +    struct irq_desc *desc = NULL;
>
> Pointless initialization.
>
>>      unsigned long flags;
>>      int retval = 0;
>>
>> @@ -394,6 +406,23 @@ int route_irq_to_guest(struct domain *d, unsigned int irq,
>>      action->name = devname;
>>      action->free_on_release = 1;
>>
>> +    spin_lock(&rb_tree_desc_lock);
>> +    desc = find_irq_desc(&desc_root, irq);
>> +
>> +    if ( desc == NULL )
>> +    {
>> +        /* Allocate descriptor and add to rb tree */
>> +        desc = xzalloc(struct irq_desc);
>> +        if ( desc == NULL )
>> +        {
>> +            spin_unlock(&rb_tree_desc_lock);
>> +            return -ENOMEM;
>> +        }
>> +        init_irq_data(irq, desc);
>> +        insert_irq_desc(&desc_root, irq, desc);
>> +    }
>> +    spin_unlock(&rb_tree_desc_lock);
>> +
>
> Why do you need this in route_irq_to_guest?

For LPI's descriptors are allocated only on ITS command. In this
case we call route_irq_to_guest. So here we allocate memory for
descriptor

>
>>      spin_lock_irqsave(&desc->lock, flags);
>>
>>      /* If the IRQ is already used by someone
>> @@ -470,13 +499,16 @@ static bool_t irq_validate_new_type(unsigned int curr, unsigned new)
>>  int irq_set_spi_type(unsigned int spi, unsigned int type)
>>  {
>>      unsigned long flags;
>> -    struct irq_desc *desc = irq_to_desc(spi);
>> +    struct irq_desc *desc = NULL;
>>      int ret = -EBUSY;
>>
>>      /* This function should not be used for other than SPIs */
>>      if ( spi < NR_LOCAL_IRQS )
>>          return -EINVAL;
>>
>> +    desc = irq_to_desc(spi);
>> +    ASSERT(desc != NULL);
>> +
>>      spin_lock_irqsave(&desc->lock, flags);
>>
>>      if ( !irq_validate_new_type(desc->arch.type, type) )
>> @@ -532,6 +564,7 @@ int platform_get_irq(const struct dt_device_node *device, int index)
>>  {
>>      struct dt_irq dt_irq;
>>      unsigned int type, irq;
>> +    struct irq_desc *desc;
>>      int res;
>>
>>      res = dt_device_get_irq(device, index, &dt_irq);
>> @@ -541,6 +574,22 @@ int platform_get_irq(const struct dt_device_node *device, int index)
>>      irq = dt_irq.irq;
>>      type = dt_irq.type;
>>
>> +    spin_lock(&rb_tree_desc_lock);
>> +    desc = find_irq_desc(&desc_root, irq);
>> +    if ( desc == NULL )
>> +    {
>> +        /* Allocate descriptor for this irq */
>> +        desc = xzalloc(struct irq_desc);
>> +        if ( desc == NULL )
>> +        {
>> +            spin_unlock(&rb_tree_desc_lock);
>> +            return -ENOMEM;
>> +        }
>> +        init_irq_data(irq, desc);
>> +        insert_irq_desc(&desc_root, irq, desc);
>> +    }
>> +    spin_unlock(&rb_tree_desc_lock);
>> +
>
>>      /* Setup the IRQ type */
>>      if ( irq < NR_LOCAL_IRQS )
>>          res = irq_local_set_type(irq, type);
>> diff --git a/xen/common/irq.c b/xen/common/irq.c
>> index 3e55dfa..ddf85c5 100644
>> --- a/xen/common/irq.c
>> +++ b/xen/common/irq.c
>> @@ -40,3 +40,61 @@ unsigned int irq_startup_none(struct irq_desc *desc)
>>  {
>>      return 0;
>>  }
>> +
>> +irq_desc_t * find_irq_desc( struct rb_root *root_node, int irq)
>> +{
>> +    struct rb_node *node = root_node->rb_node;
>> +
>> +    while ( node )
>> +    {
>> +        irq_desc_t *this;
>> +
>> +        this = container_of(node, irq_desc_t, node);
>> +
>> +        if ( irq < this->irq )
>> +            node = node->rb_left;
>> +        else if (irq > this->irq)
>> +            node = node->rb_right;
>> +        else
>> +            return this;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +int insert_irq_desc(struct rb_root *root_node, int irq, struct irq_desc *desc)
>> +{
>> +    struct rb_node **new, *parent;
>> +
>> +    new = &root_node->rb_node;
>> +    parent = NULL;
>> +
>> +    while ( *new )
>> +    {
>> +        irq_desc_t *this;
>> +
>> +        this = container_of(*new, irq_desc_t, node);
>> +
>> +        parent = *new;
>> +        if ( desc->irq < this->irq )
>> +            new = &((*new)->rb_left);
>> +        else if (desc->irq > this->irq)
>> +            new = &((*new)->rb_right);
>> +        else
>> +            return -EEXIST;
>> +    }
>> +
>> +    rb_link_node(&desc->node, parent, new);
>> +    rb_insert_color(&desc->node, root_node);
>> +
>> +    return 0;
>> +}
>> +
>> +void delete_irq_desc(struct rb_root *root_node, int irq)
>> +{
>> +   struct irq_desc *desc;
>> +
>> +   desc = find_irq_desc(root_node, irq);
>> +   if ( desc )
>> +      rb_erase(&desc->node, root_node);
>> +}
>
> What's the matter to put this function on the common code when the
> variable itself lives in the architecture code?

Which code is common here?

>
>> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
>> index 9e0155c..d623ce3 100644
>> --- a/xen/include/xen/irq.h
>> +++ b/xen/include/xen/irq.h
>> @@ -6,6 +6,7 @@
>>  #include <xen/spinlock.h>
>>  #include <xen/time.h>
>>  #include <xen/list.h>
>> +#include <xen/rbtree.h>
>>  #include <asm/regs.h>
>>  #include <asm/hardirq.h>
>>
>> @@ -83,6 +84,7 @@ struct msi_desc;
>>   * first field.
>>   */
>>  typedef struct irq_desc {
>> +    struct rb_node node;
>
> Please move this after the field status. So code on ARM rely on the
> alignment to access this field without lock (via bit_* manipulation).
>
>>      unsigned int status;        /* IRQ status */
>>      hw_irq_controller *handler;
>>      struct msi_desc   *msi_desc;
>> @@ -168,6 +170,10 @@ static inline void set_native_irq_info(unsigned int irq, const cpumask_t *mask)
>>
>>  unsigned int set_desc_affinity(struct irq_desc *, const cpumask_t *);
>>
>> +irq_desc_t * find_irq_desc(struct rb_root *root_node, int irq);
>> +int insert_irq_desc(struct rb_root *root_node, int irq, struct irq_desc *desc);
>> +void delete_irq_desc(struct rb_root *root_node, int irq);
>> +
>>  #ifndef arch_hwdom_irqs
>>  unsigned int arch_hwdom_irqs(domid_t);
>>  #endif
>>
>
> Regards,
>
> --
> Julien Grall

Re: [PATCH] xen/arm: Implement dynamic allocation of irq descriptors

[Julien Grall]

Hello Vijay,

On 23/02/15 13:04, Vijay Kilari wrote:
> On Thu, Feb 19, 2015 at 7:33 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> On 19/02/15 07:17, vijay.kilari@gmail.com wrote:
>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>
>>> For arm memory for 1024 irq descriptors are allocated
>>> statically irrespective of number of interrupt supported
>>> by the platform.
>>>
>>> With this patch, irq descriptors are allocated at run time
>>> and managed using red-black tree. Functions to insert, search
>>> and delete irq descriptor are implemented in xen/common/irq.c.
>>
>> I think we may want to allocate SPIs/SGIs/PPIs at boot time. This number
>> will never change. We can avoid to always to allocate 1024 IRQs by using
>> the number provided by the GIC.
> 
> The irq descriptor is allocated when platform_get_irq() is called or
> route_irq_to_guest()
> only. So we are not allocating based on GIC.

You didn't understand what I said... I was suggesting to allocate SPIs
at boot time. Using an array for them allow us to access to IRQ desc in
constant time. This may help interrupt latency.

> BTW, I have a query regarding pending_irq[].
> 
> pending_irq[] structure is also allocated boot time. Shouldn't we
> allocate similar to
> irq descriptors?. In case of LPI's (ITS) interrupts we cannot pre-allocate the
> GIC ITS supported number of pending_irq[] at boot time.

pending_irqs is not allocated at boot time but at domain creation.

I think we should consider to create a separate structure for LPI's.

> If so, I propose to allocate pending_irq[] along with irq_descritor and destroy
> along with irq descriptor
> 
>>
>> [..]
>>
>>>  void __cpuinit init_secondary_IRQ(void)
>>> @@ -265,6 +269,7 @@ void release_irq(unsigned int irq, const void *dev_id)
>>>
>>>      desc = irq_to_desc(irq);
>>>
>>> +    ASSERT(desc != NULL);
>>
>> What happen if the code decides to try to free a non-existing IRQ on
>> non-debug mode? It will just segfault... and bring a possible security
>> issue.
> 
>  Should quietly return?

Yes, of course.

[..]

>>>      unsigned long flags;
>>>      int retval = 0;
>>>
>>> @@ -394,6 +406,23 @@ int route_irq_to_guest(struct domain *d, unsigned int irq,
>>>      action->name = devname;
>>>      action->free_on_release = 1;
>>>
>>> +    spin_lock(&rb_tree_desc_lock);
>>> +    desc = find_irq_desc(&desc_root, irq);
>>> +
>>> +    if ( desc == NULL )
>>> +    {
>>> +        /* Allocate descriptor and add to rb tree */
>>> +        desc = xzalloc(struct irq_desc);
>>> +        if ( desc == NULL )
>>> +        {
>>> +            spin_unlock(&rb_tree_desc_lock);
>>> +            return -ENOMEM;
>>> +        }
>>> +        init_irq_data(irq, desc);
>>> +        insert_irq_desc(&desc_root, irq, desc);
>>> +    }
>>> +    spin_unlock(&rb_tree_desc_lock);
>>> +
>>
>> Why do you need this in route_irq_to_guest?
> 
> For LPI's descriptors are allocated only on ITS command. In this
> case we call route_irq_to_guest. So here we allocate memory for
> descriptor

We don't support LPIs right now, so this change doesn't make sense.

Aside that, this is not the right place. route_irq_to_guest should route
a validate physical IRQ to the guest.

LPIs should have been created before, for instance while adding a new
PCI. Otherwise, who will validate the validity of the LPIs?

>>
>>>      spin_lock_irqsave(&desc->lock, flags);
>>>
>>>      /* If the IRQ is already used by someone
>>> @@ -470,13 +499,16 @@ static bool_t irq_validate_new_type(unsigned int curr, unsigned new)
>>>  int irq_set_spi_type(unsigned int spi, unsigned int type)
>>>  {
>>>      unsigned long flags;
>>> -    struct irq_desc *desc = irq_to_desc(spi);
>>> +    struct irq_desc *desc = NULL;
>>>      int ret = -EBUSY;
>>>
>>>      /* This function should not be used for other than SPIs */
>>>      if ( spi < NR_LOCAL_IRQS )
>>>          return -EINVAL;
>>>
>>> +    desc = irq_to_desc(spi);
>>> +    ASSERT(desc != NULL);
>>> +
>>>      spin_lock_irqsave(&desc->lock, flags);
>>>
>>>      if ( !irq_validate_new_type(desc->arch.type, type) )
>>> @@ -532,6 +564,7 @@ int platform_get_irq(const struct dt_device_node *device, int index)
>>>  {
>>>      struct dt_irq dt_irq;
>>>      unsigned int type, irq;
>>> +    struct irq_desc *desc;
>>>      int res;
>>>
>>>      res = dt_device_get_irq(device, index, &dt_irq);
>>> @@ -541,6 +574,22 @@ int platform_get_irq(const struct dt_device_node *device, int index)
>>>      irq = dt_irq.irq;
>>>      type = dt_irq.type;
>>>
>>> +    spin_lock(&rb_tree_desc_lock);
>>> +    desc = find_irq_desc(&desc_root, irq);
>>> +    if ( desc == NULL )
>>> +    {
>>> +        /* Allocate descriptor for this irq */
>>> +        desc = xzalloc(struct irq_desc);
>>> +        if ( desc == NULL )
>>> +        {
>>> +            spin_unlock(&rb_tree_desc_lock);
>>> +            return -ENOMEM;
>>> +        }
>>> +        init_irq_data(irq, desc);
>>> +        insert_irq_desc(&desc_root, irq, desc);
>>> +    }
>>> +    spin_unlock(&rb_tree_desc_lock);
>>> +
>>
>>>      /* Setup the IRQ type */
>>>      if ( irq < NR_LOCAL_IRQS )
>>>          res = irq_local_set_type(irq, type);
>>> diff --git a/xen/common/irq.c b/xen/common/irq.c
>>> index 3e55dfa..ddf85c5 100644
>>> --- a/xen/common/irq.c
>>> +++ b/xen/common/irq.c
>>> @@ -40,3 +40,61 @@ unsigned int irq_startup_none(struct irq_desc *desc)
>>>  {
>>>      return 0;
>>>  }
>>> +
>>> +irq_desc_t * find_irq_desc( struct rb_root *root_node, int irq)
>>> +{
>>> +    struct rb_node *node = root_node->rb_node;
>>> +
>>> +    while ( node )
>>> +    {
>>> +        irq_desc_t *this;
>>> +
>>> +        this = container_of(node, irq_desc_t, node);
>>> +
>>> +        if ( irq < this->irq )
>>> +            node = node->rb_left;
>>> +        else if (irq > this->irq)
>>> +            node = node->rb_right;
>>> +        else
>>> +            return this;
>>> +    }
>>> +
>>> +    return NULL;
>>> +}
>>> +
>>> +int insert_irq_desc(struct rb_root *root_node, int irq, struct irq_desc *desc)
>>> +{
>>> +    struct rb_node **new, *parent;
>>> +
>>> +    new = &root_node->rb_node;
>>> +    parent = NULL;
>>> +
>>> +    while ( *new )
>>> +    {
>>> +        irq_desc_t *this;
>>> +
>>> +        this = container_of(*new, irq_desc_t, node);
>>> +
>>> +        parent = *new;
>>> +        if ( desc->irq < this->irq )
>>> +            new = &((*new)->rb_left);
>>> +        else if (desc->irq > this->irq)
>>> +            new = &((*new)->rb_right);
>>> +        else
>>> +            return -EEXIST;
>>> +    }
>>> +
>>> +    rb_link_node(&desc->node, parent, new);
>>> +    rb_insert_color(&desc->node, root_node);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +void delete_irq_desc(struct rb_root *root_node, int irq)
>>> +{
>>> +   struct irq_desc *desc;
>>> +
>>> +   desc = find_irq_desc(root_node, irq);
>>> +   if ( desc )
>>> +      rb_erase(&desc->node, root_node);
>>> +}
>>
>> What's the matter to put this function on the common code when the
>> variable itself lives in the architecture code?
> 
> Which code is common here?

Any file living in xen/common is part of the common code...

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Implement dynamic allocation of irq descriptors

[Vijay Kilari]

Hi Julien,

On Mon, Feb 23, 2015 at 9:10 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hello Vijay,
>
> On 23/02/15 13:04, Vijay Kilari wrote:
>> On Thu, Feb 19, 2015 at 7:33 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>> On 19/02/15 07:17, vijay.kilari@gmail.com wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>>
>>>> For arm memory for 1024 irq descriptors are allocated
>>>> statically irrespective of number of interrupt supported
>>>> by the platform.
>>>>
>>>> With this patch, irq descriptors are allocated at run time
>>>> and managed using red-black tree. Functions to insert, search
>>>> and delete irq descriptor are implemented in xen/common/irq.c.
>>>
>>> I think we may want to allocate SPIs/SGIs/PPIs at boot time. This number
>>> will never change. We can avoid to always to allocate 1024 IRQs by using
>>> the number provided by the GIC.
>>
>> The irq descriptor is allocated when platform_get_irq() is called or
>> route_irq_to_guest()
>> only. So we are not allocating based on GIC.
>
> You didn't understand what I said... I was suggesting to allocate SPIs
> at boot time. Using an array for them allow us to access to IRQ desc in
> constant time. This may help interrupt latency.

Yes, I have thought about it. May be we can choose this approach for SPIs
but for LPI's, the GIC can support upto ~32K. So in this case it won't make
sense for LPI's

>
>> BTW, I have a query regarding pending_irq[].
>>
>> pending_irq[] structure is also allocated boot time. Shouldn't we
>> allocate similar to
>> irq descriptors?. In case of LPI's (ITS) interrupts we cannot pre-allocate the
>> GIC ITS supported number of pending_irq[] at boot time.
>
> pending_irqs is not allocated at boot time but at domain creation.
>
> I think we should consider to create a separate structure for LPI's.

Yes, I have created separate structure for LPI's for ITS driver.
 But as I said LPI's are to many, so cannot allocate at domain creation.

>
>> If so, I propose to allocate pending_irq[] along with irq_descritor and destroy
>> along with irq descriptor
>>
>>>
>>> [..]
>>>
>>>>  void __cpuinit init_secondary_IRQ(void)
>>>> @@ -265,6 +269,7 @@ void release_irq(unsigned int irq, const void *dev_id)
>>>>
>>>>      desc = irq_to_desc(irq);
>>>>
>>>> +    ASSERT(desc != NULL);
>>>
>>> What happen if the code decides to try to free a non-existing IRQ on
>>> non-debug mode? It will just segfault... and bring a possible security
>>> issue.
>>
>>  Should quietly return?
>
> Yes, of course.
>
> [..]
>
>>>>      unsigned long flags;
>>>>      int retval = 0;
>>>>
>>>> @@ -394,6 +406,23 @@ int route_irq_to_guest(struct domain *d, unsigned int irq,
>>>>      action->name = devname;
>>>>      action->free_on_release = 1;
>>>>
>>>> +    spin_lock(&rb_tree_desc_lock);
>>>> +    desc = find_irq_desc(&desc_root, irq);
>>>> +
>>>> +    if ( desc == NULL )
>>>> +    {
>>>> +        /* Allocate descriptor and add to rb tree */
>>>> +        desc = xzalloc(struct irq_desc);
>>>> +        if ( desc == NULL )
>>>> +        {
>>>> +            spin_unlock(&rb_tree_desc_lock);
>>>> +            return -ENOMEM;
>>>> +        }
>>>> +        init_irq_data(irq, desc);
>>>> +        insert_irq_desc(&desc_root, irq, desc);
>>>> +    }
>>>> +    spin_unlock(&rb_tree_desc_lock);
>>>> +
>>>
>>> Why do you need this in route_irq_to_guest?
>>
>> For LPI's descriptors are allocated only on ITS command. In this
>> case we call route_irq_to_guest. So here we allocate memory for
>> descriptor
>
> We don't support LPIs right now, so this change doesn't make sense.

With addition of ITS driver it makes sense. So I have added it.
May be I can remove this for now and add it along with ITS driver.

>
> Aside that, this is not the right place. route_irq_to_guest should route
> a validate physical IRQ to the guest.
>
> LPIs should have been created before, for instance while adding a new
> PCI. Otherwise, who will validate the validity of the LPIs?
>
>>>
>>>>      spin_lock_irqsave(&desc->lock, flags);
>>>>
>>>>      /* If the IRQ is already used by someone
>>>> @@ -470,13 +499,16 @@ static bool_t irq_validate_new_type(unsigned int curr, unsigned new)
>>>>  int irq_set_spi_type(unsigned int spi, unsigned int type)
>>>>  {
>>>>      unsigned long flags;
>>>> -    struct irq_desc *desc = irq_to_desc(spi);
>>>> +    struct irq_desc *desc = NULL;
>>>>      int ret = -EBUSY;
>>>>
>>>>      /* This function should not be used for other than SPIs */
>>>>      if ( spi < NR_LOCAL_IRQS )
>>>>          return -EINVAL;
>>>>
>>>> +    desc = irq_to_desc(spi);
>>>> +    ASSERT(desc != NULL);
>>>> +
>>>>      spin_lock_irqsave(&desc->lock, flags);
>>>>
>>>>      if ( !irq_validate_new_type(desc->arch.type, type) )
>>>> @@ -532,6 +564,7 @@ int platform_get_irq(const struct dt_device_node *device, int index)
>>>>  {
>>>>      struct dt_irq dt_irq;
>>>>      unsigned int type, irq;
>>>> +    struct irq_desc *desc;
>>>>      int res;
>>>>
>>>>      res = dt_device_get_irq(device, index, &dt_irq);
>>>> @@ -541,6 +574,22 @@ int platform_get_irq(const struct dt_device_node *device, int index)
>>>>      irq = dt_irq.irq;
>>>>      type = dt_irq.type;
>>>>
>>>> +    spin_lock(&rb_tree_desc_lock);
>>>> +    desc = find_irq_desc(&desc_root, irq);
>>>> +    if ( desc == NULL )
>>>> +    {
>>>> +        /* Allocate descriptor for this irq */
>>>> +        desc = xzalloc(struct irq_desc);
>>>> +        if ( desc == NULL )
>>>> +        {
>>>> +            spin_unlock(&rb_tree_desc_lock);
>>>> +            return -ENOMEM;
>>>> +        }
>>>> +        init_irq_data(irq, desc);
>>>> +        insert_irq_desc(&desc_root, irq, desc);
>>>> +    }
>>>> +    spin_unlock(&rb_tree_desc_lock);
>>>> +
>>>
>>>>      /* Setup the IRQ type */
>>>>      if ( irq < NR_LOCAL_IRQS )
>>>>          res = irq_local_set_type(irq, type);
>>>> diff --git a/xen/common/irq.c b/xen/common/irq.c
>>>> index 3e55dfa..ddf85c5 100644
>>>> --- a/xen/common/irq.c
>>>> +++ b/xen/common/irq.c
>>>> @@ -40,3 +40,61 @@ unsigned int irq_startup_none(struct irq_desc *desc)
>>>>  {
>>>>      return 0;
>>>>  }
>>>> +
>>>> +irq_desc_t * find_irq_desc( struct rb_root *root_node, int irq)
>>>> +{
>>>> +    struct rb_node *node = root_node->rb_node;
>>>> +
>>>> +    while ( node )
>>>> +    {
>>>> +        irq_desc_t *this;
>>>> +
>>>> +        this = container_of(node, irq_desc_t, node);
>>>> +
>>>> +        if ( irq < this->irq )
>>>> +            node = node->rb_left;
>>>> +        else if (irq > this->irq)
>>>> +            node = node->rb_right;
>>>> +        else
>>>> +            return this;
>>>> +    }
>>>> +
>>>> +    return NULL;
>>>> +}
>>>> +
>>>> +int insert_irq_desc(struct rb_root *root_node, int irq, struct irq_desc *desc)
>>>> +{
>>>> +    struct rb_node **new, *parent;
>>>> +
>>>> +    new = &root_node->rb_node;
>>>> +    parent = NULL;
>>>> +
>>>> +    while ( *new )
>>>> +    {
>>>> +        irq_desc_t *this;
>>>> +
>>>> +        this = container_of(*new, irq_desc_t, node);
>>>> +
>>>> +        parent = *new;
>>>> +        if ( desc->irq < this->irq )
>>>> +            new = &((*new)->rb_left);
>>>> +        else if (desc->irq > this->irq)
>>>> +            new = &((*new)->rb_right);
>>>> +        else
>>>> +            return -EEXIST;
>>>> +    }
>>>> +
>>>> +    rb_link_node(&desc->node, parent, new);
>>>> +    rb_insert_color(&desc->node, root_node);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +void delete_irq_desc(struct rb_root *root_node, int irq)
>>>> +{
>>>> +   struct irq_desc *desc;
>>>> +
>>>> +   desc = find_irq_desc(root_node, irq);
>>>> +   if ( desc )
>>>> +      rb_erase(&desc->node, root_node);
>>>> +}
>>>
>>> What's the matter to put this function on the common code when the
>>> variable itself lives in the architecture code?
>>
>> Which code is common here?
>
> Any file living in xen/common is part of the common code...

The rb_root variable is in arch specific code. But it is passed as parameter.
The API's to handle addtion/deletion/search based on irq can be use by
other platforms eg. x86 irq_desc being commong structure (xen/include/xen/irq.h)

regards
Vijay

Re: [PATCH] xen/arm: Implement dynamic allocation of irq descriptors

[Julien Grall]

On 23/02/15 16:09, Vijay Kilari wrote:
> On Mon, Feb 23, 2015 at 9:10 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hello Vijay,
>>
>> On 23/02/15 13:04, Vijay Kilari wrote:
>>> On Thu, Feb 19, 2015 at 7:33 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>>> On 19/02/15 07:17, vijay.kilari@gmail.com wrote:
>>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>>>
>>>>> For arm memory for 1024 irq descriptors are allocated
>>>>> statically irrespective of number of interrupt supported
>>>>> by the platform.
>>>>>
>>>>> With this patch, irq descriptors are allocated at run time
>>>>> and managed using red-black tree. Functions to insert, search
>>>>> and delete irq descriptor are implemented in xen/common/irq.c.
>>>>
>>>> I think we may want to allocate SPIs/SGIs/PPIs at boot time. This number
>>>> will never change. We can avoid to always to allocate 1024 IRQs by using
>>>> the number provided by the GIC.
>>>
>>> The irq descriptor is allocated when platform_get_irq() is called or
>>> route_irq_to_guest()
>>> only. So we are not allocating based on GIC.
>>
>> You didn't understand what I said... I was suggesting to allocate SPIs
>> at boot time. Using an array for them allow us to access to IRQ desc in
>> constant time. This may help interrupt latency.
> 
> Yes, I have thought about it. May be we can choose this approach for SPIs
> but for LPI's, the GIC can support upto ~32K. So in this case it won't make
> sense for LPI's

Again... I never said it was a bad idea for LPIs. I was only point out
that it may not be worth for SPIs.

[..]

>> I think we should consider to create a separate structure for LPI's.
> 
> Yes, I have created separate structure for LPI's for ITS driver.
>  But as I said LPI's are to many, so cannot allocate at domain creation.

Ditto. The new structure can be a radix tree. I never suggested to use
an array.

[..]

>>>> What's the matter to put this function on the common code when the
>>>> variable itself lives in the architecture code?
>>>
>>> Which code is common here?
>>
>> Any file living in xen/common is part of the common code...
> 
> The rb_root variable is in arch specific code. But it is passed as parameter.
> The API's to handle addtion/deletion/search based on irq can be use by
> other platforms eg. x86 irq_desc being commong structure (xen/include/xen/irq.h)

Please add a comment somewhere to explain it.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Implement dynamic allocation of irq descriptors

[Vijay Kilari]

Hi Julien,

On Mon, Feb 23, 2015 at 9:55 PM, Julien Grall <julien.grall@linaro.org> wrote:
> On 23/02/15 16:09, Vijay Kilari wrote:
>> On Mon, Feb 23, 2015 at 9:10 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>> Hello Vijay,
>>>
>>> On 23/02/15 13:04, Vijay Kilari wrote:
>>>> On Thu, Feb 19, 2015 at 7:33 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>>>> On 19/02/15 07:17, vijay.kilari@gmail.com wrote:
>>>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>>>>
>>>>>> For arm memory for 1024 irq descriptors are allocated
>>>>>> statically irrespective of number of interrupt supported
>>>>>> by the platform.
>>>>>>
>>>>>> With this patch, irq descriptors are allocated at run time
>>>>>> and managed using red-black tree. Functions to insert, search
>>>>>> and delete irq descriptor are implemented in xen/common/irq.c.
>>>>>
>>>>> I think we may want to allocate SPIs/SGIs/PPIs at boot time. This number
>>>>> will never change. We can avoid to always to allocate 1024 IRQs by using
>>>>> the number provided by the GIC.
>>>>
>>>> The irq descriptor is allocated when platform_get_irq() is called or
>>>> route_irq_to_guest()
>>>> only. So we are not allocating based on GIC.
>>>
>>> You didn't understand what I said... I was suggesting to allocate SPIs
>>> at boot time. Using an array for them allow us to access to IRQ desc in
>>> constant time. This may help interrupt latency.
>>
>> Yes, I have thought about it. May be we can choose this approach for SPIs
>> but for LPI's, the GIC can support upto ~32K. So in this case it won't make
>> sense for LPI's
>
> Again... I never said it was a bad idea for LPIs. I was only point out
> that it may not be worth for SPIs.
>
> [..]
>
>>> I think we should consider to create a separate structure for LPI's.
>>
>> Yes, I have created separate structure for LPI's for ITS driver.
>>  But as I said LPI's are to many, so cannot allocate at domain creation.
>
> Ditto. The new structure can be a radix tree. I never suggested to use
> an array.

Thanks for your suggestions. Are u refering to Radix or rb tree?

Regards
Vijay

Re: [PATCH] xen/arm: Implement dynamic allocation of irq descriptors

[Julien Grall]

On 23/02/15 16:40, Vijay Kilari wrote:
> Hi Julien,
> 
> On Mon, Feb 23, 2015 at 9:55 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> On 23/02/15 16:09, Vijay Kilari wrote:
>>> On Mon, Feb 23, 2015 at 9:10 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>>> Hello Vijay,
>>>>
>>>> On 23/02/15 13:04, Vijay Kilari wrote:
>>>>> On Thu, Feb 19, 2015 at 7:33 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>>>>> On 19/02/15 07:17, vijay.kilari@gmail.com wrote:
>>>>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>>>>>
>>>>>>> For arm memory for 1024 irq descriptors are allocated
>>>>>>> statically irrespective of number of interrupt supported
>>>>>>> by the platform.
>>>>>>>
>>>>>>> With this patch, irq descriptors are allocated at run time
>>>>>>> and managed using red-black tree. Functions to insert, search
>>>>>>> and delete irq descriptor are implemented in xen/common/irq.c.
>>>>>>
>>>>>> I think we may want to allocate SPIs/SGIs/PPIs at boot time. This number
>>>>>> will never change. We can avoid to always to allocate 1024 IRQs by using
>>>>>> the number provided by the GIC.
>>>>>
>>>>> The irq descriptor is allocated when platform_get_irq() is called or
>>>>> route_irq_to_guest()
>>>>> only. So we are not allocating based on GIC.
>>>>
>>>> You didn't understand what I said... I was suggesting to allocate SPIs
>>>> at boot time. Using an array for them allow us to access to IRQ desc in
>>>> constant time. This may help interrupt latency.
>>>
>>> Yes, I have thought about it. May be we can choose this approach for SPIs
>>> but for LPI's, the GIC can support upto ~32K. So in this case it won't make
>>> sense for LPI's
>>
>> Again... I never said it was a bad idea for LPIs. I was only point out
>> that it may not be worth for SPIs.
>>
>> [..]
>>
>>>> I think we should consider to create a separate structure for LPI's.
>>>
>>> Yes, I have created separate structure for LPI's for ITS driver.
>>>  But as I said LPI's are to many, so cannot allocate at domain creation.
>>
>> Ditto. The new structure can be a radix tree. I never suggested to use
>> an array.
> 
> Thanks for your suggestions. Are u refering to Radix or rb tree?

Radix is better to store sparse array. I haven't really think which tree
structure is better for this use case.

Regards,

-- 
Julien Grall