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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sat, 2014-03-22 at 15:29 +0530, Vijay Kilari wrote:
> 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.

But against what? What is the purpose of this lock if not to enforce a
critical section against the callbacks?

Ian.


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