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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-04-16 at 11:39 +0100, Julien Grall wrote:
> On 04/16/2013 10:12 AM, Ian Campbell wrote:
> 
> > 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.
> 
> Indeed, I didn't pay attention that interrupts are disabled in
> __cpu_disable. I will use spin_lock and add an assert.

Thanks.

> >> 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?
> 
> 
> If we modify the name, we also need to modify gic_{hyp,cpu,dist}_init.
> I would prefer to let in state or rename all the functions. It's clearer
> to have separate functions (init and disable) for each part of the gic.

Agreed, but only if the names are actually clear, which at least for the
disable case they aren't really. I suppose at least one of them is
static, which mitigates a bit...

Ian



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