Re: [Xen-devel] [PATCH v5 04/10] xen/arm: support HW interrupts, do not request maintenance_interrupts

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 1 Apr 2014, Ian Campbell wrote:
> On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> >  
> > +static void gic_clear_one_lr(struct vcpu *v, int i)
> > +{
> > +    struct pending_irq *p;
> > +    uint32_t lr;
> > +    int irq;
> > +    bool_t inflight;
> > +
> > +    ASSERT(!local_irq_is_enabled());
> > +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> [...]
> > +        if ( !inflight )
> > +        {
> > +            spin_lock(&v->arch.vgic.lock);
> 
> Aren't you recursively taking this lock here?
> 
> In principal could you remove the requirement for the caller to hold
> this lock as long as you took it just before p = irq_to_pending(v, irq);
> and released it on each iteration? (I'm not at all sure of that, just
> wondering)

Yes, you are right. I had already fixed this problem in v6 (already sent
to the list). I didn't realize this problem because the spin_lock goes
away with a later patch.


> > +            list_del_init(&p->inflight);
> > +            spin_unlock(&v->arch.vgic.lock);
> > +        }
> > +    }
> > +}
> > +
> > +void gic_clear_lrs(struct vcpu *v)
> > +{
> > +    int i = 0;
> > +    unsigned long flags;
> > +
> > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > +
> > +    while ((i = find_next_bit((const unsigned long *) &this_cpu(lr_mask),
> > +                              nr_lrs, i)) < nr_lrs) {
> > +        gic_clear_one_lr(v, i);
> > +        i++;
> > +    }
> > +
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +}
> > +
> >  static void gic_restore_pending_irqs(struct vcpu *v)
> >  {
> >      int i;
> [...]
> 
> > +    /* 
> > +     * The maintenance interrupt handler doesn't do anything anymore, but
> 
> "This is a dummy interrupt handler." (We don't care about "anymore" once
> this is applied

OK

> > +     * receiving the interrupt is going to cause gic_inject to be called on
> 
>           Receiving... (because of the full stop I added above)
> 
> > @@ -1642,6 +1647,11 @@ asmlinkage void leave_hypervisor_tail(void)
> >      }
> >  }
> >  
> > +static void enter_hypervisor_head(void)
> > +{
> > +    gic_clear_lrs(current);
> > +}
> 
> Just put it before the first use and avoid the need for the forward
> declaration.
 
OK

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