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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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



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