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

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

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