| Message ID | 1389387012-26247-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, 2014-01-10 at 20:50 +0000, Julien Grall wrote: > The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the > IRQ is correctly setup. > > As IRQ can be shared between devices, if the devices are not assigned to the > same domain or Xen, this could result to IRQ route to the domain instead of > Xen ... > > Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> Does this patch relate to or rely on " setup_dt_irq: don't enable the IRQ if the creation has failed" at all? > > --- > Hopefully, none of the supported platforms have UARTs (the only device > currently used by Xen). It would be nice to have this patch for Xen 4.4 to > avoid waste of time for developer. Hrm, at some point I think we have to say no and I think post-rc "nice to avoid waste of time for developer" might be it. After all in a little over a month developers will be using 4.5-pre with this patch applied. What actually happens without this patch? The Xen console UART stops working because the IRQ is delivered to the guest and not to Xen? How did you discover this? Does this happen in practice on any of the platforms which Xen supports? I think in general shared interrupts are reasonably rare on ARM, especially for on-SoC peripherals which the UART very often will be. > The downside of this patch is if someone wants to support a such platform > (eg IRQ shared between device assigned to different domain/XEN), it will > end up to a error message and a panic. I suppose that at least serves as an indication that some actual development work would be required. > --- > xen/arch/arm/domain_build.c | 8 ++++++-- > xen/arch/arm/gic.c | 40 +++++++++++++++++++++++++++++++++++++++- > 2 files changed, 45 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 47b781b..1fc359a 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -712,8 +712,12 @@ 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); > - /* Don't check return because the IRQ can be use by multiple device */ > - gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); > + res = gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to route the IRQ %u to dom0\n", irq.irq); > + return res; > + } > } > > /* Map the address ranges */ > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 62510e3..829d767 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -602,6 +602,21 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > desc = irq_to_desc(irq->irq); > > spin_lock_irqsave(&desc->lock, flags); > + > + if ( desc->status & IRQ_GUEST ) > + { > + struct domain *d; > + > + ASSERT(desc->action != NULL); > + > + d = desc->action->dev_id; > + > + spin_unlock_irqrestore(&desc->lock, flags); > + printk(XENLOG_ERR "ERROR: IRQ %u is already used by the domain %u\n", > + irq->irq, d->domain_id); > + return -EADDRINUSE; > + } > + > rc = __setup_irq(desc, irq->irq, new); > spin_unlock_irqrestore(&desc->lock, flags); > > @@ -756,7 +771,7 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > struct irqaction *action; > struct irq_desc *desc = irq_to_desc(irq->irq); > unsigned long flags; > - int retval; > + int retval = 0; > bool_t level; > struct pending_irq *p; > > @@ -771,6 +786,29 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > spin_lock_irqsave(&desc->lock, flags); > spin_lock(&gic.lock); > > + /* If the IRQ is already used by someone > + * - If it's the same domain -> Xen doesn't need to update the IRQ desc > + * - Otherwise -> For now, don't allow the IRQ to be shared between > + * Xen and domains. > + */ > + if ( desc->action != NULL ) > + { > + if ( (desc->status & IRQ_GUEST) && d == desc->action->dev_id ) > + goto out; > + > + if ( desc->status & IRQ_GUEST ) > + { > + d = desc->action->dev_id; > + printk(XENLOG_ERR "ERROR: IRQ %u is already used by the domain %u\n", > + irq->irq, d->domain_id); > + } > + else > + printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n", > + irq->irq); > + retval = -EADDRINUSE; > + goto out; > + } > + > desc->handler = &gic_guest_irq_type; > desc->status |= IRQ_GUEST; >
On 01/15/2014 01:40 PM, Ian Campbell wrote: > On Fri, 2014-01-10 at 20:50 +0000, Julien Grall wrote: >> The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the >> IRQ is correctly setup. >> >> As IRQ can be shared between devices, if the devices are not assigned to the >> same domain or Xen, this could result to IRQ route to the domain instead of >> Xen ... >> >> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > Does this patch relate to or rely on " setup_dt_irq: don't enable the > IRQ if the creation has failed" at all? There is no relation between the 2 patches. Each one fix a different bug. >> >> --- >> Hopefully, none of the supported platforms have UARTs (the only device >> currently used by Xen). It would be nice to have this patch for Xen 4.4 to >> avoid waste of time for developer. > > Hrm, at some point I think we have to say no and I think post-rc "nice > to avoid waste of time for developer" might be it. After all in a little > over a month developers will be using 4.5-pre with this patch applied. I'm fine to wait after Xen 4.4 release. > What actually happens without this patch? The Xen console UART stops > working because the IRQ is delivered to the guest and not to Xen? Right. > How did you discover this? Does this happen in practice on any of the > platforms which Xen supports? I think in general shared interrupts are > reasonably rare on ARM, especially for on-SoC peripherals which the UART > very often will be. By reading the code, IRQ_GUEST is set unconditionally in dt_route_irq_to_guest. All the current supported platform are safe. --- Julien Grall
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 47b781b..1fc359a 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -712,8 +712,12 @@ 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); - /* Don't check return because the IRQ can be use by multiple device */ - gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); + res = gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); + if ( res ) + { + printk(XENLOG_ERR "Unable to route the IRQ %u to dom0\n", irq.irq); + return res; + } } /* Map the address ranges */ diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 62510e3..829d767 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -602,6 +602,21 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) desc = irq_to_desc(irq->irq); spin_lock_irqsave(&desc->lock, flags); + + if ( desc->status & IRQ_GUEST ) + { + struct domain *d; + + ASSERT(desc->action != NULL); + + d = desc->action->dev_id; + + spin_unlock_irqrestore(&desc->lock, flags); + printk(XENLOG_ERR "ERROR: IRQ %u is already used by the domain %u\n", + irq->irq, d->domain_id); + return -EADDRINUSE; + } + rc = __setup_irq(desc, irq->irq, new); spin_unlock_irqrestore(&desc->lock, flags); @@ -756,7 +771,7 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, struct irqaction *action; struct irq_desc *desc = irq_to_desc(irq->irq); unsigned long flags; - int retval; + int retval = 0; bool_t level; struct pending_irq *p; @@ -771,6 +786,29 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, spin_lock_irqsave(&desc->lock, flags); spin_lock(&gic.lock); + /* If the IRQ is already used by someone + * - If it's the same domain -> Xen doesn't need to update the IRQ desc + * - Otherwise -> For now, don't allow the IRQ to be shared between + * Xen and domains. + */ + if ( desc->action != NULL ) + { + if ( (desc->status & IRQ_GUEST) && d == desc->action->dev_id ) + goto out; + + if ( desc->status & IRQ_GUEST ) + { + d = desc->action->dev_id; + printk(XENLOG_ERR "ERROR: IRQ %u is already used by the domain %u\n", + irq->irq, d->domain_id); + } + else + printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n", + irq->irq); + retval = -EADDRINUSE; + goto out; + } + desc->handler = &gic_guest_irq_type; desc->status |= IRQ_GUEST;
The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the IRQ is correctly setup. As IRQ can be shared between devices, if the devices are not assigned to the same domain or Xen, this could result to IRQ route to the domain instead of Xen ... Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Hopefully, none of the supported platforms have UARTs (the only device currently used by Xen). It would be nice to have this patch for Xen 4.4 to avoid waste of time for developer. The downside of this patch is if someone wants to support a such platform (eg IRQ shared between device assigned to different domain/XEN), it will end up to a error message and a panic. --- xen/arch/arm/domain_build.c | 8 ++++++-- xen/arch/arm/gic.c | 40 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 3 deletions(-)