Re: [Xen-devel] [PATCH v3] arm: support fewer LR registers than virtual irqs

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 27 Feb 2012, Ian Campbell wrote:
> On Thu, 2012-02-23 at 15:45 +0000, Stefano Stabellini wrote:
> > On Fri, 17 Feb 2012, Ian Campbell wrote:
> > > If there's only going to be one active LR then we can jut always use LR0
> > > and do away with the bitmap for the time being.
> > 
> > We have two lists: one list, inflight_irqs, is kept by the vgic to keep
> > track of which irqs the vgic has injected and have not been eoi'd by the
> > guest yet.
> 
> This means "currently live in an LR" ?

nope, that means "I want to be injected in the guest"


> > The second list, gic.lr_pending, is a list of irqs that from the vgic
> > POV have been already injected (they have been added to
> > inflight_irqs already) but because of hardware limitations
> > (limited availability of LR registers) the gic hasn't been able to
> > inject them yet.
> 
> This means "would be in an LR if there was space" ?

yes


> Is lr_pending list a superset of the inflight_irqs ones? Or are they
> always disjoint? If they are disjoint then doesn't a single list next
> pointer in struct pending_irq suffice?

inflight_irqs is a superset of lr_pending


> It would be really nice to have a comment somewhere which describes the
> purpose of these lists and what the invariants are for an entry on them.

yeah


> > Here we are going through the second list, gic.lr_pending, that is
> > ordered by priority, and we are inserting this last guest irq in the
> > right spot so that as soon as an LR register is available we can
> > actually inject it into the guest.
> > 
> > The vgic is not actually aware that the gic hasn't managed to inject the
> > irq yet.
> 
> I was just looking at applying v4 and it has:
> 
> > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > index 3372d14..75095ff 100644
> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -21,6 +21,7 @@ struct pending_irq
> >      struct irq_desc *desc; /* only set it the irq corresponds to a 
> > physical irq */
> >      uint8_t priority;
> >      struct list_head link;
> > +    struct list_head lr_link;
> 
> I think two list fields named link and lr_link need something to
> describe and/or distinguish them, their purpose etc.
> 
> The use of the name "pending" as the field in the struct is a little
> confusing, because there are multiple ways in which an interrupt can be
> pending, both in and out of an LR.
 
yes, they deserve at least a comment

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