| Message ID | 1392393098-7351-2-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
On 02/14/2014 03:51 PM, Stefano Stabellini wrote: > If the irq to be injected is an hardware irq (p->desc != NULL), set > GICH_LR_HW. > > Remove the code to EOI a physical interrupt on behalf of the guest > because it has become unnecessary. > > Also add a struct vcpu* parameter to gic_set_lr. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> IRL you told me that this patch as dependency on another. It would be nice to mention this dependency in the commit message for bisection. > --- > > Changes in v2: > - remove the EOI code, now unnecessary; > - do not assume physical IRQ == virtual IRQ; > - refactor gic_set_lr. > --- > xen/arch/arm/gic.c | 52 +++++++++++++++++----------------------------------- > 1 file changed, 17 insertions(+), 35 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index acf7195..64c8aa7 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -618,20 +618,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > return rc; > } > > -static inline void gic_set_lr(int lr, unsigned int virtual_irq, > +static inline void gic_set_lr(struct vcpu *v, int lr, unsigned int irq, > unsigned int state, unsigned int priority) > { > - int maintenance_int = GICH_LR_MAINTENANCE_IRQ; > - struct pending_irq *p = irq_to_pending(current, virtual_irq); > + struct pending_irq *p = irq_to_pending(v, irq); > + uint32_t lr_reg; > > BUG_ON(lr >= nr_lrs); > BUG_ON(lr < 0); > BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT)); > > - GICH[GICH_LR + lr] = state | > - maintenance_int | > + lr_reg = state | GICH_LR_MAINTENANCE_IRQ | > ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > - ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > + if ( p->desc != NULL ) > + lr_reg |= GICH_LR_HW | > + ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT); > + > + GICH[GICH_LR + lr] = lr_reg; > > set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); > clear_bit(GIC_IRQ_GUEST_PENDING, &p->status); > @@ -666,7 +670,7 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) > spin_unlock(&gic.lock); > } > > -void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq, > +void gic_set_guest_irq(struct vcpu *v, unsigned int irq, > unsigned int state, unsigned int priority) > { > int i; > @@ -679,12 +683,12 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq, > i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs); > if (i < nr_lrs) { > set_bit(i, &this_cpu(lr_mask)); > - gic_set_lr(i, virtual_irq, state, priority); > + gic_set_lr(v, i, irq, state, priority); > goto out; > } > } > > - gic_add_to_lr_pending(v, virtual_irq, priority); > + gic_add_to_lr_pending(v, irq, priority); > > out: > spin_unlock_irqrestore(&gic.lock, flags); > @@ -703,7 +707,7 @@ static void gic_restore_pending_irqs(struct vcpu *v) > if ( i >= nr_lrs ) return; > > spin_lock_irqsave(&gic.lock, flags); > - gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority); > + gic_set_lr(v, i, p->irq, GICH_LR_PENDING, p->priority); > list_del_init(&p->lr_queue); > set_bit(i, &this_cpu(lr_mask)); > spin_unlock_irqrestore(&gic.lock, flags); > @@ -904,15 +908,9 @@ int gicv_setup(struct domain *d) > > } > > -static void gic_irq_eoi(void *info) > -{ > - int virq = (uintptr_t) info; > - GICC[GICC_DIR] = virq; > -} > - > static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) > { > - int i = 0, virq, pirq = -1; > + int i = 0, virq; > uint32_t lr; > struct vcpu *v = current; > uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32); > @@ -920,10 +918,8 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r > while ((i = find_next_bit((const long unsigned int *) &eisr, > 64, i)) < 64) { > struct pending_irq *p, *p2; > - int cpu; > bool_t inflight; > > - cpu = -1; > inflight = 0; > > spin_lock_irq(&gic.lock); > @@ -933,12 +929,8 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r > clear_bit(i, &this_cpu(lr_mask)); > > p = irq_to_pending(v, virq); > - if ( p->desc != NULL ) { > + if ( p->desc != NULL ) > p->desc->status &= ~IRQ_INPROGRESS; > - /* Assume only one pcpu needs to EOI the irq */ > - cpu = p->desc->arch.eoi_cpu; > - pirq = p->desc->irq; > - } > if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) && > test_bit(GIC_IRQ_GUEST_ENABLED, &p->status)) > { > @@ -950,7 +942,7 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r > > if ( !list_empty(&v->arch.vgic.lr_pending) ) { > p2 = list_entry(v->arch.vgic.lr_pending.next, typeof(*p2), lr_queue); > - gic_set_lr(i, p2->irq, GICH_LR_PENDING, p2->priority); > + gic_set_lr(v, i, p2->irq, GICH_LR_PENDING, p2->priority); > list_del_init(&p2->lr_queue); > set_bit(i, &this_cpu(lr_mask)); > } > @@ -963,16 +955,6 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r > spin_unlock_irq(&v->arch.vgic.lock); > } > > - if ( p->desc != NULL ) { > - /* this is not racy because we can't receive another irq of the > - * same type until we EOI it. */ > - if ( cpu == smp_processor_id() ) > - gic_irq_eoi((void*)(uintptr_t)pirq); > - else > - on_selected_cpus(cpumask_of(cpu), > - gic_irq_eoi, (void*)(uintptr_t)pirq, 0); > - } > - > i++; > } > } >
On Fri, 2014-02-14 at 17:49 +0000, Julien Grall wrote: > On 02/14/2014 03:51 PM, Stefano Stabellini wrote: > > If the irq to be injected is an hardware irq (p->desc != NULL), set > > GICH_LR_HW. > > > > Remove the code to EOI a physical interrupt on behalf of the guest > > because it has become unnecessary. > > > > Also add a struct vcpu* parameter to gic_set_lr. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > IRL you told me that this patch as dependency on another. It would be > nice to mention this dependency in the commit message for bisection. As in bisection is broken and needs manual intervention? Please don't do that, reorder it or resplit things to make the issue go away. The patch itself looks ok to me, but I won't ack until this is explained or resolved. Ian.
On Tue, 18 Mar 2014, Ian Campbell wrote: > On Fri, 2014-02-14 at 17:49 +0000, Julien Grall wrote: > > On 02/14/2014 03:51 PM, Stefano Stabellini wrote: > > > If the irq to be injected is an hardware irq (p->desc != NULL), set > > > GICH_LR_HW. > > > > > > Remove the code to EOI a physical interrupt on behalf of the guest > > > because it has become unnecessary. > > > > > > Also add a struct vcpu* parameter to gic_set_lr. > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > IRL you told me that this patch as dependency on another. It would be > > nice to mention this dependency in the commit message for bisection. > > As in bisection is broken and needs manual intervention? Please don't do > that, reorder it or resplit things to make the issue go away. > > The patch itself looks ok to me, but I won't ack until this is explained > or resolved. Patch #2 and #3 should really be applied together, I separated them out just for the sake of readibility. The reason is that you can't receive maintenance interrupts for hw interrupts, so in order for this to work properly we need the following patch "do not request maintenance_interrupts". I can send them as a single patch next time, but it would be harder to review.
On Tue, 2014-03-18 at 17:38 +0000, Stefano Stabellini wrote: > On Tue, 18 Mar 2014, Ian Campbell wrote: > > On Fri, 2014-02-14 at 17:49 +0000, Julien Grall wrote: > > > On 02/14/2014 03:51 PM, Stefano Stabellini wrote: > > > > If the irq to be injected is an hardware irq (p->desc != NULL), set > > > > GICH_LR_HW. > > > > > > > > Remove the code to EOI a physical interrupt on behalf of the guest > > > > because it has become unnecessary. > > > > > > > > Also add a struct vcpu* parameter to gic_set_lr. > > > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > > > IRL you told me that this patch as dependency on another. It would be > > > nice to mention this dependency in the commit message for bisection. > > > > As in bisection is broken and needs manual intervention? Please don't do > > that, reorder it or resplit things to make the issue go away. > > > > The patch itself looks ok to me, but I won't ack until this is explained > > or resolved. > > Patch #2 and #3 should really be applied together, I separated them out > just for the sake of readibility. The reason is that you can't receive > maintenance interrupts for hw interrupts, so in order for this to work > properly we need the following patch "do not request > maintenance_interrupts". > > I can send them as a single patch next time, but it would be harder to > review. I think it'll be ok and/or we'll have to live with it. Ian.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index acf7195..64c8aa7 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -618,20 +618,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) return rc; } -static inline void gic_set_lr(int lr, unsigned int virtual_irq, +static inline void gic_set_lr(struct vcpu *v, int lr, unsigned int irq, unsigned int state, unsigned int priority) { - int maintenance_int = GICH_LR_MAINTENANCE_IRQ; - struct pending_irq *p = irq_to_pending(current, virtual_irq); + struct pending_irq *p = irq_to_pending(v, irq); + uint32_t lr_reg; BUG_ON(lr >= nr_lrs); BUG_ON(lr < 0); BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT)); - GICH[GICH_LR + lr] = state | - maintenance_int | + lr_reg = state | GICH_LR_MAINTENANCE_IRQ | ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | - ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); + if ( p->desc != NULL ) + lr_reg |= GICH_LR_HW | + ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT); + + GICH[GICH_LR + lr] = lr_reg; set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); clear_bit(GIC_IRQ_GUEST_PENDING, &p->status); @@ -666,7 +670,7 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) spin_unlock(&gic.lock); } -void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq, +void gic_set_guest_irq(struct vcpu *v, unsigned int irq, unsigned int state, unsigned int priority) { int i; @@ -679,12 +683,12 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq, i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs); if (i < nr_lrs) { set_bit(i, &this_cpu(lr_mask)); - gic_set_lr(i, virtual_irq, state, priority); + gic_set_lr(v, i, irq, state, priority); goto out; } } - gic_add_to_lr_pending(v, virtual_irq, priority); + gic_add_to_lr_pending(v, irq, priority); out: spin_unlock_irqrestore(&gic.lock, flags); @@ -703,7 +707,7 @@ static void gic_restore_pending_irqs(struct vcpu *v) if ( i >= nr_lrs ) return; spin_lock_irqsave(&gic.lock, flags); - gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority); + gic_set_lr(v, i, p->irq, GICH_LR_PENDING, p->priority); list_del_init(&p->lr_queue); set_bit(i, &this_cpu(lr_mask)); spin_unlock_irqrestore(&gic.lock, flags); @@ -904,15 +908,9 @@ int gicv_setup(struct domain *d) } -static void gic_irq_eoi(void *info) -{ - int virq = (uintptr_t) info; - GICC[GICC_DIR] = virq; -} - static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) { - int i = 0, virq, pirq = -1; + int i = 0, virq; uint32_t lr; struct vcpu *v = current; uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32); @@ -920,10 +918,8 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r while ((i = find_next_bit((const long unsigned int *) &eisr, 64, i)) < 64) { struct pending_irq *p, *p2; - int cpu; bool_t inflight; - cpu = -1; inflight = 0; spin_lock_irq(&gic.lock); @@ -933,12 +929,8 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r clear_bit(i, &this_cpu(lr_mask)); p = irq_to_pending(v, virq); - if ( p->desc != NULL ) { + if ( p->desc != NULL ) p->desc->status &= ~IRQ_INPROGRESS; - /* Assume only one pcpu needs to EOI the irq */ - cpu = p->desc->arch.eoi_cpu; - pirq = p->desc->irq; - } if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) && test_bit(GIC_IRQ_GUEST_ENABLED, &p->status)) { @@ -950,7 +942,7 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r if ( !list_empty(&v->arch.vgic.lr_pending) ) { p2 = list_entry(v->arch.vgic.lr_pending.next, typeof(*p2), lr_queue); - gic_set_lr(i, p2->irq, GICH_LR_PENDING, p2->priority); + gic_set_lr(v, i, p2->irq, GICH_LR_PENDING, p2->priority); list_del_init(&p2->lr_queue); set_bit(i, &this_cpu(lr_mask)); } @@ -963,16 +955,6 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r spin_unlock_irq(&v->arch.vgic.lock); } - if ( p->desc != NULL ) { - /* this is not racy because we can't receive another irq of the - * same type until we EOI it. */ - if ( cpu == smp_processor_id() ) - gic_irq_eoi((void*)(uintptr_t)pirq); - else - on_selected_cpus(cpumask_of(cpu), - gic_irq_eoi, (void*)(uintptr_t)pirq, 0); - } - i++; } }
If the irq to be injected is an hardware irq (p->desc != NULL), set GICH_LR_HW. Remove the code to EOI a physical interrupt on behalf of the guest because it has become unnecessary. Also add a struct vcpu* parameter to gic_set_lr. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- Changes in v2: - remove the EOI code, now unnecessary; - do not assume physical IRQ == virtual IRQ; - refactor gic_set_lr. --- xen/arch/arm/gic.c | 52 +++++++++++++++++----------------------------------- 1 file changed, 17 insertions(+), 35 deletions(-)