| Message ID | 1395686975-12649-8-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote: > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq > while the first one is still active. > If the first irq is already pending (not active), just clear > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is > already visible by the guest. I'm confused by this. If the interrupt is pending but not active then the guest has yet to read GICC_IAR, so while it might be "visible" to the guest it has not been observed by the guest. The comment says: * GIC_IRQ_GUEST_PENDING: the irq is asserted I'm not sure how a second irq arriving would correspond to deasserting the IRQ. Also if you are clearing GIC_IRQ_GUEST_PENDING without clearing the pending bit in the LR that's rather confusing -- I thought the state was supposed to correspond to (the most recently observed) LR state. (having been told that there should be a v6 I'm going to stop here while I figure out where it went...) Ian.
On Tue, 1 Apr 2014, Ian Campbell wrote: > On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote: > > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq > > while the first one is still active. > > If the first irq is already pending (not active), just clear > > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is > > already visible by the guest. > > I'm confused by this. > > If the interrupt is pending but not active then the guest has yet to > read GICC_IAR, so while it might be "visible" to the guest it has not > been observed by the guest. Yes, it is visible to the guest VM but not yet been observed by the guest operating system. > The comment says: > > * GIC_IRQ_GUEST_PENDING: the irq is asserted > > I'm not sure how a second irq arriving would correspond to deasserting > the IRQ. I see where the confusion is coming from. This comment is not quite correct unfortunately. GIC_IRQ_GUEST_PENDING is set when the irq needs to be asserted (by adding it to the LRs). Once the irq has become guest visible, GIC_IRQ_GUEST_PENDING is cleared. Going back to the top of the commit message: "If the first irq is already pending (not active), just clear GIC_IRQ_GUEST_PENDING because the irq has already been injected and is already visible by the guest" means that if the first irq has already been added to the LRs but it is still in pending state, we cannot add it a second time, so just go ahead and clear GIC_IRQ_GUEST_PENDING as if we did add it to the LRs, because the guest is still going to receive a notification. > Also if you are clearing GIC_IRQ_GUEST_PENDING without clearing the > pending bit in the LR that's rather confusing -- I thought the state was > supposed to correspond to (the most recently observed) LR state. Unfortunately not quite: GIC_IRQ_GUEST_PENDING is set by vgic_vcpu_inject_irq and cleared by gic_set_lr and gic_clear_one_lr. Later in this series it can also be set by gic_restore_pending_irqs in case we want to evict an irq from an LR to leave room for an higher priority irq. > (having been told that there should be a v6 I'm going to stop here while > I figure out where it went...) This patch is unchanged in v6.
On Wed, 2014-04-02 at 16:31 +0100, Stefano Stabellini wrote: > On Tue, 1 Apr 2014, Ian Campbell wrote: > > On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote: > > > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq > > > while the first one is still active. > > > If the first irq is already pending (not active), just clear > > > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is > > > already visible by the guest. > > > > I'm confused by this. > > > > If the interrupt is pending but not active then the guest has yet to > > read GICC_IAR, so while it might be "visible" to the guest it has not > > been observed by the guest. > > Yes, it is visible to the guest VM but not yet been observed by the > guest operating system. > > > > The comment says: > > > > * GIC_IRQ_GUEST_PENDING: the irq is asserted > > > > I'm not sure how a second irq arriving would correspond to deasserting > > the IRQ. > > I see where the confusion is coming from. > This comment is not quite correct unfortunately. > > GIC_IRQ_GUEST_PENDING is set when the irq needs to be asserted (by > adding it to the LRs). Once the irq has become guest visible, > GIC_IRQ_GUEST_PENDING is cleared. > > Going back to the top of the commit message: > > "If the first irq is already pending (not active), just clear > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is > already visible by the guest" > > means that if the first irq has already been added to the LRs but it is > still in pending state, we cannot add it a second time, so just go ahead > and clear GIC_IRQ_GUEST_PENDING as if we did add it to the LRs, because > the guest is still going to receive a notification. OK, so when you use lower case pending/active you are talking about the LR state. But when you use upper case GIC_IRQ_GUEST_PENDING this is a completely separate state related to the queueing of interrupts within Xen itself? Can we s/GIC_IRQ_GUEST_PENDING/GIC_IRQ_GUEST_QUEUED/ perhaps? The comment would be something like: * GIC_IRQ_GUEST_QUEUED: the irq is asserted and queued for injection into the guests LRs. > > > > Also if you are clearing GIC_IRQ_GUEST_PENDING without clearing the > > pending bit in the LR that's rather confusing -- I thought the state was > > supposed to correspond to (the most recently observed) LR state. > > Unfortunately not quite: GIC_IRQ_GUEST_PENDING is set by > vgic_vcpu_inject_irq and cleared by gic_set_lr and gic_clear_one_lr. > Later in this series it can also be set by gic_restore_pending_irqs in > case we want to evict an irq from an LR to leave room for an higher > priority irq. > > > > (having been told that there should be a v6 I'm going to stop here while > > I figure out where it went...) > > This patch is unchanged in v6. Thanks. I'll look into v6 ASAP, probably tomorrow AM. Ian.
On Wed, 2 Apr 2014, Ian Campbell wrote: > On Wed, 2014-04-02 at 16:31 +0100, Stefano Stabellini wrote: > > On Tue, 1 Apr 2014, Ian Campbell wrote: > > > On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote: > > > > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq > > > > while the first one is still active. > > > > If the first irq is already pending (not active), just clear > > > > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is > > > > already visible by the guest. > > > > > > I'm confused by this. > > > > > > If the interrupt is pending but not active then the guest has yet to > > > read GICC_IAR, so while it might be "visible" to the guest it has not > > > been observed by the guest. > > > > Yes, it is visible to the guest VM but not yet been observed by the > > guest operating system. > > > > > > > The comment says: > > > > > > * GIC_IRQ_GUEST_PENDING: the irq is asserted > > > > > > I'm not sure how a second irq arriving would correspond to deasserting > > > the IRQ. > > > > I see where the confusion is coming from. > > This comment is not quite correct unfortunately. > > > > GIC_IRQ_GUEST_PENDING is set when the irq needs to be asserted (by > > adding it to the LRs). Once the irq has become guest visible, > > GIC_IRQ_GUEST_PENDING is cleared. > > > > Going back to the top of the commit message: > > > > "If the first irq is already pending (not active), just clear > > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is > > already visible by the guest" > > > > means that if the first irq has already been added to the LRs but it is > > still in pending state, we cannot add it a second time, so just go ahead > > and clear GIC_IRQ_GUEST_PENDING as if we did add it to the LRs, because > > the guest is still going to receive a notification. > > OK, so when you use lower case pending/active you are talking about the > LR state. But when you use upper case GIC_IRQ_GUEST_PENDING this is a > completely separate state related to the queueing of interrupts within > Xen itself? > > Can we s/GIC_IRQ_GUEST_PENDING/GIC_IRQ_GUEST_QUEUED/ perhaps? The > comment would be something like: > * GIC_IRQ_GUEST_QUEUED: the irq is asserted and queued for > injection into the guests LRs. I realize now that the current naming scheme could be confusing. When I talk about pending, active and GICH_LR_PENDING in this commit message, I am talking about the irq status in the LR register. When I talk about GIC_IRQ_GUEST_PENDING, I am referring to the GIC internal state tracker. Because we don't actually know when the guest irq goes from pending to active, we are clearing GIC_IRQ_GUEST_PENDING as soon as we add an irq to an LR, as stated in the comment in xen/include/asm-arm/domain.h. However as a result GIC_IRQ_GUEST_PENDING is more like "GIC_IRQ_GUEST_QUEUED" rather than "GIC_IRQ_GUEST_PENDING", like you wrote. I'll add a patch to do the renaming as suggested.
On Sun, 2014-04-06 at 17:58 +0100, Stefano Stabellini wrote: > I realize now that the current naming scheme could be confusing. When I > talk about pending, active and GICH_LR_PENDING in this commit message, I > am talking about the irq status in the LR register. When I talk about > GIC_IRQ_GUEST_PENDING, I am referring to the GIC internal state tracker. > > Because we don't actually know when the guest irq goes from pending to > active, we are clearing GIC_IRQ_GUEST_PENDING as soon as we add an irq > to an LR, as stated in the comment in xen/include/asm-arm/domain.h. > However as a result GIC_IRQ_GUEST_PENDING is more like > "GIC_IRQ_GUEST_QUEUED" rather than "GIC_IRQ_GUEST_PENDING", like you > wrote. > > I'll add a patch to do the renaming as suggested. Thanks, in that case I guess I won't need the cold towel on my forehead when I think about this stuff ;-) Ian.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 0ebfbf0..77bdfe7 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -680,6 +680,14 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq, { int i; unsigned long flags; + struct pending_irq *n = irq_to_pending(v, virtual_irq); + + if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) + { + if ( v == current ) + gic_clear_one_lr(v, n->lr); + return; + } spin_lock_irqsave(&gic.lock, flags); @@ -705,21 +713,28 @@ static void gic_clear_one_lr(struct vcpu *v, int i) struct pending_irq *p; uint32_t lr; int irq; - bool_t inflight; ASSERT(!local_irq_is_enabled()); ASSERT(spin_is_locked(&v->arch.vgic.lock)); lr = GICH[GICH_LR + i]; - if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) ) + irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK; + p = irq_to_pending(v, irq); + if ( lr & GICH_LR_ACTIVE ) { - inflight = 0; + /* HW interrupts cannot be ACTIVE and PENDING */ + if ( p->desc == NULL && + test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) && + test_and_clear_bit(GIC_IRQ_GUEST_PENDING, &p->status) ) + GICH[GICH_LR + i] = lr | GICH_LR_PENDING; + } 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)); - irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK; - spin_lock(&gic.lock); - p = irq_to_pending(v, irq); if ( p->desc != NULL ) p->desc->status &= ~IRQ_INPROGRESS; clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); @@ -727,16 +742,11 @@ static void gic_clear_one_lr(struct vcpu *v, int i) if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) && test_bit(GIC_IRQ_GUEST_ENABLED, &p->status)) { - inflight = 1; gic_raise_guest_irq(v, irq, p->priority); - } - spin_unlock(&gic.lock); - if ( !inflight ) - { - spin_lock(&v->arch.vgic.lock); + } else list_del_init(&p->inflight); - spin_unlock(&v->arch.vgic.lock); - } + + spin_unlock(&gic.lock); } } @@ -796,9 +806,6 @@ int gic_events_need_delivery(void) void gic_inject(void) { - if ( vcpu_info(current, evtchn_upcall_pending) ) - vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq); - gic_restore_pending_irqs(current); if ( !list_empty(¤t->arch.vgic.lr_pending) && lr_all_full() ) diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 3913cf5..dc3a75f 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -389,7 +389,11 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) irq = i + (32 * n); p = irq_to_pending(v, irq); set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); - if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) + if ( irq == v->domain->arch.evtchn_irq && + 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); if ( p->desc != NULL ) p->desc->handler->enable(p->desc); @@ -696,14 +700,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) spin_lock_irqsave(&v->arch.vgic.lock, flags); - if ( !list_empty(&n->inflight) ) - { - if ( (irq != current->domain->arch.evtchn_irq) || - (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) ) - set_bit(GIC_IRQ_GUEST_PENDING, &n->status); - goto out; - } - /* vcpu offline */ if ( test_bit(_VPF_down, &v->pause_flags) ) { @@ -715,21 +711,26 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) n->irq = irq; set_bit(GIC_IRQ_GUEST_PENDING, &n->status); - n->priority = priority; /* the irq is enabled */ if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) ) gic_raise_guest_irq(v, irq, priority); - list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight ) + if ( list_empty(&n->inflight) ) { - if ( iter->priority > priority ) + n->priority = priority; + list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight ) { - list_add_tail(&n->inflight, &iter->inflight); - goto out; + if ( iter->priority > priority ) + { + list_add_tail(&n->inflight, &iter->inflight); + goto out; + } } - } - list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs); + list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs); + } else if ( n->priority != priority ) + gdprintk(XENLOG_WARNING, "Changing priority of an inflight interrupt is not supported"); + out: spin_unlock_irqrestore(&v->arch.vgic.lock, flags); /* we have a new higher priority irq, inject it into the guest */
Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq while the first one is still active. If the first irq is already pending (not active), just clear GIC_IRQ_GUEST_PENDING because the irq has already been injected and is already visible by the guest. If the irq has already been EOI'ed then just clear the GICH_LR right away and move the interrupt to lr_pending so that it is going to be reinjected by gic_restore_pending_irqs on return to guest. If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_PENDING and send an SGI. The target cpu is going to be interrupted and call gic_clear_lrs, that is going to take the same actions. Unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq. Do not call vgic_vcpu_inject_irq from gic_inject if evtchn_upcall_pending is set. If we remove that call, we don't need to special case evtchn_irq in vgic_vcpu_inject_irq anymore. We also need to force the first injection of evtchn_irq (call gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending is already set by common code on vcpu creation. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- Changes in v3: - do not use the PENDING and ACTIVE state for HW interrupts; - unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq. --- xen/arch/arm/gic.c | 41 ++++++++++++++++++++++++----------------- xen/arch/arm/vgic.c | 33 +++++++++++++++++---------------- 2 files changed, 41 insertions(+), 33 deletions(-)