| Message ID | 1372115067-17071-6-git-send-email-julien.grall@citrix.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 25 Jun 2013, Julien Grall wrote: > If the guest VCPU receives an interrupts when it's disabled, it will throw ^interrupt > away the IRQ with EOIed it. What does this mean? I cannot parse the sentence. > This is result to lost IRQ forever. > Directly EOIed the interrupt doesn't help because the IRQ could be fired > again and result to an infinited loop. > > It happens during dom0 boot on the versatile express TC2 with the ethernet > card. > > Let the interrupt disabled when Xen setups the route and enable it when Linux > asks to enable it. Is the problem that Xen keeps the interrupt enabled even when Linux disables it at the gic level? Is this what this patch is trying to address? > Signed-off-by: Julien Grall <julien.grall@citrix.com> > --- > xen/arch/arm/domain_build.c | 14 ++++++++++++++ > xen/arch/arm/gic.c | 25 +++++++++++++++++++++++-- > xen/arch/arm/vgic.c | 7 +++---- > xen/include/asm-arm/gic.h | 4 ++++ > xen/include/asm-arm/irq.h | 6 ++++++ > 5 files changed, 50 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index f5befbd..0470a2d 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct dt_device_node *dev) > } > > DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type); > + > + /* > + * Only map SGI interrupt in the guest as XEN won't handle > + * it correctly. > + * TODO: Fix it > + */ > + if ( !irq_is_sgi(irq.irq) ) > + { > + printk(XENLOG_WARNING "WARNING: Don't map irq %u for %s has " > + "XEN doesn't handle properly non-SGI interrupt\n", > + i, dt_node_full_name(dev)); do you mean SPI? > + continue; > + } > + > /* Don't check return because the IRQ can be use by multiple device */ > gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); > } > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index b16ba8c..e7d082a 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = { > .set_affinity = gic_irq_set_affinity, > }; > > +void gic_irq_enable(struct irq_desc *desc) > +{ > + spin_lock_irq(&desc->lock); > + spin_lock(&gic.lock); > + > + desc->handler->enable(desc); > + > + spin_unlock(&gic.lock); > + spin_unlock_irq(&desc->lock); > +} This function looks a bit too similar to gic_irq_mask and friends, except that it takes two locks. To make that obvious it's probably better to call it gic_irq_enable_safe or gic_irq_enable_locked. > /* needs to be called with gic.lock held */ > static void gic_set_irq_properties(unsigned int irq, bool_t level, > unsigned int cpu_mask, unsigned int priority) > @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irq, > desc->status &= ~IRQ_DISABLED; > dsb(); > > - desc->handler->startup(desc); > - > return 0; > } > > @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > > rc = __setup_irq(desc, irq->irq, new); > > + desc->handler->startup(desc); > + > spin_unlock_irqrestore(&desc->lock, flags); > > return rc; This two changes make it so guest irqs are not enabled by default, good. > @@ -711,6 +722,7 @@ void gic_inject(void) > gic_inject_irq_start(); > } > > +/* TODO: Handle properly non SGI-interrupt */ > int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > const char * devname) > { > @@ -719,11 +731,18 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > unsigned long flags; > int retval; > bool_t level; > + struct pending_irq *p; > + /* XXX: handler other VCPU than 0 */ > + struct vcpu *v = d->vcpu[0]; > > action = xmalloc(struct irqaction); > if (!action) > return -ENOMEM; > > + /* XXX: Here we assume a 1:1 interrupt mapping between the host and dom0 */ > + BUG_ON(irq->irq >= (d->arch.vgic.nr_lines + NR_LOCAL_IRQS)); > + p = irq_to_pending(v, irq->irq); > + > action->dev_id = d; > action->name = devname; > action->free_on_release = 1; > @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > goto out; > } > > + p->desc = desc; > + > out: > spin_unlock(&gic.lock); > spin_unlock_irqrestore(&desc->lock, flags); > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index cea9233..4f3d816 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) > if ( !list_empty(&p->inflight) && !gic_irq_injected(irq) ) > gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); > > + if ( p->desc != NULL ) > + gic_irq_enable(p->desc); > + > i++; > } > } Should we add a gic_irq_disable call when the guest disables irqs? > @@ -685,10 +688,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual) > > n->irq = irq; > n->priority = priority; > - if (!virtual) > - n->desc = irq_to_desc(irq); > - else > - n->desc = NULL; > > /* the irq is enabled */ > if ( rank->ienable & (1 << (irq % 32)) ) I don't quite understand why are you changing where the "desc" assignement is done. If it is a cleanup you shouldn't mix it with a patch like this that is supposed to fix a specific issue. Otherwise please explain why you need this change. > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index f9e9ef1..f7f3c1e 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -134,6 +134,7 @@ > > #ifndef __ASSEMBLY__ > #include <xen/device_tree.h> > +#include <xen/irq.h> > > extern int domain_vgic_init(struct domain *d); > extern void domain_vgic_free(struct domain *d); > @@ -154,6 +155,9 @@ extern void gic_inject(void); > extern void gic_clear_pending_irqs(struct vcpu *v); > extern int gic_events_need_delivery(void); > > +/* Helper to enable an IRQ and take all the needed locks */ > +extern void gic_irq_enable(struct irq_desc *desc); > + > extern void __cpuinit init_maintenance_interrupt(void); > extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq, > unsigned int state, unsigned int priority); > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index 80ff68d..346dc1d 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq, > void *dev_id); > int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new); > > +#define FIRST_SGI_IRQ 32 > +static inline bool_t irq_is_sgi(unsigned int irq) > +{ > + return (irq >= FIRST_SGI_IRQ); > +} Aren't SGIs supposed to be between 0 and 15? Aren't you talking about SPIs here?
On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote: > If the guest VCPU receives an interrupts when it's disabled, it will throw > away the IRQ with EOIed it. Did you mean "without EOIing it" or perhaps "having EOIed it"? > This is result to lost IRQ forever. "This results in losing the IRQ forever". > Directly EOIed the interrupt doesn't help because the IRQ could be fired EOIing in this context. > again and result to an infinited loop. infinite > It happens during dom0 boot on the versatile express TC2 with the ethernet > card. > > Let the interrupt disabled when Xen setups the route and enable it when Linux "Lets ... when Xen sets up the route..." > asks to enable it. > > Signed-off-by: Julien Grall <julien.grall@citrix.com> > --- > xen/arch/arm/domain_build.c | 14 ++++++++++++++ > xen/arch/arm/gic.c | 25 +++++++++++++++++++++++-- > xen/arch/arm/vgic.c | 7 +++---- > xen/include/asm-arm/gic.h | 4 ++++ > xen/include/asm-arm/irq.h | 6 ++++++ > 5 files changed, 50 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index f5befbd..0470a2d 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct dt_device_node *dev) > } > > DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type); > + > + /* > + * Only map SGI interrupt in the guest as XEN won't handle > + * it correctly. > + * TODO: Fix it > + */ > + if ( !irq_is_sgi(irq.irq) ) > + { > + printk(XENLOG_WARNING "WARNING: Don't map irq %u for %s has " s/has/as/ I think. Did you really mean SGI here? I'd have thought from the context that you would mean SPIs. SGIs aren't anything to do with any real devices almost by definition -- if you saw on in the device tree I'd be very surprised! > + "XEN doesn't handle properly non-SGI interrupt\n", > + i, dt_node_full_name(dev)); > + continue; > + } > + > /* Don't check return because the IRQ can be use by multiple device */ > gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); > } > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index b16ba8c..e7d082a 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = { > .set_affinity = gic_irq_set_affinity, > }; > > +void gic_irq_enable(struct irq_desc *desc) > +{ > + spin_lock_irq(&desc->lock); > + spin_lock(&gic.lock); > + > + desc->handler->enable(desc); > + > + spin_unlock(&gic.lock); > + spin_unlock_irq(&desc->lock); > +} > + > /* needs to be called with gic.lock held */ > static void gic_set_irq_properties(unsigned int irq, bool_t level, > unsigned int cpu_mask, unsigned int priority) > @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irq, > desc->status &= ~IRQ_DISABLED; > dsb(); > > - desc->handler->startup(desc); > - > return 0; > } > > @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > > rc = __setup_irq(desc, irq->irq, new); > > + desc->handler->startup(desc); > + > spin_unlock_irqrestore(&desc->lock, flags); > > return rc; > @@ -711,6 +722,7 @@ void gic_inject(void) > gic_inject_irq_start(); > } > > +/* TODO: Handle properly non SGI-interrupt */ > int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > const char * devname) > { > @@ -719,11 +731,18 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > unsigned long flags; > int retval; > bool_t level; > + struct pending_irq *p; > + /* XXX: handler other VCPU than 0 */ That should be something like "XXX: handle VCPUs other than 0". This only matters if we can route SGIs or PPIs to the guest though I think, since they are the only banked interrupts? For SPIs we actually want to actively avoid doing this multiple times, don't we? For the banked interrupts I think we just need a loop here, or for p->desc to not be part of the pending_irq struct but actually part of some separate per-domain datastructure, since it would be very weird to have a domain where the PPIs differed between CPUs. (I'm not sure if that is allowed by the hardware, I bet it is, but it would be a pathological case IMHO...). I think a perdomain irq_desc * array is probably the right answer, unless someone can convincingly argue that PPI routing differing between VCPUs in a guest is a useful thing... > + struct vcpu *v = d->vcpu[0]; > > action = xmalloc(struct irqaction); > if (!action) > return -ENOMEM; > > + /* XXX: Here we assume a 1:1 interrupt mapping between the host and dom0 */ > + BUG_ON(irq->irq >= (d->arch.vgic.nr_lines + NR_LOCAL_IRQS)); > + p = irq_to_pending(v, irq->irq); > + > action->dev_id = d; > action->name = devname; > action->free_on_release = 1; > @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > goto out; > } > > + p->desc = desc; > + > out: > spin_unlock(&gic.lock); > spin_unlock_irqrestore(&desc->lock, flags); > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index cea9233..4f3d816 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) > if ( !list_empty(&p->inflight) && !gic_irq_injected(irq) ) > gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); > > + if ( p->desc != NULL ) > + gic_irq_enable(p->desc); > + > i++; > } > } > @@ -685,10 +688,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual) > > n->irq = irq; > n->priority = priority; > - if (!virtual) > - n->desc = irq_to_desc(irq); > - else > - n->desc = NULL; > > /* the irq is enabled */ > if ( rank->ienable & (1 << (irq % 32)) ) > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index f9e9ef1..f7f3c1e 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -134,6 +134,7 @@ > > #ifndef __ASSEMBLY__ > #include <xen/device_tree.h> > +#include <xen/irq.h> > > extern int domain_vgic_init(struct domain *d); > extern void domain_vgic_free(struct domain *d); > @@ -154,6 +155,9 @@ extern void gic_inject(void); > extern void gic_clear_pending_irqs(struct vcpu *v); > extern int gic_events_need_delivery(void); > > +/* Helper to enable an IRQ and take all the needed locks */ > +extern void gic_irq_enable(struct irq_desc *desc); > + > extern void __cpuinit init_maintenance_interrupt(void); > extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq, > unsigned int state, unsigned int priority); > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index 80ff68d..346dc1d 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq, > void *dev_id); > int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new); > > +#define FIRST_SGI_IRQ 32 > +static inline bool_t irq_is_sgi(unsigned int irq) > +{ > + return (irq >= FIRST_SGI_IRQ); > +} > + > #endif /* _ASM_HW_IRQ_H */ > /* > * Local variables:
On 06/25/2013 05:19 PM, Stefano Stabellini wrote: > On Tue, 25 Jun 2013, Julien Grall wrote: >> If the guest VCPU receives an interrupts when it's disabled, it will throw > ^interrupt > >> away the IRQ with EOIed it. > > What does this mean? I cannot parse the sentence. "it will throw away the IRQ without EOIing". > >> This is result to lost IRQ forever. >> Directly EOIed the interrupt doesn't help because the IRQ could be fired >> again and result to an infinited loop. >> >> It happens during dom0 boot on the versatile express TC2 with the ethernet >> card. >> >> Let the interrupt disabled when Xen setups the route and enable it when Linux >> asks to enable it. > > Is the problem that Xen keeps the interrupt enabled even when Linux > disables it at the gic level? Is this what this patch is trying to > address? This patch only delays the physical IRQ activation until Linux will enable it. This patch avoids to lose IRQ when a VCPU is disabled. I tried to also disable the IRQ when Linux asks to disable it but the versatile express platform hang. > >> Signed-off-by: Julien Grall <julien.grall@citrix.com> >> --- >> xen/arch/arm/domain_build.c | 14 ++++++++++++++ >> xen/arch/arm/gic.c | 25 +++++++++++++++++++++++-- >> xen/arch/arm/vgic.c | 7 +++---- >> xen/include/asm-arm/gic.h | 4 ++++ >> xen/include/asm-arm/irq.h | 6 ++++++ >> 5 files changed, 50 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index f5befbd..0470a2d 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct dt_device_node *dev) >> } >> >> DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type); >> + >> + /* >> + * Only map SGI interrupt in the guest as XEN won't handle >> + * it correctly. >> + * TODO: Fix it >> + */ >> + if ( !irq_is_sgi(irq.irq) ) >> + { >> + printk(XENLOG_WARNING "WARNING: Don't map irq %u for %s has " >> + "XEN doesn't handle properly non-SGI interrupt\n", >> + i, dt_node_full_name(dev)); > > do you mean SPI? > > >> + continue; >> + } >> + >> /* Don't check return because the IRQ can be use by multiple device */ >> gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); >> } >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index b16ba8c..e7d082a 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = { >> .set_affinity = gic_irq_set_affinity, >> }; >> >> +void gic_irq_enable(struct irq_desc *desc) >> +{ >> + spin_lock_irq(&desc->lock); >> + spin_lock(&gic.lock); >> + >> + desc->handler->enable(desc); >> + >> + spin_unlock(&gic.lock); >> + spin_unlock_irq(&desc->lock); >> +} > > This function looks a bit too similar to gic_irq_mask and friends, > except that it takes two locks. > > To make that obvious it's probably better to call it gic_irq_enable_safe > or gic_irq_enable_locked. > > >> /* needs to be called with gic.lock held */ >> static void gic_set_irq_properties(unsigned int irq, bool_t level, >> unsigned int cpu_mask, unsigned int priority) >> @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irq, >> desc->status &= ~IRQ_DISABLED; >> dsb(); >> >> - desc->handler->startup(desc); >> - >> return 0; >> } >> >> @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) >> >> rc = __setup_irq(desc, irq->irq, new); >> >> + desc->handler->startup(desc); >> + >> spin_unlock_irqrestore(&desc->lock, flags); >> >> return rc; > > This two changes make it so guest irqs are not enabled by default, good. > > >> @@ -711,6 +722,7 @@ void gic_inject(void) >> gic_inject_irq_start(); >> } >> >> +/* TODO: Handle properly non SGI-interrupt */ >> int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, >> const char * devname) >> { >> @@ -719,11 +731,18 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, >> unsigned long flags; >> int retval; >> bool_t level; >> + struct pending_irq *p; >> + /* XXX: handler other VCPU than 0 */ >> + struct vcpu *v = d->vcpu[0]; >> >> action = xmalloc(struct irqaction); >> if (!action) >> return -ENOMEM; >> >> + /* XXX: Here we assume a 1:1 interrupt mapping between the host and dom0 */ >> + BUG_ON(irq->irq >= (d->arch.vgic.nr_lines + NR_LOCAL_IRQS)); >> + p = irq_to_pending(v, irq->irq); >> + >> action->dev_id = d; >> action->name = devname; >> action->free_on_release = 1; >> @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, >> goto out; >> } >> >> + p->desc = desc; >> + >> out: >> spin_unlock(&gic.lock); >> spin_unlock_irqrestore(&desc->lock, flags); >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index cea9233..4f3d816 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) >> if ( !list_empty(&p->inflight) && !gic_irq_injected(irq) ) >> gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); >> >> + if ( p->desc != NULL ) >> + gic_irq_enable(p->desc); >> + >> i++; >> } >> } > > Should we add a gic_irq_disable call when the guest disables irqs? > > >> @@ -685,10 +688,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual) >> >> n->irq = irq; >> n->priority = priority; >> - if (!virtual) >> - n->desc = irq_to_desc(irq); >> - else >> - n->desc = NULL; >> >> /* the irq is enabled */ >> if ( rank->ienable & (1 << (irq % 32)) ) > > I don't quite understand why are you changing where the "desc" > assignement is done. > If it is a cleanup you shouldn't mix it with a patch like this that is > supposed to fix a specific issue. Otherwise please explain why you need > this change. > > >> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >> index f9e9ef1..f7f3c1e 100644 >> --- a/xen/include/asm-arm/gic.h >> +++ b/xen/include/asm-arm/gic.h >> @@ -134,6 +134,7 @@ >> >> #ifndef __ASSEMBLY__ >> #include <xen/device_tree.h> >> +#include <xen/irq.h> >> >> extern int domain_vgic_init(struct domain *d); >> extern void domain_vgic_free(struct domain *d); >> @@ -154,6 +155,9 @@ extern void gic_inject(void); >> extern void gic_clear_pending_irqs(struct vcpu *v); >> extern int gic_events_need_delivery(void); >> >> +/* Helper to enable an IRQ and take all the needed locks */ >> +extern void gic_irq_enable(struct irq_desc *desc); >> + >> extern void __cpuinit init_maintenance_interrupt(void); >> extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq, >> unsigned int state, unsigned int priority); >> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h >> index 80ff68d..346dc1d 100644 >> --- a/xen/include/asm-arm/irq.h >> +++ b/xen/include/asm-arm/irq.h >> @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq, >> void *dev_id); >> int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new); >> >> +#define FIRST_SGI_IRQ 32 >> +static inline bool_t irq_is_sgi(unsigned int irq) >> +{ >> + return (irq >= FIRST_SGI_IRQ); >> +} > > Aren't SGIs supposed to be between 0 and 15? Aren't you talking about > SPIs here?
On Tue, 25 Jun 2013, Julien Grall wrote: > On 06/25/2013 05:19 PM, Stefano Stabellini wrote: > > > On Tue, 25 Jun 2013, Julien Grall wrote: > >> If the guest VCPU receives an interrupts when it's disabled, it will throw > > ^interrupt > > > >> away the IRQ with EOIed it. > > > > What does this mean? I cannot parse the sentence. > > "it will throw away the IRQ without EOIing". > > > > >> This is result to lost IRQ forever. > >> Directly EOIed the interrupt doesn't help because the IRQ could be fired > >> again and result to an infinited loop. > >> > >> It happens during dom0 boot on the versatile express TC2 with the ethernet > >> card. > >> > >> Let the interrupt disabled when Xen setups the route and enable it when Linux > >> asks to enable it. > > > > Is the problem that Xen keeps the interrupt enabled even when Linux > > disables it at the gic level? Is this what this patch is trying to > > address? > > This patch only delays the physical IRQ activation until Linux will > enable it. This patch avoids to lose IRQ when a VCPU is disabled. > > I tried to also disable the IRQ when Linux asks to disable it but the > versatile express platform hang. do you know why? and the arndale?
On 06/25/2013 05:28 PM, Ian Campbell wrote: > On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote: >> If the guest VCPU receives an interrupts when it's disabled, it will throw >> away the IRQ with EOIed it. > > Did you mean "without EOIing it" or perhaps "having EOIed it"? > >> This is result to lost IRQ forever. > > "This results in losing the IRQ forever". > >> Directly EOIed the interrupt doesn't help because the IRQ could be fired > > EOIing in this context. > >> again and result to an infinited loop. > > infinite > >> It happens during dom0 boot on the versatile express TC2 with the ethernet >> card. >> >> Let the interrupt disabled when Xen setups the route and enable it when Linux > > "Lets ... when Xen sets up the route..." > >> asks to enable it. >> >> Signed-off-by: Julien Grall <julien.grall@citrix.com> >> --- >> xen/arch/arm/domain_build.c | 14 ++++++++++++++ >> xen/arch/arm/gic.c | 25 +++++++++++++++++++++++-- >> xen/arch/arm/vgic.c | 7 +++---- >> xen/include/asm-arm/gic.h | 4 ++++ >> xen/include/asm-arm/irq.h | 6 ++++++ >> 5 files changed, 50 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index f5befbd..0470a2d 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct dt_device_node *dev) >> } >> >> DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type); >> + >> + /* >> + * Only map SGI interrupt in the guest as XEN won't handle >> + * it correctly. >> + * TODO: Fix it >> + */ >> + if ( !irq_is_sgi(irq.irq) ) >> + { >> + printk(XENLOG_WARNING "WARNING: Don't map irq %u for %s has " > > s/has/as/ I think. > > Did you really mean SGI here? I'd have thought from the context that you > would mean SPIs. SGIs aren't anything to do with any real devices almost > by definition -- if you saw on in the device tree I'd be very surprised! It was SPIs. I will fix it in the next patch series. > >> + "XEN doesn't handle properly non-SGI interrupt\n", >> + i, dt_node_full_name(dev)); >> + continue; >> + } >> + >> /* Don't check return because the IRQ can be use by multiple device */ >> gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); >> } >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index b16ba8c..e7d082a 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = { >> .set_affinity = gic_irq_set_affinity, >> }; >> >> +void gic_irq_enable(struct irq_desc *desc) >> +{ >> + spin_lock_irq(&desc->lock); >> + spin_lock(&gic.lock); >> + >> + desc->handler->enable(desc); >> + >> + spin_unlock(&gic.lock); >> + spin_unlock_irq(&desc->lock); >> +} >> + >> /* needs to be called with gic.lock held */ >> static void gic_set_irq_properties(unsigned int irq, bool_t level, >> unsigned int cpu_mask, unsigned int priority) >> @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irq, >> desc->status &= ~IRQ_DISABLED; >> dsb(); >> >> - desc->handler->startup(desc); >> - >> return 0; >> } >> >> @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) >> >> rc = __setup_irq(desc, irq->irq, new); >> >> + desc->handler->startup(desc); >> + >> spin_unlock_irqrestore(&desc->lock, flags); >> >> return rc; >> @@ -711,6 +722,7 @@ void gic_inject(void) >> gic_inject_irq_start(); >> } >> >> +/* TODO: Handle properly non SGI-interrupt */ >> int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, >> const char * devname) >> { >> @@ -719,11 +731,18 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, >> unsigned long flags; >> int retval; >> bool_t level; >> + struct pending_irq *p; >> + /* XXX: handler other VCPU than 0 */ > > That should be something like "XXX: handle VCPUs other than 0". > > This only matters if we can route SGIs or PPIs to the guest though I > think, since they are the only banked interrupts? For SPIs we actually > want to actively avoid doing this multiple times, don't we? Yes. Here the VCPU is only used to retrieved the struct pending_irq. > > For the banked interrupts I think we just need a loop here, or for > p->desc to not be part of the pending_irq struct but actually part of > some separate per-domain datastructure, since it would be very weird to > have a domain where the PPIs differed between CPUs. (I'm not sure if > that is allowed by the hardware, I bet it is, but it would be a > pathological case IMHO...). > I think a perdomain irq_desc * array is probably the right answer, > unless someone can convincingly argue that PPI routing differing between > VCPUs in a guest is a useful thing... Until now, I didn't see PPIs on other devices than the arch timers and the GIC. I don't know if it's possible, but pending_irq are also banked for PPIs, so it's not an issue. The issue is how do we link the physical PPI to the virtual PPI? Is a 1:1 mapping. How does Xen handle PPI when a it is coming on VCPUs which doesn't handle it (for instance a domU)? >> + struct vcpu *v = d->vcpu[0]; >> >> action = xmalloc(struct irqaction); >> if (!action) >> return -ENOMEM; >> >> + /* XXX: Here we assume a 1:1 interrupt mapping between the host and dom0 */ >> + BUG_ON(irq->irq >= (d->arch.vgic.nr_lines + NR_LOCAL_IRQS)); >> + p = irq_to_pending(v, irq->irq); >> + >> action->dev_id = d; >> action->name = devname; >> action->free_on_release = 1; >> @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, >> goto out; >> } >> >> + p->desc = desc; >> + >> out: >> spin_unlock(&gic.lock); >> spin_unlock_irqrestore(&desc->lock, flags); >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index cea9233..4f3d816 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) >> if ( !list_empty(&p->inflight) && !gic_irq_injected(irq) ) >> gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); >> >> + if ( p->desc != NULL ) >> + gic_irq_enable(p->desc); >> + >> i++; >> } >> } >> @@ -685,10 +688,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual) >> >> n->irq = irq; >> n->priority = priority; >> - if (!virtual) >> - n->desc = irq_to_desc(irq); >> - else >> - n->desc = NULL; >> >> /* the irq is enabled */ >> if ( rank->ienable & (1 << (irq % 32)) ) >> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >> index f9e9ef1..f7f3c1e 100644 >> --- a/xen/include/asm-arm/gic.h >> +++ b/xen/include/asm-arm/gic.h >> @@ -134,6 +134,7 @@ >> >> #ifndef __ASSEMBLY__ >> #include <xen/device_tree.h> >> +#include <xen/irq.h> >> >> extern int domain_vgic_init(struct domain *d); >> extern void domain_vgic_free(struct domain *d); >> @@ -154,6 +155,9 @@ extern void gic_inject(void); >> extern void gic_clear_pending_irqs(struct vcpu *v); >> extern int gic_events_need_delivery(void); >> >> +/* Helper to enable an IRQ and take all the needed locks */ >> +extern void gic_irq_enable(struct irq_desc *desc); >> + >> extern void __cpuinit init_maintenance_interrupt(void); >> extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq, >> unsigned int state, unsigned int priority); >> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h >> index 80ff68d..346dc1d 100644 >> --- a/xen/include/asm-arm/irq.h >> +++ b/xen/include/asm-arm/irq.h >> @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq, >> void *dev_id); >> int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new); >> >> +#define FIRST_SGI_IRQ 32 >> +static inline bool_t irq_is_sgi(unsigned int irq) >> +{ >> + return (irq >= FIRST_SGI_IRQ); >> +} >> + >> #endif /* _ASM_HW_IRQ_H */ >> /* >> * Local variables: > >
On Tue, 25 Jun 2013, Julien Grall wrote: > On 06/25/2013 05:28 PM, Ian Campbell wrote: > > > On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote: > >> If the guest VCPU receives an interrupts when it's disabled, it will throw > >> away the IRQ with EOIed it. > > > > Did you mean "without EOIing it" or perhaps "having EOIed it"? > > > >> This is result to lost IRQ forever. > > > > "This results in losing the IRQ forever". > > > >> Directly EOIed the interrupt doesn't help because the IRQ could be fired > > > > EOIing in this context. > > > >> again and result to an infinited loop. > > > > infinite > > > >> It happens during dom0 boot on the versatile express TC2 with the ethernet > >> card. > >> > >> Let the interrupt disabled when Xen setups the route and enable it when Linux > > > > "Lets ... when Xen sets up the route..." > > > >> asks to enable it. > >> > >> Signed-off-by: Julien Grall <julien.grall@citrix.com> > >> --- > >> xen/arch/arm/domain_build.c | 14 ++++++++++++++ > >> xen/arch/arm/gic.c | 25 +++++++++++++++++++++++-- > >> xen/arch/arm/vgic.c | 7 +++---- > >> xen/include/asm-arm/gic.h | 4 ++++ > >> xen/include/asm-arm/irq.h | 6 ++++++ > >> 5 files changed, 50 insertions(+), 6 deletions(-) > >> > >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > >> index f5befbd..0470a2d 100644 > >> --- a/xen/arch/arm/domain_build.c > >> +++ b/xen/arch/arm/domain_build.c > >> @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct dt_device_node *dev) > >> } > >> > >> DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type); > >> + > >> + /* > >> + * Only map SGI interrupt in the guest as XEN won't handle > >> + * it correctly. > >> + * TODO: Fix it > >> + */ > >> + if ( !irq_is_sgi(irq.irq) ) > >> + { > >> + printk(XENLOG_WARNING "WARNING: Don't map irq %u for %s has " > > > > s/has/as/ I think. > > > > Did you really mean SGI here? I'd have thought from the context that you > > would mean SPIs. SGIs aren't anything to do with any real devices almost > > by definition -- if you saw on in the device tree I'd be very surprised! > > > It was SPIs. I will fix it in the next patch series. > > > > >> + "XEN doesn't handle properly non-SGI interrupt\n", > >> + i, dt_node_full_name(dev)); > >> + continue; > >> + } > >> + > >> /* Don't check return because the IRQ can be use by multiple device */ > >> gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); > >> } > >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > >> index b16ba8c..e7d082a 100644 > >> --- a/xen/arch/arm/gic.c > >> +++ b/xen/arch/arm/gic.c > >> @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = { > >> .set_affinity = gic_irq_set_affinity, > >> }; > >> > >> +void gic_irq_enable(struct irq_desc *desc) > >> +{ > >> + spin_lock_irq(&desc->lock); > >> + spin_lock(&gic.lock); > >> + > >> + desc->handler->enable(desc); > >> + > >> + spin_unlock(&gic.lock); > >> + spin_unlock_irq(&desc->lock); > >> +} > >> + > >> /* needs to be called with gic.lock held */ > >> static void gic_set_irq_properties(unsigned int irq, bool_t level, > >> unsigned int cpu_mask, unsigned int priority) > >> @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irq, > >> desc->status &= ~IRQ_DISABLED; > >> dsb(); > >> > >> - desc->handler->startup(desc); > >> - > >> return 0; > >> } > >> > >> @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > >> > >> rc = __setup_irq(desc, irq->irq, new); > >> > >> + desc->handler->startup(desc); > >> + > >> spin_unlock_irqrestore(&desc->lock, flags); > >> > >> return rc; > >> @@ -711,6 +722,7 @@ void gic_inject(void) > >> gic_inject_irq_start(); > >> } > >> > >> +/* TODO: Handle properly non SGI-interrupt */ > >> int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > >> const char * devname) > >> { > >> @@ -719,11 +731,18 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > >> unsigned long flags; > >> int retval; > >> bool_t level; > >> + struct pending_irq *p; > >> + /* XXX: handler other VCPU than 0 */ > > > > That should be something like "XXX: handle VCPUs other than 0". > > > > This only matters if we can route SGIs or PPIs to the guest though I > > think, since they are the only banked interrupts? For SPIs we actually > > want to actively avoid doing this multiple times, don't we? > > > Yes. Here the VCPU is only used to retrieved the struct pending_irq. > > > > > For the banked interrupts I think we just need a loop here, or for > > p->desc to not be part of the pending_irq struct but actually part of > > some separate per-domain datastructure, since it would be very weird to > > have a domain where the PPIs differed between CPUs. (I'm not sure if > > that is allowed by the hardware, I bet it is, but it would be a > > pathological case IMHO...). > > > I think a perdomain irq_desc * array is probably the right answer, > > unless someone can convincingly argue that PPI routing differing between > > VCPUs in a guest is a useful thing... > > > Until now, I didn't see PPIs on other devices than the arch timers and > the GIC. I don't know if it's possible, but pending_irq are also banked > for PPIs, so it's not an issue. > > The issue is how do we link the physical PPI to the virtual PPI? Is a > 1:1 mapping. How does Xen handle PPI when a it is coming on VCPUs which > doesn't handle it (for instance a domU)? We should be already able to handle this case: vgic_vcpu_inject_irq uses smp_send_event_check_mask to make sure the other physical processor receives the notification and injects the virq.
On Tue, 2013-06-25 at 18:38 +0100, Julien Grall wrote: > >> @@ -719,11 +731,18 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > >> unsigned long flags; > >> int retval; > >> bool_t level; > >> + struct pending_irq *p; > >> + /* XXX: handler other VCPU than 0 */ > > > > That should be something like "XXX: handle VCPUs other than 0". > > > > This only matters if we can route SGIs or PPIs to the guest though I > > think, since they are the only banked interrupts? For SPIs we actually > > want to actively avoid doing this multiple times, don't we? > > > Yes. Here the VCPU is only used to retrieved the struct pending_irq. Which is per-CPU for PPIs and SGIs. Do we not care about PPIs here? > > > > For the banked interrupts I think we just need a loop here, or for > > p->desc to not be part of the pending_irq struct but actually part of > > some separate per-domain datastructure, since it would be very weird to > > have a domain where the PPIs differed between CPUs. (I'm not sure if > > that is allowed by the hardware, I bet it is, but it would be a > > pathological case IMHO...). > > > I think a perdomain irq_desc * array is probably the right answer, > > unless someone can convincingly argue that PPI routing differing between > > VCPUs in a guest is a useful thing... > > > Until now, I didn't see PPIs on other devices than the arch timers and > the GIC. I don't know if it's possible, but pending_irq are also banked > for PPIs, so it's not an issue. > > The issue is how do we link the physical PPI to the virtual PPI? Is a > 1:1 mapping. How does Xen handle PPI when a it is coming on VCPUs which > doesn't handle it (for instance a domU)? How do you mean? Ian.
On 06/26/2013 11:55 AM, Ian Campbell wrote: > On Tue, 2013-06-25 at 18:38 +0100, Julien Grall wrote: >>>> @@ -719,11 +731,18 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, >>>> unsigned long flags; >>>> int retval; >>>> bool_t level; >>>> + struct pending_irq *p; >>>> + /* XXX: handler other VCPU than 0 */ >>> >>> That should be something like "XXX: handle VCPUs other than 0". >>> >>> This only matters if we can route SGIs or PPIs to the guest though I >>> think, since they are the only banked interrupts? For SPIs we actually >>> want to actively avoid doing this multiple times, don't we? >> >> >> Yes. Here the VCPU is only used to retrieved the struct pending_irq. > > Which is per-CPU for PPIs and SGIs. Do we not care about PPIs here? I don't see reason to route physical PPIs. Is it possible to have a device (other than the timer and the GIC) with PPIs? >>> >>> For the banked interrupts I think we just need a loop here, or for >>> p->desc to not be part of the pending_irq struct but actually part of >>> some separate per-domain datastructure, since it would be very weird to >>> have a domain where the PPIs differed between CPUs. (I'm not sure if >>> that is allowed by the hardware, I bet it is, but it would be a >>> pathological case IMHO...). >> >>> I think a perdomain irq_desc * array is probably the right answer, >>> unless someone can convincingly argue that PPI routing differing between >>> VCPUs in a guest is a useful thing... >> >> >> Until now, I didn't see PPIs on other devices than the arch timers and >> the GIC. I don't know if it's possible, but pending_irq are also banked >> for PPIs, so it's not an issue. >> >> The issue is how do we link the physical PPI to the virtual PPI? Is a >> 1:1 mapping. How does Xen handle PPI when a it is coming on VCPUs which >> doesn't handle it (for instance a domU)? > > How do you mean? My sentence wasn't clear. As for the arch timer, which is using PPIs, routing theses interrupts require some code to support the device in Xen, mainly to save/restore the context when the VCPU is moved. Can we assume that Xen will never route PPIs to a guest?
On Tue, 2013-06-25 at 17:19 +0100, Stefano Stabellini wrote: We really need to get something along these lines into 4.4, as a bug fix I think. It seems from the below that the issue which this patch tries to address was actually observed on vexpress, although it doesn't seem likely to be specific to any platform. > > @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > > goto out; > > } > > > > + p->desc = desc; > > + > > out: > > spin_unlock(&gic.lock); > > spin_unlock_irqrestore(&desc->lock, flags); > > @@ -685,10 +688,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual) > > > > n->irq = irq; > > n->priority = priority; > > - if (!virtual) > > - n->desc = irq_to_desc(irq); > > - else > > - n->desc = NULL; > > > > /* the irq is enabled */ > > if ( rank->ienable & (1 << (irq % 32)) ) > > I don't quite understand why are you changing where the "desc" > assignement is done. > If it is a cleanup you shouldn't mix it with a patch like this that is > supposed to fix a specific issue. Otherwise please explain why you need > this change. Was this because gic_router_irq_to_guest has set p->desc as shown above and therefore the feeling was that it wasn't required here? I don't think this can be right though, gic_route_irq_to_guest is only called for SPIs routed via the DTB or the platform specific mapping hook. In particular it is never called for the various PPIs, such as the timer PPI and the evtchn PPI. As it happens the existing PPIs which we pass to the guest are a virtual, and perhaps we can already rely on n->desc==NULL already in that case. TBH, the existing code here is very weird, n->desc should have been set (possibly on all vcpus) at the time the interrupt was configured/routed, it certainly shouldn't be updated on every injection. In practice I think it only happened on the first and was static thereafter so this is just an odd form of lazy initialisation. If it is in fact changing on each injection then I've no idea what this code is doing ;-) Ian.
On Mon, 2 Dec 2013, Ian Campbell wrote: > On Tue, 2013-06-25 at 17:19 +0100, Stefano Stabellini wrote: > > We really need to get something along these lines into 4.4, as a bug fix > I think. It seems from the below that the issue which this patch tries > to address was actually observed on vexpress, although it doesn't seem > likely to be specific to any platform. FYI I am reworking this series using a different approach
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index f5befbd..0470a2d 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct dt_device_node *dev) } DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type); + + /* + * Only map SGI interrupt in the guest as XEN won't handle + * it correctly. + * TODO: Fix it + */ + if ( !irq_is_sgi(irq.irq) ) + { + printk(XENLOG_WARNING "WARNING: Don't map irq %u for %s has " + "XEN doesn't handle properly non-SGI interrupt\n", + i, dt_node_full_name(dev)); + continue; + } + /* Don't check return because the IRQ can be use by multiple device */ gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); } diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index b16ba8c..e7d082a 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = { .set_affinity = gic_irq_set_affinity, }; +void gic_irq_enable(struct irq_desc *desc) +{ + spin_lock_irq(&desc->lock); + spin_lock(&gic.lock); + + desc->handler->enable(desc); + + spin_unlock(&gic.lock); + spin_unlock_irq(&desc->lock); +} + /* needs to be called with gic.lock held */ static void gic_set_irq_properties(unsigned int irq, bool_t level, unsigned int cpu_mask, unsigned int priority) @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irq, desc->status &= ~IRQ_DISABLED; dsb(); - desc->handler->startup(desc); - return 0; } @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) rc = __setup_irq(desc, irq->irq, new); + desc->handler->startup(desc); + spin_unlock_irqrestore(&desc->lock, flags); return rc; @@ -711,6 +722,7 @@ void gic_inject(void) gic_inject_irq_start(); } +/* TODO: Handle properly non SGI-interrupt */ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, const char * devname) { @@ -719,11 +731,18 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, unsigned long flags; int retval; bool_t level; + struct pending_irq *p; + /* XXX: handler other VCPU than 0 */ + struct vcpu *v = d->vcpu[0]; action = xmalloc(struct irqaction); if (!action) return -ENOMEM; + /* XXX: Here we assume a 1:1 interrupt mapping between the host and dom0 */ + BUG_ON(irq->irq >= (d->arch.vgic.nr_lines + NR_LOCAL_IRQS)); + p = irq_to_pending(v, irq->irq); + action->dev_id = d; action->name = devname; action->free_on_release = 1; @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, goto out; } + p->desc = desc; + out: spin_unlock(&gic.lock); spin_unlock_irqrestore(&desc->lock, flags); diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index cea9233..4f3d816 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) if ( !list_empty(&p->inflight) && !gic_irq_injected(irq) ) gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); + if ( p->desc != NULL ) + gic_irq_enable(p->desc); + i++; } } @@ -685,10 +688,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual) n->irq = irq; n->priority = priority; - if (!virtual) - n->desc = irq_to_desc(irq); - else - n->desc = NULL; /* the irq is enabled */ if ( rank->ienable & (1 << (irq % 32)) ) diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index f9e9ef1..f7f3c1e 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -134,6 +134,7 @@ #ifndef __ASSEMBLY__ #include <xen/device_tree.h> +#include <xen/irq.h> extern int domain_vgic_init(struct domain *d); extern void domain_vgic_free(struct domain *d); @@ -154,6 +155,9 @@ extern void gic_inject(void); extern void gic_clear_pending_irqs(struct vcpu *v); extern int gic_events_need_delivery(void); +/* Helper to enable an IRQ and take all the needed locks */ +extern void gic_irq_enable(struct irq_desc *desc); + extern void __cpuinit init_maintenance_interrupt(void); extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq, unsigned int state, unsigned int priority); diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h index 80ff68d..346dc1d 100644 --- a/xen/include/asm-arm/irq.h +++ b/xen/include/asm-arm/irq.h @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq, void *dev_id); int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new); +#define FIRST_SGI_IRQ 32 +static inline bool_t irq_is_sgi(unsigned int irq) +{ + return (irq >= FIRST_SGI_IRQ); +} + #endif /* _ASM_HW_IRQ_H */ /* * Local variables:
If the guest VCPU receives an interrupts when it's disabled, it will throw away the IRQ with EOIed it. This is result to lost IRQ forever. Directly EOIed the interrupt doesn't help because the IRQ could be fired again and result to an infinited loop. It happens during dom0 boot on the versatile express TC2 with the ethernet card. Let the interrupt disabled when Xen setups the route and enable it when Linux asks to enable it. Signed-off-by: Julien Grall <julien.grall@citrix.com> --- xen/arch/arm/domain_build.c | 14 ++++++++++++++ xen/arch/arm/gic.c | 25 +++++++++++++++++++++++-- xen/arch/arm/vgic.c | 7 +++---- xen/include/asm-arm/gic.h | 4 ++++ xen/include/asm-arm/irq.h | 6 ++++++ 5 files changed, 50 insertions(+), 6 deletions(-)