Message ID | 20170609174141.5068-6-andre.przywara@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Andre, On 09/06/17 18:41, Andre Przywara wrote: > The function name gic_remove_from_queues() was a bit of a misnomer, > since it just removes an IRQ from the pending queue, not both queues. > Rename the function to make this more clear, also give it a pointer to > a struct pending_irq directly and rely on the VGIC VCPU lock to be > already taken, so this can be used in more places. > Replace the list removal in gic_clear_pending_irqs() with a call to > this function. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic.c | 12 ++++-------- > xen/arch/arm/vgic.c | 5 ++++- > xen/include/asm-arm/gic.h | 2 +- > 3 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index da19130..6c0c9c3 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -400,15 +400,11 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n) > list_add_tail(&n->lr_queue, &v->arch.vgic.lr_pending); > } > > -void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) > +void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p) > { > - struct pending_irq *p = irq_to_pending(v, virtual_irq); > - unsigned long flags; > + ASSERT(spin_is_locked(&v->arch.vgic.lock)); > > - spin_lock_irqsave(&v->arch.vgic.lock, flags); > - if ( !list_empty(&p->lr_queue) ) > - list_del_init(&p->lr_queue); > - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > + list_del_init(&p->lr_queue); > } > > void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq) > @@ -609,7 +605,7 @@ void gic_clear_pending_irqs(struct vcpu *v) > > v->arch.lr_mask = 0; > list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue ) > - list_del_init(&p->lr_queue); > + gic_remove_from_lr_pending(v, p); > } > > int gic_events_need_delivery(void) > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 18fe420..45926ab 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -307,9 +307,12 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n) > while ( (i = find_next_bit(&mask, 32, i)) < 32 ) { > irq = i + (32 * n); > v_target = vgic_get_target_vcpu(v, irq); > + spin_lock_irqsave(&v_target->arch.vgic.lock, flags); Whilst I can understand why you move the lock here (because of #5) this should have required some explanation in the commit message. Indeed, leaving aside patch #5, there are no reason to take the lock for so long. > p = irq_to_pending(v_target, irq); > clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > - gic_remove_from_queues(v_target, irq); > + gic_remove_from_lr_pending(v_target, p); > + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); > + > if ( p->desc != NULL ) > { > spin_lock_irqsave(&p->desc->lock, flags); > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 836a103..3130634 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -243,7 +243,7 @@ extern void init_maintenance_interrupt(void); > extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq, > unsigned int priority); > extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq); > -extern void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq); > +extern void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p); > > /* Accept an interrupt from the GIC and dispatch its handler */ > extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq); > Cheers,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index da19130..6c0c9c3 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -400,15 +400,11 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n) list_add_tail(&n->lr_queue, &v->arch.vgic.lr_pending); } -void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) +void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p) { - struct pending_irq *p = irq_to_pending(v, virtual_irq); - unsigned long flags; + ASSERT(spin_is_locked(&v->arch.vgic.lock)); - spin_lock_irqsave(&v->arch.vgic.lock, flags); - if ( !list_empty(&p->lr_queue) ) - list_del_init(&p->lr_queue); - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); + list_del_init(&p->lr_queue); } void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq) @@ -609,7 +605,7 @@ void gic_clear_pending_irqs(struct vcpu *v) v->arch.lr_mask = 0; list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue ) - list_del_init(&p->lr_queue); + gic_remove_from_lr_pending(v, p); } int gic_events_need_delivery(void) diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 18fe420..45926ab 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -307,9 +307,12 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n) while ( (i = find_next_bit(&mask, 32, i)) < 32 ) { irq = i + (32 * n); v_target = vgic_get_target_vcpu(v, irq); + spin_lock_irqsave(&v_target->arch.vgic.lock, flags); p = irq_to_pending(v_target, irq); clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); - gic_remove_from_queues(v_target, irq); + gic_remove_from_lr_pending(v_target, p); + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); + if ( p->desc != NULL ) { spin_lock_irqsave(&p->desc->lock, flags); diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 836a103..3130634 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -243,7 +243,7 @@ extern void init_maintenance_interrupt(void); extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq, unsigned int priority); extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq); -extern void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq); +extern void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p); /* Accept an interrupt from the GIC and dispatch its handler */ extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
The function name gic_remove_from_queues() was a bit of a misnomer, since it just removes an IRQ from the pending queue, not both queues. Rename the function to make this more clear, also give it a pointer to a struct pending_irq directly and rely on the VGIC VCPU lock to be already taken, so this can be used in more places. Replace the list removal in gic_clear_pending_irqs() with a call to this function. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic.c | 12 ++++-------- xen/arch/arm/vgic.c | 5 ++++- xen/include/asm-arm/gic.h | 2 +- 3 files changed, 9 insertions(+), 10 deletions(-)