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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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.


>> 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.

Julien

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