|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] arm: support fewer LR registers than virtual irqs
On Wed, 2012-02-15 at 14:03 +0000, Stefano Stabellini wrote:
> If the vgic needs to inject a virtual irq into the guest, but no free
> LR registers are available, add the irq to a list and return.
> Whenever an LR register becomes available we add the queued irq to it
> and remove it from the list.
> We use the gic lock to protect the list and the bitmask.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
> xen/arch/arm/gic.c | 61 ++++++++++++++++++++++++++++++++++++++---
> xen/include/asm-arm/domain.h | 1 +
> 2 files changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index adc10bb..520a400 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -25,6 +25,7 @@
> #include <xen/sched.h>
> #include <xen/errno.h>
> #include <xen/softirq.h>
> +#include <xen/list.h>
> #include <asm/p2m.h>
> #include <asm/domain.h>
>
> @@ -45,6 +46,8 @@ static struct {
> unsigned int lines;
> unsigned int cpus;
> spinlock_t lock;
> + uint64_t lr_mask;
Is there somewhere in the code which clamps the number of LRs reported
by the h/w to 64? (and warns appropriately)
This is a mask of in-use LRs, right?
> + struct list_head lr_pending;
> } gic;
>
> irq_desc_t irq_desc[NR_IRQS];
> @@ -247,6 +250,8 @@ static void __cpuinit gic_hyp_init(void)
>
> GICH[GICH_HCR] = GICH_HCR_EN;
> GICH[GICH_MISR] = GICH_MISR_EOI;
> + gic.lr_mask = 0ULL;
> + INIT_LIST_HEAD(&gic.lr_pending);
> }
>
> /* Set up the GIC */
> @@ -345,16 +350,51 @@ int __init setup_irq(unsigned int irq, struct irqaction
> *new)
> return rc;
> }
>
> -void gic_set_guest_irq(unsigned int virtual_irq,
> +static inline void gic_set_lr(int lr, unsigned int virtual_irq,
> unsigned int state, unsigned int priority)
> {
> - BUG_ON(virtual_irq > nr_lrs);
> - GICH[GICH_LR + virtual_irq] = state |
> + BUG_ON(lr > nr_lrs);
> + GICH[GICH_LR + lr] = state |
> GICH_LR_MAINTENANCE_IRQ |
> ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> }
>
> +void gic_set_guest_irq(unsigned int virtual_irq,
> + unsigned int state, unsigned int priority)
> +{
> + int i;
> + struct pending_irq *iter, *n;
> +
> + spin_lock(&gic.lock);
> +
> + if ( list_empty(&gic.lr_pending) )
> + {
> + for (i = 0; i < nr_lrs; i++) {
One of the bitops.h functions should be usable avoid this loop. We don't
seem to have a find-and-set type operation so you still need the lock.
Do we have any per-LR state which we keep? If we did then it be worth
chaining them into a free list, which you could cheaply enqueue and
dequeue entries from.
> + if (!test_and_set_bit(i, &gic.lr_mask))
test_and_set_bit is atomic which is unnecessary since you are also
protecting this field with a lock, or if you use the find_first_zero
idea you could just use __set_bit.
> + {
> + gic_set_lr(i, virtual_irq, state, priority);
> + spin_unlock(&gic.lock);
> + return;
> + }
> + }
> + }
> +
> + n = irq_to_pending(current, virtual_irq);
> + list_for_each_entry ( iter, &gic.lr_pending, lr_link )
> + {
> + if ( iter->priority < priority )
> + {
> + list_add_tail(&n->lr_link, &iter->lr_link);
> + spin_unlock(&gic.lock);
> + return;
> + }
> + }
> + list_add(&n->lr_link, &gic.lr_pending);
> + spin_unlock(&gic.lock);
I'm not sure I follow the logic here -- it seems that only one IRQ will
ever be pending in the LRs at a time, but if we've got 4 LRs wouldn't we
want to have 4 active at once and only trap every 4 Acks?
If there's only going to be one active LR then we can jut always use LR0
and do away with the bitmap for the time being.
Are interrupts only on lr_pending if they are not active in an LR? Are
pending irqs which are in an lr kept in a list somewhere else?
Also it would be nice to have a single exit and unlock path.
> + return;
> +}
> +
> void gic_inject_irq_start(void)
> {
> uint32_t hcr;
> @@ -435,13 +475,25 @@ static void maintenance_interrupt(int irq, void
> *dev_id, struct cpu_user_regs *r
> uint32_t lr;
> uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32);
>
> - for ( i = 0; i < 64; i++ ) {
> + for ( i = 0; i < nr_lrs; i++ ) {
> if ( eisr & ((uint64_t)1 << i) ) {
This loop and test could also use a call to whichever bitops.h thing is
appropriate.
Maybe doesn't matter for loops of the order of 64 iterations but would
be a nice cleanup to make.
> struct pending_irq *p;
>
> + spin_lock(&gic.lock);
> lr = GICH[GICH_LR + i];
> virq = lr & GICH_LR_VIRTUAL_MASK;
> GICH[GICH_LR + i] = 0;
> + clear_bit(i, &gic.lr_mask);
> +
> + if ( !list_empty(gic.lr_pending.next) ) {
> + p = list_entry(gic.lr_pending.next, typeof(*p), lr_link);
> + gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> + list_del_init(&p->lr_link);
> + set_bit(i, &gic.lr_mask);
> + } else {
> + gic_inject_irq_stop();
> + }
> + spin_unlock(&gic.lock);
>
> spin_lock(¤t->arch.vgic.lock);
> p = irq_to_pending(current, virq);
> @@ -449,7 +501,6 @@ static void maintenance_interrupt(int irq, void *dev_id,
> struct cpu_user_regs *r
> p->desc->status &= ~IRQ_INPROGRESS;
> GICC[GICC_DIR] = virq;
> }
> - gic_inject_irq_stop();
> list_del(&p->link);
> INIT_LIST_HEAD(&p->link);
> cpu_raise_softirq(current->processor, VGIC_SOFTIRQ);
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 3372d14..75095ff 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -21,6 +21,7 @@ struct pending_irq
> struct irq_desc *desc; /* only set it the irq corresponds to a physical
> irq */
> uint8_t priority;
> struct list_head link;
> + struct list_head lr_link;
> };
>
> struct arch_domain
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |