| Message ID | 1396557727-19102-12-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote: $subject "do not allow IRQs to be shared between..." > The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the s/set/sets/; s/no matter if/even if/ > IRQ is correctly setup. "is already correctly setup" (I think that's what you meant) > As IRQ can be shared between devices, if the devices are not assigned to the s/As/An/ ? > same domain or Xen, this could result to route the IRQ to the domain instead of "then this could result in routing the IRQ..." > Xen ... > > Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0. "Also avoid relying on the wrong behaviour ..." perhaps? If that not then I'm not sure what this is referring to. Apart from that doubt the code looks ok to me. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v2: > - Use EBUSY instead of EADDRINUSE for error code > - Don't hardcode the domain in error message > - Fix typo in error message > - Use irq_get_domain > --- > xen/arch/arm/domain_build.c | 9 +++++++-- > xen/arch/arm/irq.c | 34 +++++++++++++++++++++++++++++++++- > 2 files changed, 40 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index e7b1674..f85e5a9 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -718,8 +718,13 @@ 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 */ > - route_dt_irq_to_guest(d, &irq, dt_node_name(dev)); > + res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev)); > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n", > + irq.irq, d->domain_id); > + return res; > + } > } > > /* Map the address ranges */ > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 1262a9c..2bf6618 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -253,6 +253,16 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > > spin_lock_irqsave(&desc->lock, flags); > > + if ( desc->status & IRQ_GUEST ) > + { > + struct domain *d = irq_get_domain(desc); > + > + spin_unlock_irqrestore(&desc->lock, flags); > + printk(XENLOG_ERR "ERROR: IRQ %u is already in use by the domain %u\n", > + irq->irq, d->domain_id); > + return -EBUSY; > + } > + > disabled = (desc->action == NULL); > > rc = __setup_irq(desc, new); > @@ -289,7 +299,7 @@ int route_dt_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; > > action = xmalloc(struct irqaction); > @@ -302,6 +312,28 @@ int route_dt_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 ) > + { > + struct domain *ad = irq_get_domain(desc); > + > + if ( (desc->status & IRQ_GUEST) && d == ad ) > + goto out; > + > + if ( desc->status & IRQ_GUEST ) > + printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain %u\n", > + irq->irq, ad->domain_id); > + else > + printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n", > + irq->irq); > + retval = -EBUSY; > + goto out; > + } > + > retval = __setup_irq(desc, action); > if ( retval ) > {
On 04/07/2014 03:46 PM, Ian Campbell wrote: > On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote: > > $subject "do not allow IRQs to be shared between..." I will change the commit title. >> The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the > > s/set/sets/; s/no matter if/even if/ > >> IRQ is correctly setup. > > "is already correctly setup" (I think that's what you meant) > >> As IRQ can be shared between devices, if the devices are not assigned to the > > s/As/An/ ? I meant "As an ...". But "An" sounds better with the change below. >> same domain or Xen, this could result to route the IRQ to the domain instead of > > "then this could result in routing the IRQ..." >> Xen ... >> >> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0. > > "Also avoid relying on the wrong behaviour ..." perhaps? If that not > then I'm not sure what this is referring to. Apart from that doubt the > code looks ok to me. Currently Xen doesn't check the return of routing the IRQ into DOM0. So, if it fails we silently ignore the error. Regards,
On Mon, 2014-04-07 at 15:53 +0100, Julien Grall wrote: > >> Xen ... > >> > >> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0. > > > > "Also avoid relying on the wrong behaviour ..." perhaps? If that not > > then I'm not sure what this is referring to. Apart from that doubt the > > code looks ok to me. > > Currently Xen doesn't check the return of routing the IRQ into DOM0. So, > if it fails we silently ignore the error. OK, that's what I thought you were talking about then. You might want append to the existing sentence: "Therefore check the return code from XXX in YYYY." to make it clear what it means. Ian.
On 04/07/2014 04:12 PM, Ian Campbell wrote: > On Mon, 2014-04-07 at 15:53 +0100, Julien Grall wrote: >>>> Xen ... >>>> >>>> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0. >>> >>> "Also avoid relying on the wrong behaviour ..." perhaps? If that not >>> then I'm not sure what this is referring to. Apart from that doubt the >>> code looks ok to me. >> >> Currently Xen doesn't check the return of routing the IRQ into DOM0. So, >> if it fails we silently ignore the error. > > OK, that's what I thought you were talking about then. You might want > append to the existing sentence: "Therefore check the return code from > XXX in YYYY." to make it clear what it means. I will update the commit message. Regards,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index e7b1674..f85e5a9 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -718,8 +718,13 @@ 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 */ - route_dt_irq_to_guest(d, &irq, dt_node_name(dev)); + res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev)); + if ( res ) + { + printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n", + irq.irq, d->domain_id); + return res; + } } /* Map the address ranges */ diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 1262a9c..2bf6618 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -253,6 +253,16 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) spin_lock_irqsave(&desc->lock, flags); + if ( desc->status & IRQ_GUEST ) + { + struct domain *d = irq_get_domain(desc); + + spin_unlock_irqrestore(&desc->lock, flags); + printk(XENLOG_ERR "ERROR: IRQ %u is already in use by the domain %u\n", + irq->irq, d->domain_id); + return -EBUSY; + } + disabled = (desc->action == NULL); rc = __setup_irq(desc, new); @@ -289,7 +299,7 @@ int route_dt_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; action = xmalloc(struct irqaction); @@ -302,6 +312,28 @@ int route_dt_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 ) + { + struct domain *ad = irq_get_domain(desc); + + if ( (desc->status & IRQ_GUEST) && d == ad ) + goto out; + + if ( desc->status & IRQ_GUEST ) + printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain %u\n", + irq->irq, ad->domain_id); + else + printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n", + irq->irq); + retval = -EBUSY; + goto out; + } + retval = __setup_irq(desc, action); if ( retval ) {
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 route the IRQ 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> --- Changes in v2: - Use EBUSY instead of EADDRINUSE for error code - Don't hardcode the domain in error message - Fix typo in error message - Use irq_get_domain --- xen/arch/arm/domain_build.c | 9 +++++++-- xen/arch/arm/irq.c | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 3 deletions(-)