Re: [Xen-devel] [PATCH v7 08/12] xen/arm: rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-05-08 at 18:57 +0100, Stefano Stabellini wrote:
> On Wed, 23 Apr 2014, Ian Campbell wrote:
> > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > > Rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED and clarify its
> > > meaning in xen/include/asm-arm/domain.h.
> > 
> > Thanks.
> > 
> > > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > > index 2d94d59..f96dc12 100644
> > > --- a/xen/include/asm-arm/domain.h
> > > +++ b/xen/include/asm-arm/domain.h
> > > @@ -27,7 +27,8 @@ struct pending_irq
> > >       * whether an irq added to an LR register is PENDING or ACTIVE, the
> > >       * following states are just an approximation.
> > >       *
> > > -     * GIC_IRQ_GUEST_PENDING: the irq is asserted
> > > +     * GIC_IRQ_GUEST_QUEUED: the irq is asserted and queued for
> > > +     * injection into the guests LRs.
> > 
> >                             guest's
> > 
> > >       *
> > >       * GIC_IRQ_GUEST_VISIBLE: the irq has been added to an LR register,
> > >       * therefore the guest is aware of it. From the guest point of view
> > > @@ -35,12 +36,12 @@ struct pending_irq
> > >       * or active (after acking the irq).
> > >       *
> > >       * In order for the state machine to be fully accurate, for level
> > > -     * interrupts, we should keep the GIC_IRQ_GUEST_PENDING state until
> > > +     * interrupts, we should keep the GIC_IRQ_GUEST_QUEUED state until
> > >       * the guest deactivates the irq. However because we are not sure
> > > -     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
> > > +     * when that happens, we simply remove the GIC_IRQ_GUEST_QUEUED
> > >       * state when we add the irq to an LR register. We add it back when
> > >       * we receive another interrupt notification.
> > 
> > This paragraph was essentially clarifying the mismatch between the name
> > PENDING and the actual behaviour. It doesn't seem to make much sense for
> > a bit named QUEUED. In particular "we should keep the QUEUE state until
> > the guest deactivates the IRQ" doesn't seem logical, so the rest doesn't
> > follow as a workaround for it.
> > 
> > How about:
> >       * In order for the state machine to be fully accurate, for level
> >       * interrupts, we should keep the interrupt's pending state until
> >       * the guest deactivates the irq. However because we are not sure
> >       * when that happens, we instead track whether there is an interrupt
> >       * queued using GIC_IRQ_GUEST_QUEUED
> >       * state when we add the irq to an LR register. We add it back when
> >       * we receive another interrupt notification.
> > 
> > (needs rewrapping). Still doesn't sound quite right to me, but you
> > understand what is going on better than me so I hope you can fix it ;-)
> 
> If I make this change then this patch won't be a pure renaming anymore.
> Should I add another patch for this?

I think renaming and updating the docs/comments to reflect the renaming
is OK in a single patch (whereas renaming and actual functional change
wouldn't, but I don't think that is what either of us was suggesting).

Ian.


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