| Message ID | 1390581822-32624-4-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2014-01-24 at 16:43 +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> > > --- > Hopefully, none of the supported platforms have UARTs (the only device ^shared? Other than wondering if EBUSY might be more natural than EADDRINUSE and some grammar nits (below) I think this patch looks good. > 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(-) > > 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); "Unable to route IRQ %u..." and I think you want to use d->domain_id rather than hardcoding 0. > + return res; > + } > } > > /* Map the address ranges */ > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 55e7622..d68bde3 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -605,6 +605,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", "is already in use by domain ..." > + irq->irq, d->domain_id); > + return -EADDRINUSE; > + } > + > rc = __setup_irq(desc, irq->irq, new); > spin_unlock_irqrestore(&desc->lock, flags); > > @@ -759,7 +774,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; > > @@ -773,6 +788,29 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > > spin_lock_irqsave(&desc->lock, flags); > > + /* 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); s/the // > + } > + 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; >
Hi Ian, On 02/19/2014 11:35 AM, Ian Campbell wrote: > On Fri, 2014-01-24 at 16:43 +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> >> >> --- >> Hopefully, none of the supported platforms have UARTs (the only device > > ^shared? Hmmm ... I don't remember what I was trying to say here :/. Anyway, this part was for argue to push it for Xen 4.4. It doesn't make sense anymore. I will remove it. > > Other than wondering if EBUSY might be more natural than EADDRINUSE and > some grammar nits (below) I think this patch looks good. Right, I will use EBUSY for the next version. > >> 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(-) >> >> 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); > > "Unable to route IRQ %u..." and I think you want to use d->domain_id > rather than hardcoding 0. I will fix it. At the same time, the error message when Xen is unable to map the range also use "dom0". I will send a separate patch for that. Cheers,
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 55e7622..d68bde3 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -605,6 +605,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); @@ -759,7 +774,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; @@ -773,6 +788,29 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, spin_lock_irqsave(&desc->lock, flags); + /* 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(-)