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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-05-08 at 19:37 +0100, Stefano Stabellini wrote:
> 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?

> > 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