| Message ID | 1395232325-19226-9-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote: > GICH is banked, protect accesses by disabling interrupts. > Protect lr_queue accesses with the vgic.lock only. Does this rely on using the irq disabling spinlock_irq variants for this lock to also protect GICH? I don't see any actual calls to irq_disable so I suppose such things are always nested inside holding a vgic lock. > gic.lock only protects accesses to GICD now. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > Changes in v4: > - improved in code comments. > --- > xen/arch/arm/gic.c | 23 +++-------------------- > xen/arch/arm/vgic.c | 9 +++++++-- > xen/include/asm-arm/domain.h | 5 ++++- > 3 files changed, 14 insertions(+), 23 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 128d071..bc9d66d 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -667,19 +667,15 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n) > void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) > { > struct pending_irq *p = irq_to_pending(v, virtual_irq); > - unsigned long flags; > > - spin_lock_irqsave(&gic.lock, flags); > if ( !list_empty(&p->lr_queue) ) > list_del_init(&p->lr_queue); Where is vgic.lock held here? I looked back in the callchain and didn't see it. > - spin_unlock_irqrestore(&gic.lock, flags); > } > > void gic_raise_guest_irq(struct vcpu *v, unsigned int irq, > unsigned int priority) > { > int i; > - unsigned long flags; > struct pending_irq *n = irq_to_pending(v, irq); > > if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) > @@ -689,23 +685,17 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int irq, > return; > } > > - spin_lock_irqsave(&gic.lock, flags); This function requires the vgic lock to be held when it is called. This locking (and implicit interrupt flag based locking) is getting pretty complex. I think it would be a good idea to start documenting this sort of requirement for this code in a comment at the top of the function, and perhaps with an assert in the entry path. Likewise for functions which require interrupts to be disabled with a comment and an assert. Ian.
On Fri, 21 Mar 2014, Ian Campbell wrote: > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote: > > GICH is banked, protect accesses by disabling interrupts. > > Protect lr_queue accesses with the vgic.lock only. > > Does this rely on using the irq disabling spinlock_irq variants for this > lock to also protect GICH? Yes, specifically in gic_set_lr and gic_clear_one_lr. > I don't see any actual calls to irq_disable so I suppose such things are > always nested inside holding a vgic lock. Yes, most of the times. However GICH changes are also made in gic_save_state, gic_restore_state, gic_hyp_init, gic_hyp_disable and gic_inject, where we can be sure that interrupts are disabled for other reasons. > > gic.lock only protects accesses to GICD now. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > --- > > > > Changes in v4: > > - improved in code comments. > > --- > > xen/arch/arm/gic.c | 23 +++-------------------- > > xen/arch/arm/vgic.c | 9 +++++++-- > > xen/include/asm-arm/domain.h | 5 ++++- > > 3 files changed, 14 insertions(+), 23 deletions(-) > > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index 128d071..bc9d66d 100644 > > --- a/xen/arch/arm/gic.c > > +++ b/xen/arch/arm/gic.c > > @@ -667,19 +667,15 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n) > > void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) > > { > > struct pending_irq *p = irq_to_pending(v, virtual_irq); > > - unsigned long flags; > > > > - spin_lock_irqsave(&gic.lock, flags); > > if ( !list_empty(&p->lr_queue) ) > > list_del_init(&p->lr_queue); > > Where is vgic.lock held here? I looked back in the callchain and didn't > see it. Well spotted! This is a mistake! I'll fix it. > > - spin_unlock_irqrestore(&gic.lock, flags); > > } > > > > void gic_raise_guest_irq(struct vcpu *v, unsigned int irq, > > unsigned int priority) > > { > > int i; > > - unsigned long flags; > > struct pending_irq *n = irq_to_pending(v, irq); > > > > if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) > > @@ -689,23 +685,17 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int irq, > > return; > > } > > > > - spin_lock_irqsave(&gic.lock, flags); > > This function requires the vgic lock to be held when it is called. > > This locking (and implicit interrupt flag based locking) is getting > pretty complex. I think it would be a good idea to start documenting > this sort of requirement for this code in a comment at the top of the > function, and perhaps with an assert in the entry path. > > Likewise for functions which require interrupts to be disabled with a > comment and an assert. Good idea, I'll make the changes.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 128d071..bc9d66d 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -667,19 +667,15 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n) void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) { struct pending_irq *p = irq_to_pending(v, virtual_irq); - unsigned long flags; - spin_lock_irqsave(&gic.lock, flags); if ( !list_empty(&p->lr_queue) ) list_del_init(&p->lr_queue); - spin_unlock_irqrestore(&gic.lock, flags); } void gic_raise_guest_irq(struct vcpu *v, unsigned int irq, unsigned int priority) { int i; - unsigned long flags; struct pending_irq *n = irq_to_pending(v, irq); if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) @@ -689,23 +685,17 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int irq, return; } - spin_lock_irqsave(&gic.lock, flags); - if ( v == current && list_empty(&v->arch.vgic.lr_pending) ) { 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, irq_to_pending(v, irq), GICH_LR_PENDING); - goto out; + return; } } gic_add_to_lr_pending(v, irq_to_pending(v, irq)); - -out: - spin_unlock_irqrestore(&gic.lock, flags); - return; } static void _gic_clear_lr(struct vcpu *v, int i) @@ -727,8 +717,6 @@ static void _gic_clear_lr(struct vcpu *v, int i) } else if ( lr & GICH_LR_PENDING ) { clear_bit(GIC_IRQ_GUEST_PENDING, &p->status); } else { - spin_lock(&gic.lock); - GICH[GICH_LR + i] = 0; clear_bit(i, &this_cpu(lr_mask)); @@ -742,8 +730,6 @@ static void _gic_clear_lr(struct vcpu *v, int i) gic_raise_guest_irq(v, irq, p->priority); } else list_del_init(&p->inflight); - - spin_unlock(&gic.lock); } } @@ -773,11 +759,11 @@ static void gic_restore_pending_irqs(struct vcpu *v) i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs); if ( i >= nr_lrs ) return; - spin_lock_irqsave(&gic.lock, flags); + spin_lock_irqsave(&v->arch.vgic.lock, flags); gic_set_lr(i, p, GICH_LR_PENDING); list_del_init(&p->lr_queue); set_bit(i, &this_cpu(lr_mask)); - spin_unlock_irqrestore(&gic.lock, flags); + spin_unlock_irqrestore(&v->arch.vgic.lock, flags); } } @@ -785,13 +771,10 @@ static void gic_restore_pending_irqs(struct vcpu *v) void gic_clear_pending_irqs(struct vcpu *v) { struct pending_irq *p, *t; - unsigned long flags; - spin_lock_irqsave(&gic.lock, flags); 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); - spin_unlock_irqrestore(&gic.lock, flags); } int gic_events_need_delivery(void) diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index dc3a75f..bd15be7 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -393,8 +393,13 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) vcpu_info(current, evtchn_upcall_pending) && list_empty(&p->inflight) ) vgic_vcpu_inject_irq(v, irq); - else if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) - gic_raise_guest_irq(v, irq, p->priority); + else { + unsigned long flags; + spin_lock_irqsave(&v->arch.vgic.lock, flags); + if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) + gic_raise_guest_irq(v, irq, p->priority); + spin_unlock_irqrestore(&v->arch.vgic.lock, flags); + } if ( p->desc != NULL ) p->desc->handler->enable(p->desc); i++; diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index ea89057..517128e 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -66,7 +66,10 @@ struct pending_irq * vgic.inflight_irqs */ struct list_head inflight; /* lr_queue is used to append instances of pending_irq to - * gic.lr_pending */ + * lr_pending. lr_pending is a per vcpu queue, therefore lr_queue + * accesses are protected with the vgic lock. + * TODO: when implementing irq migration, taking only the current + * vgic lock is not going to be enough. */ struct list_head lr_queue; };
GICH is banked, protect accesses by disabling interrupts. Protect lr_queue accesses with the vgic.lock only. gic.lock only protects accesses to GICD now. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- Changes in v4: - improved in code comments. --- xen/arch/arm/gic.c | 23 +++-------------------- xen/arch/arm/vgic.c | 9 +++++++-- xen/include/asm-arm/domain.h | 5 ++++- 3 files changed, 14 insertions(+), 23 deletions(-)