| Message ID | 20190602102614.22640-1-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Xen-devel,v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt | expand |
Hello Julien, On 02.06.19 13:26, Julien Grall wrote: > + * This should only be called with interrupt routed to guest. The flow > + * of interrupt routed to Xen any software change of the state. Sorry I can't parse the last sentence. It seems to me you missed a word/words? For the rest: Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
On 6/10/19 12:23 PM, Andrii Anisov wrote: > Hello Julien, Hi, > On 02.06.19 13:26, Julien Grall wrote: > >> + * This should only be called with interrupt routed to guest. The flow >> + * of interrupt routed to Xen any software change of the state. > > Sorry I can't parse the last sentence. It seems to me you missed a > word/words? Hmmm, sorry for that. How about the following: "This should only be called with interrupt routed to guest. The flow of interrupt routed to Xen is not able cope with software change of the active state"? > > For the rest: > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com> Thank you for the review! I am not planning to resend the patch and I will update the comment on commit (pending Stefano's review). Cheers,
Gentle ping. Cheers, On 02/06/2019 11:26, Julien Grall wrote: > While SPIs are shared between CPU, it is not possible to receive the > same interrupts on a different CPU while the interrupt is in active > state. > > For host interrupt (i.e routed to Xen), the deactivation of the > interrupt is done at the end of the handling. This can alternatively be > done outside of the handler by calling gic_set_active_state(). > > At the moment, gic_set_active_state() is only called by the vGIC for > interrupt routed to the guest. It is hard to find a reason for Xen to > directly play with the active state for interrupt routed to Xen. > > To simplify the handling of host interrupt, gic_set_activate_state() is > now restricted to interrupts routed to guest. > > This means the _IRQ_PENDING logic is now unecessary on Arm as a same > interrupt can never come up while in the loop and nobody should play > with the flag behind our back. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > Changes in v2: > - gic_set_active_state should only be called on interrupt routed > to guest. > - Update the commit message > --- > xen/arch/arm/irq.c | 32 ++++++++++---------------------- > xen/include/asm-arm/gic.h | 4 ++++ > 2 files changed, 14 insertions(+), 22 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index c51cf333ce..3877657a52 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -199,6 +199,7 @@ int request_irq(unsigned int irq, unsigned int irqflags, > void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) > { > struct irq_desc *desc = irq_to_desc(irq); > + struct irqaction *action; > > perfc_incr(irqs); > > @@ -242,35 +243,22 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) > goto out_no_end; > } > > - set_bit(_IRQ_PENDING, &desc->status); > - > - /* > - * Since we set PENDING, if another processor is handling a different > - * instance of this same irq, the other processor will take care of it. > - */ > - if ( test_bit(_IRQ_DISABLED, &desc->status) || > - test_bit(_IRQ_INPROGRESS, &desc->status) ) > + if ( test_bit(_IRQ_DISABLED, &desc->status) ) > goto out; > > set_bit(_IRQ_INPROGRESS, &desc->status); > > - while ( test_bit(_IRQ_PENDING, &desc->status) ) > - { > - struct irqaction *action; > + action = desc->action; > > - clear_bit(_IRQ_PENDING, &desc->status); > - action = desc->action; > + spin_unlock_irq(&desc->lock); > > - spin_unlock_irq(&desc->lock); > - > - do > - { > - action->handler(irq, action->dev_id, regs); > - action = action->next; > - } while ( action ); > + do > + { > + action->handler(irq, action->dev_id, regs); > + action = action->next; > + } while ( action ); > > - spin_lock_irq(&desc->lock); > - } > + spin_lock_irq(&desc->lock); > > clear_bit(_IRQ_INPROGRESS, &desc->status); > > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index fab02f19f7..876727c144 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -400,9 +400,13 @@ static inline unsigned int gic_get_nr_lrs(void) > * Set the active state of an IRQ. This should be used with care, as this > * directly forces the active bit, without considering the GIC state machine. > * For private IRQs this only works for those of the current CPU. > + * > + * This should only be called with interrupt routed to guest. The flow > + * of interrupt routed to Xen any software change of the state. > */ > static inline void gic_set_active_state(struct irq_desc *irqd, bool state) > { > + ASSERT(test_bit(_IRQ_GUEST, &irqd->status)); > gic_hw_ops->set_active_state(irqd, state); > } > >
On Mon, 10 Jun 2019, Julien Grall wrote: > On 6/10/19 12:23 PM, Andrii Anisov wrote: > > Hello Julien, > > Hi, > > > On 02.06.19 13:26, Julien Grall wrote: > > > > > + * This should only be called with interrupt routed to guest. The flow > > > + * of interrupt routed to Xen any software change of the state. > > > > Sorry I can't parse the last sentence. It seems to me you missed a > > word/words? > > Hmmm, sorry for that. How about the following: > > "This should only be called with interrupt routed to guest. The flow of > interrupt routed to Xen is not able cope with software change of the active > state"? > > > > > For the rest: > > > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com> > > Thank you for the review! I am not planning to resend the patch and I will > update the comment on commit (pending Stefano's review). Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> I'll update on commit.
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index c51cf333ce..3877657a52 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -199,6 +199,7 @@ int request_irq(unsigned int irq, unsigned int irqflags, void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) { struct irq_desc *desc = irq_to_desc(irq); + struct irqaction *action; perfc_incr(irqs); @@ -242,35 +243,22 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) goto out_no_end; } - set_bit(_IRQ_PENDING, &desc->status); - - /* - * Since we set PENDING, if another processor is handling a different - * instance of this same irq, the other processor will take care of it. - */ - if ( test_bit(_IRQ_DISABLED, &desc->status) || - test_bit(_IRQ_INPROGRESS, &desc->status) ) + if ( test_bit(_IRQ_DISABLED, &desc->status) ) goto out; set_bit(_IRQ_INPROGRESS, &desc->status); - while ( test_bit(_IRQ_PENDING, &desc->status) ) - { - struct irqaction *action; + action = desc->action; - clear_bit(_IRQ_PENDING, &desc->status); - action = desc->action; + spin_unlock_irq(&desc->lock); - spin_unlock_irq(&desc->lock); - - do - { - action->handler(irq, action->dev_id, regs); - action = action->next; - } while ( action ); + do + { + action->handler(irq, action->dev_id, regs); + action = action->next; + } while ( action ); - spin_lock_irq(&desc->lock); - } + spin_lock_irq(&desc->lock); clear_bit(_IRQ_INPROGRESS, &desc->status); diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index fab02f19f7..876727c144 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -400,9 +400,13 @@ static inline unsigned int gic_get_nr_lrs(void) * Set the active state of an IRQ. This should be used with care, as this * directly forces the active bit, without considering the GIC state machine. * For private IRQs this only works for those of the current CPU. + * + * This should only be called with interrupt routed to guest. The flow + * of interrupt routed to Xen any software change of the state. */ static inline void gic_set_active_state(struct irq_desc *irqd, bool state) { + ASSERT(test_bit(_IRQ_GUEST, &irqd->status)); gic_hw_ops->set_active_state(irqd, state); }
While SPIs are shared between CPU, it is not possible to receive the same interrupts on a different CPU while the interrupt is in active state. For host interrupt (i.e routed to Xen), the deactivation of the interrupt is done at the end of the handling. This can alternatively be done outside of the handler by calling gic_set_active_state(). At the moment, gic_set_active_state() is only called by the vGIC for interrupt routed to the guest. It is hard to find a reason for Xen to directly play with the active state for interrupt routed to Xen. To simplify the handling of host interrupt, gic_set_activate_state() is now restricted to interrupts routed to guest. This means the _IRQ_PENDING logic is now unecessary on Arm as a same interrupt can never come up while in the loop and nobody should play with the flag behind our back. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v2: - gic_set_active_state should only be called on interrupt routed to guest. - Update the commit message --- xen/arch/arm/irq.c | 32 ++++++++++---------------------- xen/include/asm-arm/gic.h | 4 ++++ 2 files changed, 14 insertions(+), 22 deletions(-)