| Message ID | 1402935486-29136-6-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, 16 Jun 2014, Julien Grall wrote: > Xen has to release IRQ routed to a domain in order to reuse later. Currently > only SPIs can be routed to the guest so we only need to browse SPIs for a > specific domain. > > Futhermore, a guest can crash and let the IRQ in an incorrect state (i.e has > not being EOIed). Add a function to reset a given IRQ to allow Xen route again > the IRQ in the future. > > Also, reset the desc->handler to no_irq_type. This will let you know if we > did something wrong with the IRQ management. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/gic.c | 12 ++++++++++++ > xen/arch/arm/irq.c | 8 ++++++++ > xen/arch/arm/vgic.c | 10 ++++++++++ > xen/include/asm-arm/gic.h | 3 +++ > 4 files changed, 33 insertions(+) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 11e53af..42fc3bc 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -928,6 +928,18 @@ int gicv_setup(struct domain *d) > > } > > +/* The guest may not have EOIed the IRQ. > + * Be sure to reset correctly the IRQ. > + */ > +void gic_reset_guest_irq(struct irq_desc *desc) > +{ > + ASSERT(spin_is_locked(&desc->lock)); > + ASSERT(desc->status & IRQ_GUEST); > + > + if ( desc->status & IRQ_INPROGRESS ) > + GICC[GICC_DIR] = desc->irq; > +} You should call gic_update_one_lr first, then check IRQ_INPROGRESS. You should also call gic_remove_from_queues, remove the irq from the inflight queue and clear the GIC_IRQ_GUEST_* status bits. > static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) > { > /* > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 4e51fee..e44a90f 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -274,7 +274,15 @@ void release_irq(unsigned int irq, const void *dev_id) > if ( !desc->action ) > { > desc->handler->shutdown(desc); > + > + if ( desc->status & IRQ_GUEST ) > + { > + gic_reset_guest_irq(desc); > + desc->status &= ~IRQ_INPROGRESS; > + } > + > desc->status &= ~IRQ_GUEST; > + desc->handler = &no_irq_type; > } > > spin_unlock_irqrestore(&desc->lock,flags); > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index cb8df3a..e451324 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -112,6 +112,16 @@ int domain_vgic_init(struct domain *d) > > void domain_vgic_free(struct domain *d) > { > + int i; > + > + for ( i = NR_LOCAL_IRQS; i < d->arch.vgic.nr_lines; i++ ) > + { > + struct irq_desc *desc = d->arch.vgic.pending_irqs[i].desc; > + > + if ( desc ) > + release_irq(desc->irq, d); > + } > + > xfree(d->arch.vgic.shared_irqs); > xfree(d->arch.vgic.pending_irqs); > } > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 6e7375c..841d845 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -228,6 +228,9 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize, > unsigned int *out_hwirq, unsigned int *out_type); > void gic_clear_lrs(struct vcpu *v); > > +/* Reset an IRQ passthrough to a guest */ > +void gic_reset_guest_irq(struct irq_desc *desc); > + > #endif /* __ASSEMBLY__ */ > #endif > > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >
On 18/06/14 19:08, Stefano Stabellini wrote: >> +/* The guest may not have EOIed the IRQ. >> + * Be sure to reset correctly the IRQ. >> + */ >> +void gic_reset_guest_irq(struct irq_desc *desc) >> +{ >> + ASSERT(spin_is_locked(&desc->lock)); >> + ASSERT(desc->status & IRQ_GUEST); >> + >> + if ( desc->status & IRQ_INPROGRESS ) >> + GICC[GICC_DIR] = desc->irq; >> +} > > You should call gic_update_one_lr first, then check IRQ_INPROGRESS. > You should also call gic_remove_from_queues, remove the irq from the > inflight queue and clear the GIC_IRQ_GUEST_* status bits. Are you sure? This function is only called when the domain is dying, so the guest is already unscheduled. Therefore gic_update_one_lr won't work. I can add an ASSERT(irq_get_domain(desc)->is_dying) here... Regards,
On Wed, 18 Jun 2014, Julien Grall wrote: > On 18/06/14 19:08, Stefano Stabellini wrote: > > > +/* The guest may not have EOIed the IRQ. > > > + * Be sure to reset correctly the IRQ. > > > + */ > > > +void gic_reset_guest_irq(struct irq_desc *desc) > > > +{ > > > + ASSERT(spin_is_locked(&desc->lock)); > > > + ASSERT(desc->status & IRQ_GUEST); > > > + > > > + if ( desc->status & IRQ_INPROGRESS ) > > > + GICC[GICC_DIR] = desc->irq; > > > +} > > > > You should call gic_update_one_lr first, then check IRQ_INPROGRESS. > > You should also call gic_remove_from_queues, remove the irq from the > > inflight queue and clear the GIC_IRQ_GUEST_* status bits. > > Are you sure? This function is only called when the domain is dying, so the > guest is already unscheduled. Therefore gic_update_one_lr won't work. > > I can add an ASSERT(irq_get_domain(desc)->is_dying) here... The ASSERT is a good idea. Given that the domain has been descheduled, gic_update_one_lr won't work but you can read the saved lr (pending_irq->lr) from v->arch.gic_lr. You can obtain the target vcpu calling vgic_get_target_vcpu. You only need to write to GICC_DIR if (gic_lr & (GICH_LR_ACTIVE|GICH_LR_PENDING)). gic_remove_from_queues should still work. Also I wonder if you need to call gic_reset_guest_irq before desc->handler->shutdown. The specification states (4.3.5): 'Disabling an interrupt only disables the forwarding of the interrupt from the Distributor to any CPU interface. It does not prevent the interrupt from changing state, for example becoming pending, or active and pending if it is already active.' So from the text above I think that EOIing an interrupt that has been disabled at the GICD level should work, but it is not 100% clear.
On 18/06/14 19:48, Stefano Stabellini wrote: >> I can add an ASSERT(irq_get_domain(desc)->is_dying) here... > > The ASSERT is a good idea. > > Given that the domain has been descheduled, gic_update_one_lr won't work > but you can read the saved lr (pending_irq->lr) from v->arch.gic_lr. > You can obtain the target vcpu calling vgic_get_target_vcpu. > You only need to write to GICC_DIR if (gic_lr & (GICH_LR_ACTIVE|GICH_LR_PENDING)). > gic_remove_from_queues should still work. Unless we assume a virq == pirq we can't retrieve the irq_pending structure via an irq_desc. Also this may has been free earlier, even tho for now SPIs are stored directly in arch_domain. I prefer to keep the check IRQ_INPROGRESS, and call gic_update_lrs before unscheduled the VCPU. BTW, is it the case? > > Also I wonder if you need to call gic_reset_guest_irq before > desc->handler->shutdown. > The specification states (4.3.5): > > 'Disabling an interrupt only disables the forwarding of the interrupt > from the Distributor to any CPU interface. It does not prevent the > interrupt from changing state, for example becoming pending, or active > and pending if it is already active.' > > So from the text above I think that EOIing an interrupt that has been > disabled at the GICD level should work, but it is not 100% clear. On oneshot IRQ, Linux is disabling the IRQ before EOIing it. This will avoid to receive spurious interrupt. Here we need to do the same thing otherwise we may receive spurious interrupt. Regards,
On Wed, 18 Jun 2014, Julien Grall wrote: > On 18/06/14 19:48, Stefano Stabellini wrote: > > > I can add an ASSERT(irq_get_domain(desc)->is_dying) here... > > > > The ASSERT is a good idea. > > > > Given that the domain has been descheduled, gic_update_one_lr won't work > > but you can read the saved lr (pending_irq->lr) from v->arch.gic_lr. > > You can obtain the target vcpu calling vgic_get_target_vcpu. > > You only need to write to GICC_DIR if (gic_lr & > > (GICH_LR_ACTIVE|GICH_LR_PENDING)). > > gic_remove_from_queues should still work. > > Unless we assume a virq == pirq we can't retrieve the irq_pending structure > via an irq_desc. That's annoying. We might have to fix this sooner or later. > Also this may has been free earlier, even tho for now SPIs > are stored directly in arch_domain. > > I prefer to keep the check IRQ_INPROGRESS, and call gic_update_lrs before > unscheduled the VCPU. I was thinking about this alternative, and it is better (see below). > BTW, is it the case? At the moment gic_update_lrs is called on hypervisor entry, so whenever the vcpu gets interrupted, gic_update_lrs is called. If gic_reset_guest_irq is called always after all the guest vcpus have been descheduled, IRQ_INPROGRESS is surely up to date and you don't need to go through the lrs. > > Also I wonder if you need to call gic_reset_guest_irq before > > desc->handler->shutdown. > > The specification states (4.3.5): > > > > 'Disabling an interrupt only disables the forwarding of the interrupt > > from the Distributor to any CPU interface. It does not prevent the > > interrupt from changing state, for example becoming pending, or active > > and pending if it is already active.' > > > > So from the text above I think that EOIing an interrupt that has been > > disabled at the GICD level should work, but it is not 100% clear. > > On oneshot IRQ, Linux is disabling the IRQ before EOIing it. This will avoid > to receive spurious interrupt. > > Here we need to do the same thing otherwise we may receive spurious interrupt. > Good to know
On 18/06/14 20:06, Stefano Stabellini wrote: > At the moment gic_update_lrs is called on hypervisor entry, so whenever > the vcpu gets interrupted, gic_update_lrs is called. If > gic_reset_guest_irq is called always after all the guest vcpus have been > descheduled, IRQ_INPROGRESS is surely up to date and you don't need to > go through the lrs. Thanks. I will add a comment in the code explaining why IRQ_INPROGRESS is valid here.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 11e53af..42fc3bc 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -928,6 +928,18 @@ int gicv_setup(struct domain *d) } +/* The guest may not have EOIed the IRQ. + * Be sure to reset correctly the IRQ. + */ +void gic_reset_guest_irq(struct irq_desc *desc) +{ + ASSERT(spin_is_locked(&desc->lock)); + ASSERT(desc->status & IRQ_GUEST); + + if ( desc->status & IRQ_INPROGRESS ) + GICC[GICC_DIR] = desc->irq; +} + static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) { /* diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 4e51fee..e44a90f 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -274,7 +274,15 @@ void release_irq(unsigned int irq, const void *dev_id) if ( !desc->action ) { desc->handler->shutdown(desc); + + if ( desc->status & IRQ_GUEST ) + { + gic_reset_guest_irq(desc); + desc->status &= ~IRQ_INPROGRESS; + } + desc->status &= ~IRQ_GUEST; + desc->handler = &no_irq_type; } spin_unlock_irqrestore(&desc->lock,flags); diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index cb8df3a..e451324 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -112,6 +112,16 @@ int domain_vgic_init(struct domain *d) void domain_vgic_free(struct domain *d) { + int i; + + for ( i = NR_LOCAL_IRQS; i < d->arch.vgic.nr_lines; i++ ) + { + struct irq_desc *desc = d->arch.vgic.pending_irqs[i].desc; + + if ( desc ) + release_irq(desc->irq, d); + } + xfree(d->arch.vgic.shared_irqs); xfree(d->arch.vgic.pending_irqs); } diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 6e7375c..841d845 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -228,6 +228,9 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize, unsigned int *out_hwirq, unsigned int *out_type); void gic_clear_lrs(struct vcpu *v); +/* Reset an IRQ passthrough to a guest */ +void gic_reset_guest_irq(struct irq_desc *desc); + #endif /* __ASSEMBLY__ */ #endif
Xen has to release IRQ routed to a domain in order to reuse later. Currently only SPIs can be routed to the guest so we only need to browse SPIs for a specific domain. Futhermore, a guest can crash and let the IRQ in an incorrect state (i.e has not being EOIed). Add a function to reset a given IRQ to allow Xen route again the IRQ in the future. Also, reset the desc->handler to no_irq_type. This will let you know if we did something wrong with the IRQ management. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/gic.c | 12 ++++++++++++ xen/arch/arm/irq.c | 8 ++++++++ xen/arch/arm/vgic.c | 10 ++++++++++ xen/include/asm-arm/gic.h | 3 +++ 4 files changed, 33 insertions(+)