Re: [Xen-devel] [PATCH v5 08/10] xen/arm: second irq injection while the first irq is still inflight

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 2 Apr 2014, Ian Campbell wrote:
> On Wed, 2014-04-02 at 16:31 +0100, Stefano Stabellini wrote:
> > On Tue, 1 Apr 2014, Ian Campbell wrote:
> > > On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> > > > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq
> > > > while the first one is still active.
> > > > If the first irq is already pending (not active), just clear
> > > > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> > > > already visible by the guest.
> > > 
> > > I'm confused by this.
> > > 
> > > If the interrupt is pending but not active then the guest has yet to
> > > read GICC_IAR, so while it might be "visible" to the guest it has not
> > > been observed by the guest.
> >  
> > Yes, it is visible to the guest VM but not yet been observed by the
> > guest operating system.
> > 
> > 
> > > The comment says:
> > > 
> > >      * GIC_IRQ_GUEST_PENDING: the irq is asserted
> > > 
> > > I'm not sure how a second irq arriving would correspond to deasserting
> > > the IRQ.
> > 
> > I see where the confusion is coming from.
> > This comment is not quite correct unfortunately.
> > 
> > GIC_IRQ_GUEST_PENDING is set when the irq needs to be asserted (by
> > adding it to the LRs). Once the irq has become guest visible,
> > GIC_IRQ_GUEST_PENDING is cleared.
> > 
> > Going back to the top of the commit message:
> > 
> > "If the first irq is already pending (not active), just clear
> > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> > already visible by the guest"
> > 
> > means that if the first irq has already been added to the LRs but it is
> > still in pending state, we cannot add it a second time, so just go ahead
> > and clear GIC_IRQ_GUEST_PENDING as if we did add it to the LRs, because
> > the guest is still going to receive a notification.
> 
> OK, so when you use lower case pending/active you are talking about the
> LR state. But when you use upper case GIC_IRQ_GUEST_PENDING this is a
> completely separate state related to the queueing of interrupts within
> Xen itself?
> 
> Can we s/GIC_IRQ_GUEST_PENDING/GIC_IRQ_GUEST_QUEUED/ perhaps? The
> comment would be something like:
>      * GIC_IRQ_GUEST_QUEUED: the irq is asserted and queued for
>        injection into the guests LRs.

I realize now that the current naming scheme could be confusing.  When I
talk about pending, active and GICH_LR_PENDING in this commit message, I
am talking about the irq status in the LR register. When I talk about
GIC_IRQ_GUEST_PENDING, I am referring to the GIC internal state tracker.

Because we don't actually know when the guest irq goes from pending to
active, we are clearing GIC_IRQ_GUEST_PENDING as soon as we add an irq
to an LR, as stated in the comment in xen/include/asm-arm/domain.h.
However as a result GIC_IRQ_GUEST_PENDING is more like
"GIC_IRQ_GUEST_QUEUED" rather than "GIC_IRQ_GUEST_PENDING", like you
wrote.

I'll add a patch to do the renaming as suggested.

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