Re: [Xen-devel] [PATCH 1/2] xen/arm: save/restore irq flags in gic_disable_cpu

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-04-15 at 17:50 +0100, Julien Grall wrote:
> On SMP board, gic_disable_cpu is called by an SGI call function when Xen is
> halting. In this specific case, the interrupts are disabled.

In the only other existing caller (__cpu_disable) interrupts are also
disabled and I'd argue that it would be invalid to call this function
with interrupts enabled due to its nature. I think it should start with
an assert to that effect.

In principal I think that would make spin_lock() OK to use rather irq or
irqsave? But the lockdep analyser (and/or reality) may not agree? 

Or is the issue here not so much that IRQs are disabled but that this
new caller is in interrupt context?

> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> ---
>  xen/arch/arm/gic.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index aa179a9..c9f64f1 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -419,10 +419,12 @@ void __cpuinit gic_init_secondary_cpu(void)
>  /* Shut down the per-CPU GIC interface */
>  void gic_disable_cpu(void)
>  {
> -    spin_lock_irq(&gic.lock);
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&gic.lock, flags);
>      gic_cpu_disable();

So we apparently have gic_cpu_disable and gic_disable_cpu, nice!

Since the only caller of gic_cpu_disable is gic_disable_cpu I think we
should just inline it. Or call it gic_(cpu_)disable_gicc

>      gic_hyp_disable();

Likewise this is the only caller of this function, either inline or
gic_(cpu)disable_gich?

> -    spin_unlock_irq(&gic.lock);
> +    spin_unlock_irqrestore(&gic.lock, flags);
>  }
>  
>  void gic_route_ppis(void)



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