| Message ID | 1395232325-19226-3-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 03/19/2014 12:31 PM, Stefano Stabellini wrote: [..] > @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > static inline void gic_set_lr(int lr, struct pending_irq *p, > unsigned int state) > { > - int maintenance_int = GICH_LR_MAINTENANCE_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 | > - ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > + lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > ((p->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); > @@ -669,7 +675,7 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) > spin_unlock_irqrestore(&gic.lock, flags); > } > > -void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq, > +void gic_set_guest_irq(struct vcpu *v, unsigned int irq, Any reason to rename virtual_irq into irq? [..] > +static void gic_clear_lrs(struct vcpu *v) > +{ > + struct pending_irq *p; > + int i = 0, irq; > + uint32_t lr; > + bool_t inflight; > + > + ASSERT(!local_irq_is_enabled()); > + > + while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask), > + nr_lrs, i)) < nr_lrs) { > + lr = GICH[GICH_LR + i]; > + if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) ) > + { > + inflight = 0; > + 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); Not completely related to this patch ... taking gic.lock seems a bit too strong here. The critical section only update data for the current domain. It seems a bit stupid to block the other interrupt to handle their interrupts at the same time. Maybe introducing a dist lock would be a better solution? [..] > void gic_dump_info(struct vcpu *v) > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index aab490c..566f0ff 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) > if ( (irq != current->domain->arch.evtchn_irq) || > (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) ) > set_bit(GIC_IRQ_GUEST_PENDING, &n->status); > - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > - return; > + goto out; We don't want to kick the other VCPU every time. I think it's enough when the interrupt is updated. Regards,
On Wed, 19 Mar 2014, Julien Grall wrote: > Hi Stefano, > > On 03/19/2014 12:31 PM, Stefano Stabellini wrote: > > [..] > > > @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > > static inline void gic_set_lr(int lr, struct pending_irq *p, > > unsigned int state) > > { > > - int maintenance_int = GICH_LR_MAINTENANCE_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 | > > - ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > > + lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > > ((p->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); > > @@ -669,7 +675,7 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) > > spin_unlock_irqrestore(&gic.lock, flags); > > } > > > > -void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq, > > +void gic_set_guest_irq(struct vcpu *v, unsigned int irq, > > Any reason to rename virtual_irq into irq? Just that with this patch we also use this function to inject hw irqs. > [..] > > > +static void gic_clear_lrs(struct vcpu *v) > > +{ > > + struct pending_irq *p; > > + int i = 0, irq; > > + uint32_t lr; > > + bool_t inflight; > > + > > + ASSERT(!local_irq_is_enabled()); > > + > > + while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask), > > + nr_lrs, i)) < nr_lrs) { > > + lr = GICH[GICH_LR + i]; > > + if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) ) > > + { > > + inflight = 0; > > + 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); > > Not completely related to this patch ... taking gic.lock seems a bit too > strong here. The critical section only update data for the current > domain. It seems a bit stupid to block the other interrupt to handle > their interrupts at the same time. > > Maybe introducing a dist lock would be a better solution? It gets removed by a later patch: "don't protect GICH and lr_queue accesses with gic.lock". > [..] > > > void gic_dump_info(struct vcpu *v) > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > > index aab490c..566f0ff 100644 > > --- a/xen/arch/arm/vgic.c > > +++ b/xen/arch/arm/vgic.c > > @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) > > if ( (irq != current->domain->arch.evtchn_irq) || > > (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) ) > > set_bit(GIC_IRQ_GUEST_PENDING, &n->status); > > - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > > - return; > > + goto out; > > We don't want to kick the other VCPU every time. I think it's enough > when the interrupt is updated. Without maintenance interrupts, the other vcpu potentially might never return. We need to send an SGI to it to make sure it gets interrupted. Once it is interrupted, it is going to run gic_clear_lrs that clears the old LR and inject the new interrupt.
Hi Stefano, On 03/19/2014 02:43 PM, Stefano Stabellini wrote: > On Wed, 19 Mar 2014, Julien Grall wrote: >> On 03/19/2014 12:31 PM, Stefano Stabellini wrote: >> >> [..] >> >>> @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) >>> static inline void gic_set_lr(int lr, struct pending_irq *p, >>> unsigned int state) >>> { >>> - int maintenance_int = GICH_LR_MAINTENANCE_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 | >>> - ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | >>> + lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | >>> ((p->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); >>> @@ -669,7 +675,7 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) >>> spin_unlock_irqrestore(&gic.lock, flags); >>> } >>> >>> -void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq, >>> +void gic_set_guest_irq(struct vcpu *v, unsigned int irq, >> >> Any reason to rename virtual_irq into irq? > > Just that with this patch we also use this function to inject hw irqs. As I understand the code, this function still takes a VIRQ. If Xen injects an hw irq, it will before translate it to a VIRQ. By VIRQ I mean the IRQ number from guest POV. >> [..] >> >>> +static void gic_clear_lrs(struct vcpu *v) >>> +{ >>> + struct pending_irq *p; >>> + int i = 0, irq; >>> + uint32_t lr; >>> + bool_t inflight; >>> + >>> + ASSERT(!local_irq_is_enabled()); >>> + >>> + while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask), >>> + nr_lrs, i)) < nr_lrs) { >>> + lr = GICH[GICH_LR + i]; >>> + if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) ) >>> + { >>> + inflight = 0; >>> + 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); >> >> Not completely related to this patch ... taking gic.lock seems a bit too >> strong here. The critical section only update data for the current >> domain. It seems a bit stupid to block the other interrupt to handle >> their interrupts at the same time. >> >> Maybe introducing a dist lock would be a better solution? > > It gets removed by a later patch: "don't protect GICH and lr_queue > accesses with gic.lock". Ok. >> [..] >> >>> void gic_dump_info(struct vcpu *v) >>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>> index aab490c..566f0ff 100644 >>> --- a/xen/arch/arm/vgic.c >>> +++ b/xen/arch/arm/vgic.c >>> @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) >>> if ( (irq != current->domain->arch.evtchn_irq) || >>> (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) ) >>> set_bit(GIC_IRQ_GUEST_PENDING, &n->status); >>> - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); >>> - return; >>> + goto out; >> >> We don't want to kick the other VCPU every time. I think it's enough >> when the interrupt is updated. > > Without maintenance interrupts, the other vcpu potentially might never > return. We need to send an SGI to it to make sure it gets interrupted. > Once it is interrupted, it is going to run gic_clear_lrs that clears the > old LR and inject the new interrupt. I'm not entirely convince, we only need to update when you set GIC_IRQ_GUEST_PENDING in &n->status (e.g in the if sentence). If you don't update the IRQ status, you don't need to kick the VCPUs because either he is already unblock or it will be schedule soon. Anyway this case only happen when we inject the evtchn IRQ. I think it might need some comment as reading only the code is unclear what is done here... Regards,
On Wed, 19 Mar 2014, Julien Grall wrote: > >> [..] > >> > >>> void gic_dump_info(struct vcpu *v) > >>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > >>> index aab490c..566f0ff 100644 > >>> --- a/xen/arch/arm/vgic.c > >>> +++ b/xen/arch/arm/vgic.c > >>> @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) > >>> if ( (irq != current->domain->arch.evtchn_irq) || > >>> (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) ) > >>> set_bit(GIC_IRQ_GUEST_PENDING, &n->status); > >>> - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > >>> - return; > >>> + goto out; > >> > >> We don't want to kick the other VCPU every time. I think it's enough > >> when the interrupt is updated. > > > > Without maintenance interrupts, the other vcpu potentially might never > > return. We need to send an SGI to it to make sure it gets interrupted. > > Once it is interrupted, it is going to run gic_clear_lrs that clears the > > old LR and inject the new interrupt. > > I'm not entirely convince, we only need to update when you set > GIC_IRQ_GUEST_PENDING in &n->status (e.g in the if sentence). If you > don't update the IRQ status, you don't need to kick the VCPUs because > either he is already unblock or it will be schedule soon. > > Anyway this case only happen when we inject the evtchn IRQ. I think it > might need some comment as reading only the code is unclear what is done > here... Reading the code again and your reply I think you might be right, it introduces few unneeded interruptions to the other cpu. However this is one of the bits of code that gets changed by another patch, see "second irq injection while the first irq is still inflight". That patch fixes this issue and more.
On 03/19/2014 03:53 PM, Stefano Stabellini wrote: > However this is one of the bits of code that gets changed by another > patch, see "second irq injection while the first irq is still inflight". > That patch fixes this issue and more. Thanks, I didn't yet look at this patch. Regards,
On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote: > If the irq to be injected is an hardware irq (p->desc != NULL), set > GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ. > > Remove the code to EOI a physical interrupt on behalf of the guest > because it has become unnecessary. Stupid question: there is no possibility of a h/w interrupt which for one reason or another cannot be injected using these GIC facilities? > > Introduce a new function, gic_clear_lrs, that goes over the GICH_LR > registers, clear the invalid ones and free the corresponding interrupts > from the inflight queue if appropriate. Add the interrupt to lr_pending > if the GIC_IRQ_GUEST_PENDING is still set. > > Call gic_clear_lrs from gic_restore_state and on return to guest > (gic_inject). > > In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu, > send and SGI to it to interrupt it and force it to clear the old LRs. s/and/an/ Is the SGI just there to cause it to flush its LRs? Does it not also serve the purpose of causing the pcpu to pick up the potential new interrupt? > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > Changes in v4: > - merged patch #3 and #4 into a single patch. > > 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 | 135 ++++++++++++++++++++++----------------------------- > xen/arch/arm/vgic.c | 3 +- > 2 files changed, 60 insertions(+), 78 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index dbba5d3..32d3bea 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -128,6 +130,7 @@ void gic_restore_state(struct vcpu *v) > GICH[GICH_HCR] = GICH_HCR_EN; > isb(); > > + gic_clear_lrs(v); > gic_restore_pending_irqs(v); Not related to this patch exactly, but is this function badly named? It seems to not actually be restoring anything but is actually looking for any newly pending irqs which it can now inject, is that right? Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which suggests that the clear should be done there (which also seems logical). > } > > @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > static inline void gic_set_lr(int lr, struct pending_irq *p, > unsigned int state) > { > - int maintenance_int = GICH_LR_MAINTENANCE_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 | > - ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > + lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > ((p->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); It seems like it would be a silicon design bug if this were ever true. So BUG_ON(p->desc->irq & ~GICH_LR_PHYSICAL_MASK)? I think this should be checked at the time the interrupt is routed instead of here, which gets it out of this hotter path. Another future cleanup: I think a lot of this register frobbing code would be cleaner if GICH_LR_FOO_MASK was already shifted. [...] > +static void gic_clear_lrs(struct vcpu *v) > +{ > + struct pending_irq *p; > + int i = 0, irq; > + uint32_t lr; > + bool_t inflight; > + > + ASSERT(!local_irq_is_enabled()); > + > + while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask), "long unsigned int" is an odd one isn't it? It seems like all of the other places which do bitops on lr_mask manage to avoid any case at all. > + nr_lrs, i)) < nr_lrs) { > + lr = GICH[GICH_LR + i]; > + if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) ) This state means the lr is "completed" (or inactive, however you want to think about it)? A little helper macro lr_completed(lr) would clarify this without needing a comment. Isn't that condition also true for an LR which was never injected? Won't we then try and complete a non-existent interrupt? Ah, this is protected by the lr_mask isn't it. > + { > + inflight = 0; > + 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); > + if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) && > + test_bit(GIC_IRQ_GUEST_ENABLED, &p->status)) > + { > + inflight = 1; > + gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); > + } > + spin_unlock(&gic.lock); Can an interrupt arrive at this point and make this interrupt "inflight" again, such that the following list remove is actually wrong? > + if ( !inflight ) > + { > + spin_lock(&v->arch.vgic.lock); > + list_del_init(&p->inflight); > + spin_unlock(&v->arch.vgic.lock); > + } > + > + } > + > + i++; > + } > +} > + > static void gic_restore_pending_irqs(struct vcpu *v) > { > int i; > static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) > { [...] Is now empty but not removed? > } > > void gic_dump_info(struct vcpu *v) > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index aab490c..566f0ff 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) > if ( (irq != current->domain->arch.evtchn_irq) || > (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) ) > set_bit(GIC_IRQ_GUEST_PENDING, &n->status); > - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > - return; > + goto out; > } > > /* vcpu offline */
On Fri, 21 Mar 2014, Ian Campbell wrote: > On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote: > > If the irq to be injected is an hardware irq (p->desc != NULL), set > > GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ. > > > > Remove the code to EOI a physical interrupt on behalf of the guest > > because it has become unnecessary. > > Stupid question: there is no possibility of a h/w interrupt which for > one reason or another cannot be injected using these GIC facilities? I don't think so. Nothing is written in spec about such a case. > > Introduce a new function, gic_clear_lrs, that goes over the GICH_LR > > registers, clear the invalid ones and free the corresponding interrupts > > from the inflight queue if appropriate. Add the interrupt to lr_pending > > if the GIC_IRQ_GUEST_PENDING is still set. > > > > Call gic_clear_lrs from gic_restore_state and on return to guest > > (gic_inject). > > > > In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu, > > send and SGI to it to interrupt it and force it to clear the old LRs. > > s/and/an/ > > Is the SGI just there to cause it to flush its LRs? Does it not also > serve the purpose of causing the pcpu to pick up the potential new > interrupt? Yes. Before this patch we were already sending an SGI to the other pcpu so that it would pick up the new IRQ. Now we also send an SGI to the other pcpu even if the IRQ is already inflight, so that the second pcpu can clear the LR corresponding to the previous injection as well as injecting the new interrupt. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > --- > > > > Changes in v4: > > - merged patch #3 and #4 into a single patch. > > > > 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 | 135 ++++++++++++++++++++++----------------------------- > > xen/arch/arm/vgic.c | 3 +- > > 2 files changed, 60 insertions(+), 78 deletions(-) > > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index dbba5d3..32d3bea 100644 > > --- a/xen/arch/arm/gic.c > > +++ b/xen/arch/arm/gic.c > > @@ -128,6 +130,7 @@ void gic_restore_state(struct vcpu *v) > > GICH[GICH_HCR] = GICH_HCR_EN; > > isb(); > > > > + gic_clear_lrs(v); > > gic_restore_pending_irqs(v); > > Not related to this patch exactly, but is this function badly named? It > seems to not actually be restoring anything but is actually looking for > any newly pending irqs which it can now inject, is that right? Yeah, that is correct. > Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which > suggests that the clear should be done there (which also seems logical). It is just temporary: patch "call gic_clear_lrs on entry to the hypervisor" moves the call to gic_clear_lrs to traps.c. > > } > > > > @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > > static inline void gic_set_lr(int lr, struct pending_irq *p, > > unsigned int state) > > { > > - int maintenance_int = GICH_LR_MAINTENANCE_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 | > > - ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > > + lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > > ((p->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); > > It seems like it would be a silicon design bug if this were ever true. > So BUG_ON(p->desc->irq & ~GICH_LR_PHYSICAL_MASK)? Right, I'll remove it. > I think this should be checked at the time the interrupt is routed > instead of here, which gets it out of this hotter path. Actually it cannot happen as desc->irq is initialized by init_irq_data in a for loop up to NR_IRQS (1024). > Another future cleanup: I think a lot of this register frobbing code > would be cleaner if GICH_LR_FOO_MASK was already shifted. > > [...] > > +static void gic_clear_lrs(struct vcpu *v) > > +{ > > + struct pending_irq *p; > > + int i = 0, irq; > > + uint32_t lr; > > + bool_t inflight; > > + > > + ASSERT(!local_irq_is_enabled()); > > + > > + while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask), > > "long unsigned int" is an odd one isn't it? > > It seems like all of the other places which do bitops on lr_mask manage > to avoid any case at all. _find_next_bit_le requires a const unsigned long *p as first argument. I'll make the change to const unsigned long. > > + nr_lrs, i)) < nr_lrs) { > > + lr = GICH[GICH_LR + i]; > > + if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) ) > > This state means the lr is "completed" (or inactive, however you want to > think about it)? A little helper macro lr_completed(lr) would clarify > this without needing a comment. This is another one of those pieces of code that are going to disappear in the following patches. I think the final version is going to look nicer, even without macros. > Isn't that condition also true for an LR which was never injected? Won't > we then try and complete a non-existent interrupt? Ah, this is protected > by the lr_mask isn't it. We are only iterating over lr_mask, that means that we have written to this lr in the recent past. > > + { > > + inflight = 0; > > + 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); > > + if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) && > > + test_bit(GIC_IRQ_GUEST_ENABLED, &p->status)) > > + { > > + inflight = 1; > > + gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); > > + } > > + spin_unlock(&gic.lock); > > Can an interrupt arrive at this point and make this interrupt "inflight" > again, such that the following list remove is actually wrong? gic_clear_lrs is always called with irq disabled, see the ASSERT at the beginning of the function > > + { > > + spin_lock(&v->arch.vgic.lock); > > + list_del_init(&p->inflight); > > + spin_unlock(&v->arch.vgic.lock); > > + } > > + > > + } > > + > > + i++; > > + } > > +} > > + > > static void gic_restore_pending_irqs(struct vcpu *v) > > { > > int i; > > static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) > > { > [...] > > Is now empty but not removed? Yes. We want to keep receiving maintenance interrupts, but we don't need to do anything anymore in the handler because everything we need to do is done on return to guest.
On Fri, 2014-03-21 at 15:55 +0000, Stefano Stabellini wrote: > On Fri, 21 Mar 2014, Ian Campbell wrote: > > On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote: > > > If the irq to be injected is an hardware irq (p->desc != NULL), set > > > GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ. > > > > > > Remove the code to EOI a physical interrupt on behalf of the guest > > > because it has become unnecessary. > > > > Stupid question: there is no possibility of a h/w interrupt which for > > one reason or another cannot be injected using these GIC facilities? > > I don't think so. Nothing is written in spec about such a case. > > > > > Introduce a new function, gic_clear_lrs, that goes over the GICH_LR > > > registers, clear the invalid ones and free the corresponding interrupts > > > from the inflight queue if appropriate. Add the interrupt to lr_pending > > > if the GIC_IRQ_GUEST_PENDING is still set. > > > > > > Call gic_clear_lrs from gic_restore_state and on return to guest > > > (gic_inject). > > > > > > In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu, > > > send and SGI to it to interrupt it and force it to clear the old LRs. > > > > s/and/an/ > > > > Is the SGI just there to cause it to flush its LRs? Does it not also > > serve the purpose of causing the pcpu to pick up the potential new > > interrupt? > > Yes. Before this patch we were already sending an SGI to the other pcpu > so that it would pick up the new IRQ. > Now we also send an SGI to the other pcpu even if the IRQ is already > inflight, so that the second pcpu can clear the LR corresponding to the > previous injection as well as injecting the new interrupt. Can you clarify that in the commit message please? (pretty much inserting what you just said will do). I assume that there is no chance we will send two SGIs? > > Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which > > suggests that the clear should be done there (which also seems logical). > > It is just temporary: patch "call gic_clear_lrs on entry to the > hypervisor" moves the call to gic_clear_lrs to traps.c. I noticed that later, any reason for taking this roundabout path to the final destination? > > > @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > > > static inline void gic_set_lr(int lr, struct pending_irq *p, > > > unsigned int state) > > > { > > > - int maintenance_int = GICH_LR_MAINTENANCE_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 | > > > - ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > > > + lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > > > ((p->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); > > > > It seems like it would be a silicon design bug if this were ever true. > > So BUG_ON(p->desc->irq & ~GICH_LR_PHYSICAL_MASK)? > > Right, I'll remove it. > > > > I think this should be checked at the time the interrupt is routed > > instead of here, which gets it out of this hotter path. > > Actually it cannot happen as desc->irq is initialized by init_irq_data > in a for loop up to NR_IRQS (1024). Excellent. (although beware gic v3 ;-)) > > > + { > > > + inflight = 0; > > > + 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); > > > + if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) && > > > + test_bit(GIC_IRQ_GUEST_ENABLED, &p->status)) > > > + { > > > + inflight = 1; > > > + gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); > > > + } > > > + spin_unlock(&gic.lock); > > > > Can an interrupt arrive at this point and make this interrupt "inflight" > > again, such that the following list remove is actually wrong? > > gic_clear_lrs is always called with irq disabled, see the ASSERT at the > beginning of the function Great. > > > > Is now empty but not removed? > > Yes. We want to keep receiving maintenance interrupts, but we don't need > to do anything anymore in the handler because everything we need to do > is done on return to guest. I figured that out on the next patch -- a comment in any empty interrupt handler would be very useful. Ian.
On Fri, 21 Mar 2014, Ian Campbell wrote: > > > Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which > > > suggests that the clear should be done there (which also seems logical). > > > > It is just temporary: patch "call gic_clear_lrs on entry to the > > hypervisor" moves the call to gic_clear_lrs to traps.c. > > I noticed that later, any reason for taking this roundabout path to the > final destination? I can merge the two patches together if you prefer. I tried to keep all the logical changes separated to allow better debugging and easier reviews. Also it is much easier to merge patches later upon request :-)
On Mon, 2014-03-24 at 12:11 +0000, Stefano Stabellini wrote: > On Fri, 21 Mar 2014, Ian Campbell wrote: > > > > Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which > > > > suggests that the clear should be done there (which also seems logical). > > > > > > It is just temporary: patch "call gic_clear_lrs on entry to the > > > hypervisor" moves the call to gic_clear_lrs to traps.c. > > > > I noticed that later, any reason for taking this roundabout path to the > > final destination? > > I can merge the two patches together if you prefer. > I tried to keep all the logical changes separated to allow better > debugging and easier reviews. Also it is much easier to merge patches > later upon request :-) The flip side is that reviewers now and up looking at some intermediate state which isn't actually useful (and is actually something of a waste of their time). Ian.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index dbba5d3..32d3bea 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -67,6 +67,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id); /* Maximum cpu interface per GIC */ #define NR_GIC_CPU_IF 8 +static void gic_clear_lrs(struct vcpu *v); + static unsigned int gic_cpu_mask(const cpumask_t *cpumask) { unsigned int cpu; @@ -128,6 +130,7 @@ void gic_restore_state(struct vcpu *v) GICH[GICH_HCR] = GICH_HCR_EN; isb(); + gic_clear_lrs(v); gic_restore_pending_irqs(v); } @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) static inline void gic_set_lr(int lr, struct pending_irq *p, unsigned int state) { - int maintenance_int = GICH_LR_MAINTENANCE_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 | - ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | + lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) | ((p->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); @@ -669,7 +675,7 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) spin_unlock_irqrestore(&gic.lock, flags); } -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; @@ -682,18 +688,62 @@ 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, irq_to_pending(v, virtual_irq), state); + gic_set_lr(i, irq_to_pending(v, irq), state); goto out; } } - gic_add_to_lr_pending(v, irq_to_pending(v, virtual_irq)); + gic_add_to_lr_pending(v, irq_to_pending(v, irq)); out: spin_unlock_irqrestore(&gic.lock, flags); return; } +static void gic_clear_lrs(struct vcpu *v) +{ + struct pending_irq *p; + int i = 0, irq; + uint32_t lr; + bool_t inflight; + + ASSERT(!local_irq_is_enabled()); + + while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask), + nr_lrs, i)) < nr_lrs) { + lr = GICH[GICH_LR + i]; + if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) ) + { + inflight = 0; + 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); + if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) && + test_bit(GIC_IRQ_GUEST_ENABLED, &p->status)) + { + inflight = 1; + gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); + } + spin_unlock(&gic.lock); + if ( !inflight ) + { + spin_lock(&v->arch.vgic.lock); + list_del_init(&p->inflight); + spin_unlock(&v->arch.vgic.lock); + } + + } + + i++; + } +} + static void gic_restore_pending_irqs(struct vcpu *v) { int i; @@ -734,6 +784,8 @@ int gic_events_need_delivery(void) void gic_inject(void) { + gic_clear_lrs(current); + if ( vcpu_info(current, evtchn_upcall_pending) ) vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq); @@ -887,77 +939,8 @@ 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; - uint32_t lr; - struct vcpu *v = current; - uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32); - - 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); - lr = GICH[GICH_LR + i]; - virq = lr & GICH_LR_VIRTUAL_MASK; - GICH[GICH_LR + i] = 0; - clear_bit(i, &this_cpu(lr_mask)); - - p = irq_to_pending(v, virq); - 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)) - { - inflight = 1; - gic_add_to_lr_pending(v, p); - } - - clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); - - 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, GICH_LR_PENDING); - list_del_init(&p2->lr_queue); - set_bit(i, &this_cpu(lr_mask)); - } - spin_unlock_irq(&gic.lock); - - if ( !inflight ) - { - spin_lock_irq(&v->arch.vgic.lock); - list_del_init(&p->inflight); - 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++; - } } void gic_dump_info(struct vcpu *v) diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index aab490c..566f0ff 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) if ( (irq != current->domain->arch.evtchn_irq) || (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) ) set_bit(GIC_IRQ_GUEST_PENDING, &n->status); - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); - return; + goto out; } /* vcpu offline */
If the irq to be injected is an hardware irq (p->desc != NULL), set GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ. Remove the code to EOI a physical interrupt on behalf of the guest because it has become unnecessary. Introduce a new function, gic_clear_lrs, that goes over the GICH_LR registers, clear the invalid ones and free the corresponding interrupts from the inflight queue if appropriate. Add the interrupt to lr_pending if the GIC_IRQ_GUEST_PENDING is still set. Call gic_clear_lrs from gic_restore_state and on return to guest (gic_inject). In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu, send and SGI to it to interrupt it and force it to clear the old LRs. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- Changes in v4: - merged patch #3 and #4 into a single patch. 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 | 135 ++++++++++++++++++++++----------------------------- xen/arch/arm/vgic.c | 3 +- 2 files changed, 60 insertions(+), 78 deletions(-)