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

[Julien Grall <julien.grall@linaro.org>]

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