|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen/arm: Implement dynamic allocation of irq descriptors
Hi Julien,
On Mon, Feb 23, 2015 at 9:10 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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@xxxxxxxxxx>
>> wrote:
>>> On 19/02/15 07:17, vijay.kilari@xxxxxxxxx wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>>>
>>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |