Re: [Xen-devel] [PATCH for-4.5 4/8] xen/arm: irq: Don't need to have a specific function to route IRQ to Xen

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> Actually, when the IRQ is handling by Xen, the setup is done in 2 steps:

s/Actually, //

I'd also go with a title like "remove need to have specific..." or
"remove function to route...".

>     - Route the IRQ to the current CPU and set priorities
>     - Set up the handler
> 
> For PPIs, these step are called on every cpus. For SPIs, it's called only

                     ^s                    cpu             they are only called

> on the boot CPU.
> 
> Divided the setup in two step complicated the code when a new driver is

Dividing           into two steps complicates

> added by Xen (for instance a SMMU driver). Xen can safely route the IRQ

       to Xen

> when the driver setup the interrupt handler.

                 sets up

Although for this final para I'm not sure why a new driver is needed --
either it is already complicated or not.

> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> ---
>  xen/arch/arm/gic.c         |   67 
> +++++++++++++++-----------------------------
>  xen/arch/arm/setup.c       |    3 --
>  xen/arch/arm/smpboot.c     |    2 --
>  xen/arch/arm/time.c        |   11 --------
>  xen/include/asm-arm/gic.h  |    7 -----
>  xen/include/asm-arm/time.h |    3 --
>  6 files changed, 23 insertions(+), 70 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index d68bde3..58bcba3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -254,43 +254,25 @@ static void gic_set_irq_properties(unsigned int irq, 
> bool_t level,
>      spin_unlock(&gic.lock);
>  }
>  
> -/* Program the GIC to route an interrupt */
> +/* Program the GIC to route an interrupt to the host (eg Xen)
> + * - needs to be called with desc.lock held

This suggests that the caller must have desc in its hand, but it then
passes irq here so we can look it up again. It may as well pass desc I
think.

>  void __init release_irq(unsigned int irq)
>  {
>      struct irq_desc *desc;
> @@ -601,6 +561,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct 
> irqaction *new)
>      int rc;
>      unsigned long flags;
>      struct irq_desc *desc;
> +    bool_t disabled = 0;
>  
>      desc = irq_to_desc(irq->irq);
>  
> @@ -620,6 +581,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct 
> irqaction *new)
>          return -EADDRINUSE;
>      }
>  
> +    disabled = (desc->action == NULL);
> +
> +    /* First time the IRQ is setup */
> +    if ( disabled )
> +    {
> +        bool_t level;
> +
> +        level = dt_irq_is_level_triggered(irq);
> +        /* It's fine to use smp_processor_id() because:
> +         * For PPI: irq_desc is banked
> +         * For SGI: we don't care for now which CPU will receive the
> +         * interrupt
> +         * TODO: Handle case where SGI is setup on different CPU than
> +         * the targeted CPU and the priority.

Do you mean s/SGI/SPI/ here? SGIs are inherently per CPU, like PPIs.

> +         */
> +        gic_route_irq(irq->irq, level, cpumask_of(smp_processor_id()), 0xa0);
> +    }
> +
>      rc = __setup_irq(desc, irq->irq, new);
>      spin_unlock_irqrestore(&desc->lock, flags);
>  



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