Re: [Xen-devel] [PATCH 3/5] xen/arm: Don't reinject the IRQ if it's already in LRs

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 26 Jun 2013, Ian Campbell wrote:
> On Tue, 2013-06-25 at 18:05 +0100, Stefano Stabellini wrote:
> > On Tue, 25 Jun 2013, Ian Campbell wrote:
> > > On Tue, 2013-06-25 at 17:36 +0100, Stefano Stabellini wrote:
> > > > I think it's time we introduce a "status" member in struct irq_desc, so
> > > > that we are not dependent on the information in the GICH_LR registers or
> > > > the queue a pending_irq has been added to.
> > > 
> > > Yes please, I find this one of the hardest things to keep straight in my
> > > head (not helped by my inability to remember which of pending and
> > > inflight is which...)
> > > 
> > > > I would implement it as a bitfield:
> > > > 
> > > > int status;
> > > > #define GIC_IRQ_ENABLED  (1<<0)
> > > > #define GIC_IRQ_INFLIGHT (1<<1)
> > > > #define GIC_IRQ_INLR     (1<<2)
> > > > 
> > > > This way you should just go through the inflight queue and check whether
> > > > status & GIC_IRQ_INLR.
> > > 
> > > Since some of this stuff happens in interrupt context you probably want
> > > test_bit/set_bit et al rather than regular boolean logic, don't you?
> > > 
> > > > At the moment we just want to represent this basic state machine:
> > > > 
> > > > irq guest asserted -> inflight -> injected (inlr) -> unasserted 
> > > > (maintenance interrupt)
> > > 
> > > Can we model the states after the active/pending states which the gic
> > > has? It might make a bunch of stuff clearer?
> > 
> > It might be worth storing those too.
> > So maybe:
> > 
> > #define GIC_IRQ_ENABLED           (1<<0)
> > #define GIC_IRQ_PENDING           (1<<1)
> > #define GIC_IRQ_ACTIVE            (1<<2)
> > #define GIC_IRQ_GUEST_INFLIGHT    (1<<3)
> > #define GIC_IRQ_GUEST_INLR        (1<<4)
> > 
> > however if we do store the physical gic states (GIC_IRQ_PENDING and
> > GIC_IRQ_ACTIVE) then maybe the right place for them is actually irq_desc
> > rather than irq_pending that is used just for guest irqs.
> 
> I was thinking of these as states of the emulated interrupts, rather
> than any underlying physical interrupts, so I think irq_pending is
> correct?

In that case yes


> It occurs to me that at least some of these bits are also fields in the
> LR. I think it is good to save them separately (reducing the
> intertwining of our interrupt handling from GIC internals is a good
> thing) but it means you will need to take care of syncing the state
> between the LR and our internal state at various points, since the LR
> can change based on the guest EOI etc.

I don't think we can accurately distinguish between pending and active
states for guest irqs, because the transition between the two happens
transparently from Xen's point of view.
The only thing we can do is update the state in irq_pending when we save
and restore the GICH_LR registers.
That might still be useful because it would allow us to know which ones
of the irqs currently in the LRs registers can be temporarily set aside
(for example to implement priorities).

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