Re: [Xen-devel] [PATCH v7 11/12] xen/arm: gic_events_need_delivery and irq priorities

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

> > On Wed, 23 Apr 2014, Ian Campbell wrote:
> > > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > > > +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & 
> > > > GICH_VMCR_PRIORITY_MASK;
> > > 
> > > mask_priority is a bit meaningless (I know the docs call it
> > > priority_mask), but its the vcpus current priority, right?
> > 
> > It is the minimum priority that an interrupt needs to have for the cpu
> > to be interrupted by the gic.
> 
> OK, I think you are stating the same thing as me but from a different
> angle.
> > 
> > 
> > > Also, by adding a << 3 here then I think the rest of the logic reads
> > > more easily, because you are then using the priority directly, and
> > > ignoring the fact that VMCR has a limited precision. Whereas with the
> > > shift >> 3 at the comparison sights you kind of have to think about it
> > > in each case.
> > >
> > > > +
> > > > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > > +
> > > > +    /* TODO: We order the guest irqs by priority, but we don't change
> > > > +     * the priority of host irqs. */
> > > > +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> > > > +    {
> > > > +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > > > +        {
> > > > +            if ( p->priority < active_priority )
> > > > +                active_priority = p->priority;
> > > > +            break;
> > > > +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> > > > +            if ( p->priority < max_priority )
> > > > +                max_priority = p->priority;
> > > > +        }
> > > > +        if ( (p->priority >> 3) >= mask_priority )
> > > > +            break;
> > > 
> > > This lrs-- stuff needs a comment. I think along the lines of only the
> > > first nr_lrs interrupt need to be considered because XXX.
> > > 
> > > Not sure what XXX is -- if we have 4 lrs and inflight_irqs has 10
> > > entries on it (say) of which the first 5 happen to be masked, don't we
> > > want to keep going until we've looked at more than the first 4 entries
> > > or something? Or should this decrement be conditional on ENABLE and/or
> > > ACTIVE perhaps?
> > 
> > If we have 4 lrs and inflight_irqs has 10 entries on it (say) of which
> > the first 5 happen to be masked, we want to keep going until we've
> > looked at more than the first 4 entries but we certainly cannot swap
> > more than 4 entries.
> 
> I think what is confusing me is that I don't see where the lrs-- is
> skipped for a masked interrupt. So aren't you counting down the lrs
> variable even for the first 5 which happen to be masked?

At the moment when the guest disables an irq, we remove it from the
lr_pending queue but we don't remove it from the inflight queue. So if
the irq has already been added to an LR register, the guest is going to
receive a notification still.

This patch doesn't change this behaviour: the eviction code in
gic_restore_pending_irqs doesn't distinguish between masked and unmasked
irqs, it treats them the same way, simply going by priority.
Consistently in gic_events_need_delivery, we only analyze the first
nr_lrs irqs by priority, regardless if they are masked or unmasked.

To answer your original question: no, we don't need to keep going past
the first 4 irqs in inflight_irqs, even if they are all masked.

Admittedly this behaviour could be improved, but it might be best to fix
it in a consequent patch series.


> > > You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
> > > exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
> > > haven't yet seen an active one then don't you know that max_priority <
> > > active_priority? (this might be best discussed around a whiteboard...)
> > >
> > > > +        lrs--;
> > > > +        if ( lrs == 0 )
> > > > +            break;
> > > > +    }
> > > > +
> > > > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > > > +
> > > > +    if ( max_priority < active_priority &&
> > > > +         (max_priority >> 3) < mask_priority )
> > > > +        return 1;
> > > > +    else
> > > > +        return 0;
> > > >  }
> > > >  
> > > >  void gic_inject(void)
> > > 
> > > Ian
> > > 
> 
> 

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