| Message ID | 1391799378-31664-2-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 07/02/14 18:56, Stefano Stabellini wrote: > If the irq to be injected is an hardware irq (p->desc != NULL), set > GICH_LR_HW. If you set the GICH_LR_HW, I think you should remove the EOI of physical interrupt in maintenance IRQ in this patch. Otherwise we will EOI twice and from the documentation the behavior is unpredicatable. > Also add a struct vcpu* parameter to gic_set_lr. > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > xen/arch/arm/gic.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index acf7195..215b679 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); > > 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 | > - ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > - ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > + if ( p->desc != NULL ) > + GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ | > + ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > + ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) | We should not assume that the physical IRQ == virtual IRQ. You should use p->desc->irq > + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > + else > + GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ | > + ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); The final result of virtual IRQ is a subset of the physical IRQ. Can you factor the code?
On Fri, 7 Feb 2014, Julien Grall wrote: > Hi Stefano, > > On 07/02/14 18:56, Stefano Stabellini wrote: > > If the irq to be injected is an hardware irq (p->desc != NULL), set > > GICH_LR_HW. > > If you set the GICH_LR_HW, I think you should remove the EOI of physical > interrupt in maintenance IRQ in this patch. Otherwise we will EOI twice and > from the documentation the behavior is unpredicatable. Yes, you are right. > > Also add a struct vcpu* parameter to gic_set_lr. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > xen/arch/arm/gic.c | 28 ++++++++++++++++------------ > > 1 file changed, 16 insertions(+), 12 deletions(-) > > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index acf7195..215b679 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); > > > > 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 | > > - ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > > - ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > > + if ( p->desc != NULL ) > > + GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ | > > + ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > > + ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) | > > We should not assume that the physical IRQ == virtual IRQ. You should use > p->desc->irq Right, I'll make the change. > > + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > > + else > > + GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ | > > + ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > > + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > > The final result of virtual IRQ is a subset of the physical IRQ. Can you > factor the code? Yeah
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index acf7195..215b679 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); 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 | - ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | - ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); + if ( p->desc != NULL ) + GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ | + ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | + ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) | + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); + else + GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ | + ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); 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); @@ -950,7 +954,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)); }
If the irq to be injected is an hardware irq (p->desc != NULL), set GICH_LR_HW. Also add a struct vcpu* parameter to gic_set_lr. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- xen/arch/arm/gic.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)