Re: [PATCH v3 5/6] xen/arm: Only enable physical IRQs when the guest asks

[Julien Grall <julien.grall@linaro.org>]

On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
> Enable IRQs when the guest requests it, not unconditionally at boot time.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Changes in v2:
> - protect startup and shutdown with gic and desc locks.
> ---
>   xen/arch/arm/gic.c  |   46 ++++++++++++++++++++++++++--------------------
>   xen/arch/arm/vgic.c |    6 ++----
>   2 files changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 5e60c5a..62330dd 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -133,16 +133,26 @@ static void gic_irq_enable(struct irq_desc *desc)
>   {
>       int irq = desc->irq;
>
> +    spin_lock(&desc->lock);
> +    spin_lock(&gic.lock);
>       /* Enable routing */
>       GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
> +    desc->status &= ~IRQ_DISABLED;
> +    spin_unlock(&gic.lock);
> +    spin_unlock(&desc->lock);
>   }

I think we should have something like that:

desc->status &= ...
dsb
GICD[...] = ...

As soon as the interrupt is enabled in the distributor, it can be fired. 
With your solution, another CPU (and even this one because the interrupt 
is not disabled), can receive the interrupt. But it won't work because 
the IRQ is marked as disabled.

So you also should disable interrupt here.

-- 
Julien Grall