Re: [PATCH v4 1/2] arm: support fewer LR registers than virtual irqs

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2012-02-29 at 18:34 +0000, Stefano Stabellini wrote:

> > > +    {
> > > +        i = find_first_zero_bit(&gic.lr_mask, sizeof(uint64_t));
> > > +        if (i < sizeof(uint64_t)) {
> > 
> > What does find_first_zero_bit(0xffff.fff, 64) return?
> 
> 0

So the if is wrong?

What does it return for 0x0? I'd have expected it to return 0 too (the
zeroth bit). I guess the response must be 1-based? In which case don't
you need to subtract 1 somewhere so that bit 1 being clear leads you to
use LR0?

Also, it occurs to me that you need to check i against the number of LRs
too -- otherwise if you have 4 LRs all in use then mask is 0xF but
find_first_zero_bit(0xF, sizeof(...) will return 5 (or is it six?) and
you will try and deploy the non-existent 5th LR.

I'm a bit concerned that the #irqs > #LRs code paths probably haven't
been run, even though you tested on a system, with only 4 LRs. Perhaps
you could artificially inject a bunch of unused/spurious interrupts at
the same time e.g. from a keyhandler?

Also there is an option in the model (at build time for sure, perhaps at
runtime too via -C parameters) to reduce the number of LRs, perhaps
setting it to 1 or 2 would help exercise these code paths a bit more
than 4? We are unlikely to be hitting 5 concurrent pending interrupts
with our current uses.

> > If we imagine a list containing priorities [2,4,6] into which we are
> > inserting a priority 5 interrupt.
> > 
> > On the first iteration of the loop iter->priority == 2 so "if
> > (iter->priority < priority)" is true and we insert 5 after 2 in the list
> > resulting in [2,5,4,6].
> 
> Actually list_add_tail adds the entry before the head, not after.

Oh, yes, it treats the thing you give it as the head and therefore the
tail is == prev because the list is circular. Subtle!

> So if we have the right priority check and the list is [2,4,6], adding 5
> should result in [2,4,5,6].

Indeed it _should_ ;-)

[...]

> > Calling this "link" or "lr_link" when it is used in the context or link
> > registers (e.g. link-register_link) just adds to the confusion around
> > these two lists IMHO, as does having one just called link and the other
> > prefix_link. Calling them foo_list, both with a descriptive prefix,
> > would be better, e.g. active_list and queued_list (or whatever you deem
> > appropriate to their semantics)
> > 
> > Even better would be if the invariant "always on either active or
> > pending lists, never both" were true -- in which case only one list_head
> > would be needed here.
> 
> I'll try to come up with better descriptive names and comments.

Thanks.

> > What do we actually use "link" for? It is chained off vgic.inflight_irqs
> > but we seem to only use it for anything other than manipulating itself
> > in vgic_softirq where it is used as a binary "something to inject" flag
> > -- we could just as well maintain a nr_inflight variable if that were
> > the case, couldn't we?
> 
> It is used by the vgic to keep track of what IRQs have been injected
> from its point of view. These IRQs might have been injected and
> currently resident in an LR register, or they might be queued in
> lr_pending. I'll write a comment to better the explain the life cycle of
> an IRQ.

Awesome, cheers!

Ian.