Re: [Xen-devel] [PATCH v2 10/16] xen/arm: IRQ: Don't need to have a specific function to route IRQ to Xen

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
> When the IRQ is handling by Xen, the setup is done in 2 steps:

"an IRQ is handled" (and perhaps s/, the//)

$subject is an odd way to describe the change too (it's more like the
motivation). Something like "defer routing IRQ to Xen until setup_irq()
call" perhaps?

>     - Route the IRQ to the current CPU and set priorities
>     - Set up the handler
> 
> For PPIs, these steps are called on every cpu. For SPIs, they are only called
> on the boot CPU.
> 
> Dividing the setup in two step complicates the code when a new driver is
> added to Xen (for instance a SMMU driver). Xen can safely route the IRQ
> when the driver sets up the interrupt handler.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> ---
>     Changes in v2:
>         - Fix typo in commit message
>         - s/SGI/SPI/ in comments
>         - Rename gic_route_dt_irq into gic_route_irq_to_xen which is
>         taking a desc now
>         - Call setup_irq before initializing the GIC IRQ as the first one
>         can fail.
> ---
>  xen/arch/arm/gic.c         |   63 
> +++++++-------------------------------------
>  xen/arch/arm/irq.c         |   24 ++++++++++++++++-
>  xen/arch/arm/setup.c       |    2 --
>  xen/arch/arm/smpboot.c     |    2 --
>  xen/arch/arm/time.c        |   11 --------
>  xen/include/asm-arm/gic.h  |   10 +++----
>  xen/include/asm-arm/time.h |    3 ---
>  7 files changed, 36 insertions(+), 79 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 8c53e52..9127ecf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -257,30 +257,20 @@ static void gic_set_irq_properties(unsigned int irq, 
> bool_t level,
>      spin_unlock(&gic.lock);
>  }
>  
> -/* Program the GIC to route an interrupt */
> -static int gic_route_irq(unsigned int irq, bool_t level,
> -                         const cpumask_t *cpu_mask, unsigned int priority)
> +/* Program the GIC to route an interrupt to the host (eg Xen)

You mean i.e. not e.g.

> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 798353b..1262a9c 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -247,15 +247,37 @@ int setup_dt_irq(const struct dt_irq *irq, struct 
> irqaction *new)
>      int rc;
>      unsigned long flags;
>      struct irq_desc *desc;
> +    bool_t disabled = 0;

No need to init, it's unconditionally assigned below. But if you do want
to keep it then I think boot_t wants to go with false even if that is
the same as 0 in the end.

>      desc = irq_to_desc(irq->irq);
>  
>      spin_lock_irqsave(&desc->lock, flags);
> +
> +    disabled = (desc->action == NULL);
> +
>      rc = __setup_irq(desc, new);
> +    if ( rc )
> +        goto err;
>  
> -    if ( !rc )
> +    /* First time the IRQ is setup */
> +    if ( disabled )

There's no way we can get back into this state. Perhaps with calls to
release_irq?

> +    {
> +        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 SPI: we don't care for now which CPU will receive the
> +         * interrupt

setup_dt_irq expected to be called multiple times for a PPI and the desc
is not shared, so that's how they get setup as well, right?

> +         * TODO: Handle case where SPI is setup on different CPU than
> +         * the targeted CPU and the priority.
> +         */
> +        gic_route_irq_to_xen(desc, level, cpumask_of(smp_processor_id()),
> +                             GIC_PRI_IRQ);
>          desc->handler->startup(desc);
> +    }
>  
> +err:
>      spin_unlock_irqrestore(&desc->lock, flags);
>  
>      return rc;

Ian.


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