[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Mon, 9 Dec 2013, Ian Campbell wrote:
> On Fri, 2013-12-06 at 17:26 +0000, Stefano Stabellini wrote:
> > Introduce a status field in struct pending_irq. Valid states are
> > GUEST_PENDING and GUEST_VISIBLE and they are not mutually exclusive.
> 
> Are all four combinations valid? In particular is !PENDING and VISIBLE
> valid?

Yes, they are all valid.


> > See the in-code comment for an explanation of the states and how they
> > are used. Protect pending_irq state manipulations with the vgic lock.
> > 
> > The main effect of this patch is that an IRQ can be set to GUEST_PENDING
> > while it is being serviced by the guest. In maintenance_interrupt we
> > check whether GUEST_PENDING is set and if it is we reinject the IRQ one
> > more time.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> 
> Pulling the referenced comment to the top:
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index d5cae2e..5e7bb58 100644
> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -22,6 +22,35 @@ struct vgic_irq_rank {
> >  struct pending_irq
> >  {
> >      int irq;
> > +    /* 
> > +     * The following two states track the lifecycle of the guest irq.
> > +     * However because we are not sure and we don't want to track
> > +     * whether an irq added to an LR register is PENDING or ACTIVE, the
> > +     * following states are just an approximation.
> > +     *
> > +     * GIC_IRQ_GUEST_PENDING: the irq is asserted
> > +     *
> > +     * GIC_IRQ_GUEST_VISIBLE: the irq has been added to an LR register,
> > +     * therefore the guest is aware of it. From the guest point of view
> > +     * the irq can be pending (if the guest has not acked the irq yet)
> > +     * or active (after acking the irq).
> > 
> 
> Is this visibility "sticky" in the case where the the use of the LR is
> preempted by a new higher priority IRQ? (I suppose this is only a
> theoretical concern right now).

Yes, I think it should be "sticky".


> +     * In order for the state machine to be fully accurate, for level
> > +     * interrupts, we should keep the GIC_IRQ_GUEST_PENDING state until
> > +     * the guest deactivates the irq. However because we are not sure
> > +     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
> > +     * state when we add the irq to an LR register. We add it back when
> > +     * we receive another interrupt notification.
> > +     * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the
> > +     * irq is GIC_IRQ_GUEST_VISIBLE. We could also change the state of
> > +     * the guest irq in the LR register from active to active and
> > +     * pending, but for simplicity we simply inject a second irq after
> > +     * the guest EOIs the first one.
> > 
> 
> This last paragraph is just saying that we don't necessarily reuse the
> same LR slot, right?

Yes, if by reuse you mean change the status on the existing LR.


> +     *
> > +     */
> > +#define GIC_IRQ_GUEST_PENDING  (1<<1)
> > +#define GIC_IRQ_GUEST_VISIBLE   (1<<2)
> > +    int status;
> >
> 
> bit flags should be unsigned I think.

OK


>      struct irq_desc *desc; /* only set it the irq corresponds to a physical 
> irq */
> >      uint8_t priority;
> >      /* inflight is used to append instances of pending_irq to
> > 
> 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 7e87acb..a8a5c2a 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -626,6 +626,7 @@ static inline void gic_set_lr(int lr, unsigned int 
> > virtual_irq,
> >          ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> >  }
> >  
> > +/* needs to be called with vgic lock held */
> >  void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> >          unsigned int state, unsigned int priority)
> >  {
> > @@ -635,17 +636,20 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int 
> > virtual_irq,
> >  
> >      spin_lock_irqsave(&gic.lock, flags);
> >  
> > +    n = irq_to_pending(v, virtual_irq);
> > +
> >      if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
> >      {
> >          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
> >          if (i < nr_lrs) {
> >              set_bit(i, &this_cpu(lr_mask));
> >              gic_set_lr(i, virtual_irq, state, priority);
> > +            n->status |= GIC_IRQ_GUEST_VISIBLE;
> > +            n->status &= ~GIC_IRQ_GUEST_PENDING;
> 
> Would using set/clear_bit here allow the locking restrictions to be
> relaxed?

Yes, good idea.


> >              goto out;
> >          }
> >      }
> >  
> > -    n = irq_to_pending(v, virtual_irq);
> >      if ( !list_empty(&n->lr_queue) )
> >          goto out;
> >  
> > @@ -679,6 +683,8 @@ static void gic_restore_pending_irqs(struct vcpu *v)
> >          gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> >          list_del_init(&p->lr_queue);
> >          set_bit(i, &this_cpu(lr_mask));
> > +        p->status |= GIC_IRQ_GUEST_VISIBLE;
> > +        p->status &= ~GIC_IRQ_GUEST_PENDING;
> >          spin_unlock_irqrestore(&gic.lock, flags);
> >      }
> >  
> > @@ -884,6 +890,7 @@ static void maintenance_interrupt(int irq, void 
> > *dev_id, struct cpu_user_regs *r
> >      uint32_t lr;
> >      struct vcpu *v = current;
> >      uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 
> > 32);
> > +    unsigned long flags;
> >  
> >      while ((i = find_next_bit((const long unsigned int *) &eisr,
> >                                64, i)) < 64) {
> > @@ -893,23 +900,13 @@ static void maintenance_interrupt(int irq, void 
> > *dev_id, struct cpu_user_regs *r
> >          cpu = -1;
> >          eoi = 0;
> >  
> > -        spin_lock_irq(&gic.lock);
> > +        spin_lock(&v->arch.vgic.lock);
> > +        spin_lock_irqsave(&gic.lock, flags);
> 
> Is the locking hierarchy for vgic.lock, gic.lock and desc.lock written
> down somewhere?
> 
> Having irqsave on the inner lock is weird, isn't it? The outer one will
> clobber the irq status anyway.
> 
> You haven't changed the contexts where maintenance_interrupt can be
> called either -- have you?

This goes away thanks to the atomic bit operations, see next version of
the patch.


> >          lr = GICH[GICH_LR + i];
> >          virq = lr & GICH_LR_VIRTUAL_MASK;
> >          GICH[GICH_LR + i] = 0;
> >          clear_bit(i, &this_cpu(lr_mask));
> >  
> > -        if ( !list_empty(&v->arch.vgic.lr_pending) ) {
> > -            p = list_entry(v->arch.vgic.lr_pending.next, typeof(*p), 
> > lr_queue);
> > -            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> > -            list_del_init(&p->lr_queue);
> > -            set_bit(i, &this_cpu(lr_mask));
> > -        } else {
> > -            gic_inject_irq_stop();
> > -        }
> > -        spin_unlock_irq(&gic.lock);
> > -
> > -        spin_lock_irq(&v->arch.vgic.lock);
> >          p = irq_to_pending(v, virq);
> >          if ( p->desc != NULL ) {
> >              p->desc->status &= ~IRQ_INPROGRESS;
> > @@ -918,8 +915,32 @@ static void maintenance_interrupt(int irq, void 
> > *dev_id, struct cpu_user_regs *r
> >              eoi = 1;
> >              pirq = p->desc->irq;
> >          }
> > +        if ( p->status & GIC_IRQ_GUEST_PENDING )
> > +        {
> > +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> 
> Does this conflict with what the last para of the main comment said?
> i.e. we do infact reuse the slot via gic_set_lr here rather than
> reinjecting?

I see where the confusion is: reusing or not exactly the same LR
register doesn't make a difference. What does is that we do not
reuse the same interrupt notification by changing the status of the live
LR before it is EOIed by the guest.


> > +            p->status &= ~GIC_IRQ_GUEST_PENDING;
> 
> Don't need to mess with VISIBLE here?

Nope because it is still visible after gic_set_lr (the second
notification is visible).


> > +            i++;
> > +            spin_unlock_irqrestore(&gic.lock, flags);
> > +            spin_unlock(&v->arch.vgic.lock);
> > +            continue;
> > +        }
> > +
> > +        p->status &= ~GIC_IRQ_GUEST_VISIBLE;
> >          list_del_init(&p->inflight);
> > -        spin_unlock_irq(&v->arch.vgic.lock);
> > +
> > +        if ( !list_empty(&v->arch.vgic.lr_pending) ) {
> > +            p = list_entry(v->arch.vgic.lr_pending.next, typeof(*p), 
> > lr_queue);
> > +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> > +            p->status |= GIC_IRQ_GUEST_VISIBLE;
> > +            p->status &= ~GIC_IRQ_GUEST_PENDING;
> > +            list_del_init(&p->lr_queue);
> > +            set_bit(i, &this_cpu(lr_mask));
> > +        } else {
> > +            gic_inject_irq_stop();
> > +        }
> > +
> > +        spin_unlock_irqrestore(&gic.lock, flags);
> > +        spin_unlock(&v->arch.vgic.lock);
> >  
> >          if ( eoi ) {
> >              /* this is not racy because we can't receive another irq of the
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 2e4b11f..c71db37 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -365,6 +365,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t 
> > r, int n)
> >      struct pending_irq *p;
> >      unsigned int irq;
> >      int i = 0;
> > +    unsigned long f;
> 
> "flags" is conventional.

this also goes away after switching to atomic bits operations.


> > +
> > +    spin_lock_irqsave(&v->arch.vgic.lock, f);
> >  
> >      while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 
> > 32 ) {
> >          irq = i + (32 * n);
> > @@ -373,6 +376,8 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t 
> > r, int n)
> >              gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> >          i++;
> >      }
> > +
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, f);
> >  }
> >  
> >  static inline int is_vcpu_running(struct domain *d, int vcpuid)
> > @@ -672,8 +677,15 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
> > irq, int virtual)
> >  
> >      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >  
> > -    /* vcpu offline or irq already pending */
> > -    if (test_bit(_VPF_down, &v->pause_flags) || !list_empty(&n->inflight))
> > +    if ( !list_empty(&n->inflight) )
> > +    {
> > +        n->status |= GIC_IRQ_GUEST_PENDING;
> > +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +        return;
> > +    }
> > +
> > +    /* vcpu offline */
> 
> You should test for this first I think? Otherwise you would end up
> injecting it. 

I don't think so: a cpu can receive an interrupt from the distributor
even the cpu is not responding.
Of course the vcpu cannot actually receive the interrupt until is woken
up.


> > +    if ( test_bit(_VPF_down, &v->pause_flags) )
> >      {
> >          spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >          return;
> > @@ -682,6 +694,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
> > irq, int virtual)
> >      priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
> >  
> >      n->irq = irq;
> > +    n->status = GIC_IRQ_GUEST_PENDING;
> 
> Should this not be |=? Otherwise we potentially lose the fact that the
> IRQ is VISIBLE?

Yep


> >      n->priority = priority;
> >      if (!virtual)
> >          n->desc = irq_to_desc(irq);

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.