On Thu, Feb 19, 2015 at 7:33 PM, Julien Grall <julien.gr...@linaro.org> wrote: > On 19/02/15 07:17, vijay.kil...@gmail.com wrote: >> From: Vijaya Kumar K <vijaya.ku...@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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel