| Message ID | 1396969969-18973-8-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
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 ;-) > - * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the > + * Therefore it is possible to set GIC_IRQ_GUEST_QUEUED while the > * irq is GIC_IRQ_GUEST_VISIBLE. We could also change the state of > * the guest irq in the LR register from active to active and > * pending, but for simplicity we simply inject a second irq after Ian.
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? > > - * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the > > + * Therefore it is possible to set GIC_IRQ_GUEST_QUEUED while the > > * irq is GIC_IRQ_GUEST_VISIBLE. We could also change the state of > > * the guest irq in the LR register from active to active and > > * pending, but for simplicity we simply inject a second irq after > > Ian. >
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.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 869c077..bed6e9c 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -642,7 +642,7 @@ static inline void gic_set_lr(int lr, struct pending_irq *p, GICH[GICH_LR + lr] = lr_reg; set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); - clear_bit(GIC_IRQ_GUEST_PENDING, &p->status); + clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); p->lr = lr; } @@ -723,7 +723,7 @@ static void gic_update_one_lr(struct vcpu *v, int i) p->desc->status &= ~IRQ_INPROGRESS; clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); p->lr = GIC_INVALID_LR; - if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) && + if ( test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) && test_bit(GIC_IRQ_GUEST_ENABLED, &p->status)) { inflight = 1; diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 3913cf5..6a89a1e 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -700,7 +700,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) { if ( (irq != current->domain->arch.evtchn_irq) || (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) ) - set_bit(GIC_IRQ_GUEST_PENDING, &n->status); + set_bit(GIC_IRQ_GUEST_QUEUED, &n->status); goto out; } @@ -714,7 +714,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte); n->irq = irq; - set_bit(GIC_IRQ_GUEST_PENDING, &n->status); + set_bit(GIC_IRQ_GUEST_QUEUED, &n->status); n->priority = priority; /* the irq is enabled */ 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. * * 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. - * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the + * Therefore it is possible to set GIC_IRQ_GUEST_QUEUED while the * irq is GIC_IRQ_GUEST_VISIBLE. We could also change the state of * the guest irq in the LR register from active to active and * pending, but for simplicity we simply inject a second irq after @@ -54,7 +55,7 @@ struct pending_irq * level (GICD_ICENABLER/GICD_ISENABLER). * */ -#define GIC_IRQ_GUEST_PENDING 0 +#define GIC_IRQ_GUEST_QUEUED 0 #define GIC_IRQ_GUEST_VISIBLE 1 #define GIC_IRQ_GUEST_ENABLED 2 unsigned long status;
Rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED and clarify its meaning in xen/include/asm-arm/domain.h. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- xen/arch/arm/gic.c | 4 ++-- xen/arch/arm/vgic.c | 4 ++-- xen/include/asm-arm/domain.h | 11 ++++++----- 3 files changed, 10 insertions(+), 9 deletions(-)