Re: [Xen-devel] [PATCH] xen/arm: IRQ: Protect IRQ to be shared between domains and XEN

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>

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;
>  



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel