Re: [Xen-devel] [PATCH v4 2/6] xen/arm: track the state of guest IRQs

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 12 Dec 2013, Ian Campbell wrote:
> On Thu, 2013-12-12 at 15:45 +0000, Ian Campbell wrote:
> > On Thu, 2013-12-12 at 15:25 +0000, Stefano Stabellini wrote:
> > > On Thu, 12 Dec 2013, Ian Campbell wrote:
> > > > On Thu, 2013-12-12 at 15:17 +0000, Stefano Stabellini wrote:
> > > > 
> > > > > > > +            set_int = 1;
> > > > > > > +        }
> > > > > > > +
> > > > > > > +        clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > > > > > > +
> > > > > > > +        if ( !list_empty(&v->arch.vgic.lr_pending) ) {
> > > > > > > +            p2 = list_entry(v->arch.vgic.lr_pending.next, 
> > > > > > > typeof(*p2), lr_queue);
> > > > > > > +            gic_set_lr(i, p2->irq, GICH_LR_PENDING, 
> > > > > > > p2->priority);
> > > > > > > +            list_del_init(&p2->lr_queue);
> > > > > > > +            set_bit(i, &this_cpu(lr_mask));
> > > > > > > +            set_int = 1;
> > > > > > > +        }
> > > > > > > +        spin_unlock_irq(&gic.lock);
> > > > > > > +
> > > > > > > +        spin_lock_irq(&v->arch.vgic.lock);
> > > > > > >          list_del_init(&p->inflight);
> > > > > > >          spin_unlock_irq(&v->arch.vgic.lock);
> > > > > > >  
> > > > > > > @@ -931,6 +955,12 @@ static void maintenance_interrupt(int irq, 
> > > > > > > void *dev_id, struct cpu_user_regs *r
> > > > > > >  
> > > > > > >          i++;
> > > > > > >      }
> > > > > > > +    if ( !set_int )
> > > > > > > +    {
> > > > > > > +        spin_lock_irq(&gic.lock);
> > > > > > 
> > > > > > Dropping the lock and then retaking it here is a bit of a suspicious
> > > > > > pattern -- what if something changes in the interim?
> > > > > 
> > > > > Vcpu A has EOIed a guest IRQ, causing a maintenance_interrupt.
> > > > > At the same time vcpu B is trying to inject a new interrupt into the
> > > > > vcpu A.
> > > > > 
> > > > > 
> > > > > A: maintenance_interrupt: set_int = 0
> > > > > B: create a new inflight irq for A
> > > > > B: send an IPI
> > > > > A: receive an IPI
> > > > > A: gic_inject->gic_inject_irq_start
> > > > > A: maintenance_interrupt: gic_inject_irq_stop
> > > > > A: gic_inject->gic_inject_irq_start
> > > > > 
> > > > > 
> > > > > It works! :)
> > > > 
> > > > What is the last "A: gic_inject->gic_inject_irq_start" from?
> > > 
> > > leave_hypervisor_tail->gic_inject->gic_inject_irq_start
> > 
> > Can't the stop in the maintenance interrupt be unconditional then? If
> > there are additional interrupts pending then they will always be picked
> > up via leave_hypervisor_tail, which is a nice and simple rule to
> > remember.
> 
> Or going one step further -- why isn't leave_hypervisor_tail the only
> place which touches HCR_VI at all? That's a very simple rule...

I agree. I'll make the change.

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