|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen/arm: Implement dynamic allocation of irq descriptors
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.
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |