Re: [Xen-devel] [RFC PATCH v1 06/10] xen/arm: split gic driver into generic and gicv2 driver

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Fri, Mar 21, 2014 at 11:08 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Wed, 2014-03-19 at 19:47 +0530, vijay.kilari@xxxxxxxxx wrote:
>
>> +/* Global state */
>> +static struct {
>> +    int hw_version;
>> +    paddr_t dbase;       /* Address of distributor registers */
>> +    paddr_t cbase;       /* Address of CPU interface registers */
>> +    paddr_t hbase;       /* Address of virtual interface registers */
>> +    paddr_t vbase;       /* Address of virtual cpu interface registers */
>> +    unsigned int lines;  /* Number of interrupts (SPIs + PPIs + SGIs) */
>> +    struct dt_irq maintenance; /* IRQ maintenance */
>> +    unsigned int cpus;
>> +    spinlock_t lock;
>> +} gic;
>> +
> [...]
>> +        spin_lock(&gic.lock);
>> [...]
>> +    spin_lock(&gic.lock);
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index bb718f6..e0859ae 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
> [...]
>> +spinlock_t gic_lock;
>> [...]
>> @@ -151,12 +107,12 @@ static void gic_irq_enable(struct irq_desc *desc)
>>      unsigned long flags;
>>
>>      spin_lock_irqsave(&desc->lock, flags);
>> -    spin_lock(&gic.lock);
>> +    spin_lock(&gic_lock);
>
> You seem to have divided the gic lock into two, is that deliberate?
>
> What is the locking hierarchy between them?
>

>> @@ -286,9 +221,9 @@ static int gic_route_irq(unsigned int irq, bool_t level,
>>
>>      desc->handler = &gic_host_irq_type;
>>
>> -    spin_lock(&gic.lock);
>> +    spin_lock(&gic_lock);
>
> e.g. this function will call through to hook in the gic-v2 code which
> used to require gic.lock to be taken, and accordig to the comment it
> still does after this refactoring. Yet that lock isn't held here any
> more (nor is it accessible to this code).
>

  Yes, the lock in generic code gic.c file is kept as it is.
and gic-v2.c which contains the callbacks will not hold any lock
and assumes that caller is taking care of it.
I made this approach because there is some functions in generic
code in gic. c which require lock even after calling gic-v2 callbacks
So I kept lock with generic code in gic.c

I have add new lock in gic-v2.c file to only syn for the calls that
are not callback functions which are called from gic.c.
Ex: the gic_(hyp/cpu}_init are called directly from notifier.
Only these functions are locked.

>>      gic_set_irq_properties(irq, level, cpu_mask, priority);
>> -    spin_unlock(&gic.lock);
>> +    spin_unlock(&gic_lock);
>>
>>      spin_unlock_irqrestore(&desc->lock, flags);
>>      return 0;
>
> Ian.
>

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