| Message ID | 20190128155909.14289-1-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Xen-devel,for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt | expand |
Hi, Gentle ping. Cheers, On 1/28/19 3:59 PM, 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. The deactivation of the interrupt is done at the end of the > handling. > > This means the _IRQ_PENDING logic is unecessary on Arm as a same > interrupt can never come up while in the loop. So remove it to > simplify the interrupt handle code. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/irq.c | 32 ++++++++++---------------------- > 1 file changed, 10 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); > >
On 28.01.19 17:59, 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. The deactivation of the interrupt is done at the end of the > handling. > > This means the _IRQ_PENDING logic is unecessary on Arm as a same > interrupt can never come up while in the loop. So remove it to > simplify the interrupt handle code. What about _IRQ_PENDING macro itself? Any reasons to not eliminate it? > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/irq.c | 32 ++++++++++---------------------- > 1 file changed, 10 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); > >
On 05/04/2019 15:16, Andrii Anisov wrote: > On 28.01.19 17:59, 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. The deactivation of the interrupt is done at the end of the >> handling. >> >> This means the _IRQ_PENDING logic is unecessary on Arm as a same >> interrupt can never come up while in the loop. So remove it to >> simplify the interrupt handle code. > > What about _IRQ_PENDING macro itself? > Any reasons to not eliminate it? It is in common header and used by x86. It is preferable to keep all IRQ flags at the same place hence why this was not moved in arch-specific header. Cheers,
Hello Julien, On 05.04.19 17:34, Julien Grall wrote: > It is in common header and used by x86. It is preferable to keep all IRQ flags at the same place hence why this was not moved in arch-specific header. Ah, yes, sure. Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
On Mon, 28 Jan 2019, 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. The deactivation of the interrupt is done at the end of the > handling. > > This means the _IRQ_PENDING logic is unecessary on Arm as a same > interrupt can never come up while in the loop. So remove it to > simplify the interrupt handle code. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/irq.c | 32 ++++++++++---------------------- > 1 file changed, 10 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; It is a good idea to remove the IRQ_PENDING logic, that is OK. However, are we sure that we want to remove the _IRQ_INPROGRESS check too? IRQ handlers shouldn't be called twice in a row. Given that _IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it seems it would be a good idea to keep the check anyway? > 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); > > -- > 2.11.0 >
Hi Stefano, On 4/16/19 10:51 PM, Stefano Stabellini wrote: > On Mon, 28 Jan 2019, 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. The deactivation of the interrupt is done at the end of the >> handling. >> >> This means the _IRQ_PENDING logic is unecessary on Arm as a same >> interrupt can never come up while in the loop. So remove it to >> simplify the interrupt handle code. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/irq.c | 32 ++++++++++---------------------- >> 1 file changed, 10 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; > > It is a good idea to remove the IRQ_PENDING logic, that is OK. > > > However, are we sure that we want to remove the _IRQ_INPROGRESS check > too? IRQ handlers shouldn't be called twice in a row. Given that > _IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it seems it > would be a good idea to keep the check anyway? set_active_state is only used by the vGIC to replicate state from of the virtual interrupt to the physical interrupt. We don't have the virtual interrupt in this path (see above). Any other user (e.g interrupts routed to Xen) would be pretty broken. At best you would break the interrupt flow. At worst, you may never receive the interrupt again. So I think we can drop _IRQ_PROGRESS here. Cheers,
On Tue, 16 Apr 2019, Julien Grall wrote: > Hi Stefano, > > On 4/16/19 10:51 PM, Stefano Stabellini wrote: > > On Mon, 28 Jan 2019, 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. The deactivation of the interrupt is done at the end of the > > > handling. > > > > > > This means the _IRQ_PENDING logic is unecessary on Arm as a same > > > interrupt can never come up while in the loop. So remove it to > > > simplify the interrupt handle code. > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > --- > > > xen/arch/arm/irq.c | 32 ++++++++++---------------------- > > > 1 file changed, 10 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; > > > > It is a good idea to remove the IRQ_PENDING logic, that is OK. > > > > > > However, are we sure that we want to remove the _IRQ_INPROGRESS check > > too? IRQ handlers shouldn't be called twice in a row. Given that > > _IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it seems it > > would be a good idea to keep the check anyway? > > set_active_state is only used by the vGIC to replicate state from of the > virtual interrupt to the physical interrupt. We don't have the virtual > interrupt in this path (see above). > > Any other user (e.g interrupts routed to Xen) would be pretty broken. At best > you would break the interrupt flow. At worst, you may never receive the > interrupt again. > > So I think we can drop _IRQ_PROGRESS here. I gave it a close look. You are right, it is safe to remove the _IRQ_PROGRESS check here. Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> The thing that worries me a bit is that technically set_active_state is part of the gic_hw_operations functions which are not necessarily guest specific: we haven't written down anywhere that set_active_state cannot be called passing one of the xen irqs as parameter. I agree it would be broken to do so, but still... Maybe we should add a comment?
Hi, On 17/04/2019 18:12, Stefano Stabellini wrote: > On Tue, 16 Apr 2019, Julien Grall wrote: >> Hi Stefano, >> >> On 4/16/19 10:51 PM, Stefano Stabellini wrote: >>> On Mon, 28 Jan 2019, 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. The deactivation of the interrupt is done at the end of the >>>> handling. >>>> >>>> This means the _IRQ_PENDING logic is unecessary on Arm as a same >>>> interrupt can never come up while in the loop. So remove it to >>>> simplify the interrupt handle code. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>> --- >>>> xen/arch/arm/irq.c | 32 ++++++++++---------------------- >>>> 1 file changed, 10 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; >>> >>> It is a good idea to remove the IRQ_PENDING logic, that is OK. >>> >>> >>> However, are we sure that we want to remove the _IRQ_INPROGRESS check >>> too? IRQ handlers shouldn't be called twice in a row. Given that >>> _IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it seems it >>> would be a good idea to keep the check anyway? >> >> set_active_state is only used by the vGIC to replicate state from of the >> virtual interrupt to the physical interrupt. We don't have the virtual >> interrupt in this path (see above). >> >> Any other user (e.g interrupts routed to Xen) would be pretty broken. At best >> you would break the interrupt flow. At worst, you may never receive the >> interrupt again. >> >> So I think we can drop _IRQ_PROGRESS here. > > I gave it a close look. You are right, it is safe to remove the > _IRQ_PROGRESS check here. > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > > The thing that worries me a bit is that technically set_active_state is > part of the gic_hw_operations functions which are not necessarily guest > specific: we haven't written down anywhere that set_active_state cannot > be called passing one of the xen irqs as parameter. I agree it would be > broken to do so, but still... Maybe we should add a comment? How about adding an ASSERT(test_bit(_IRQ_GUEST, &desc->status)) ? Cheers,
On Wed, 17 Apr 2019, Julien Grall wrote: > Hi, > > On 17/04/2019 18:12, Stefano Stabellini wrote: > > On Tue, 16 Apr 2019, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 4/16/19 10:51 PM, Stefano Stabellini wrote: > > > > On Mon, 28 Jan 2019, 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. The deactivation of the interrupt is done at the end of the > > > > > handling. > > > > > > > > > > This means the _IRQ_PENDING logic is unecessary on Arm as a same > > > > > interrupt can never come up while in the loop. So remove it to > > > > > simplify the interrupt handle code. > > > > > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > > --- > > > > > xen/arch/arm/irq.c | 32 ++++++++++---------------------- > > > > > 1 file changed, 10 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; > > > > > > > > It is a good idea to remove the IRQ_PENDING logic, that is OK. > > > > > > > > > > > > However, are we sure that we want to remove the _IRQ_INPROGRESS check > > > > too? IRQ handlers shouldn't be called twice in a row. Given that > > > > _IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it seems it > > > > would be a good idea to keep the check anyway? > > > > > > set_active_state is only used by the vGIC to replicate state from of the > > > virtual interrupt to the physical interrupt. We don't have the virtual > > > interrupt in this path (see above). > > > > > > Any other user (e.g interrupts routed to Xen) would be pretty broken. At > > > best > > > you would break the interrupt flow. At worst, you may never receive the > > > interrupt again. > > > > > > So I think we can drop _IRQ_PROGRESS here. > > > > I gave it a close look. You are right, it is safe to remove the > > _IRQ_PROGRESS check here. > > > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > > > > > The thing that worries me a bit is that technically set_active_state is > > part of the gic_hw_operations functions which are not necessarily guest > > specific: we haven't written down anywhere that set_active_state cannot > > be called passing one of the xen irqs as parameter. I agree it would be > > broken to do so, but still... Maybe we should add a comment? > > How about adding an ASSERT(test_bit(_IRQ_GUEST, &desc->status)) ? even better
Hi Stefano, On 17/04/2019 18:27, Stefano Stabellini wrote: > On Wed, 17 Apr 2019, Julien Grall wrote: >> Hi, >> >> On 17/04/2019 18:12, Stefano Stabellini wrote: >>> On Tue, 16 Apr 2019, Julien Grall wrote: >>>> Hi Stefano, >>>> >>>> On 4/16/19 10:51 PM, Stefano Stabellini wrote: >>>>> On Mon, 28 Jan 2019, 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. The deactivation of the interrupt is done at the end of the >>>>>> handling. >>>>>> >>>>>> This means the _IRQ_PENDING logic is unecessary on Arm as a same >>>>>> interrupt can never come up while in the loop. So remove it to >>>>>> simplify the interrupt handle code. >>>>>> >>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>>>> --- >>>>>> xen/arch/arm/irq.c | 32 ++++++++++---------------------- >>>>>> 1 file changed, 10 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; >>>>> >>>>> It is a good idea to remove the IRQ_PENDING logic, that is OK. >>>>> >>>>> >>>>> However, are we sure that we want to remove the _IRQ_INPROGRESS check >>>>> too? IRQ handlers shouldn't be called twice in a row. Given that >>>>> _IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it seems it >>>>> would be a good idea to keep the check anyway? >>>> >>>> set_active_state is only used by the vGIC to replicate state from of the >>>> virtual interrupt to the physical interrupt. We don't have the virtual >>>> interrupt in this path (see above). >>>> >>>> Any other user (e.g interrupts routed to Xen) would be pretty broken. At >>>> best >>>> you would break the interrupt flow. At worst, you may never receive the >>>> interrupt again. >>>> >>>> So I think we can drop _IRQ_PROGRESS here. >>> >>> I gave it a close look. You are right, it is safe to remove the >>> _IRQ_PROGRESS check here. >>> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>> >>> >>> The thing that worries me a bit is that technically set_active_state is >>> part of the gic_hw_operations functions which are not necessarily guest >>> specific: we haven't written down anywhere that set_active_state cannot >>> be called passing one of the xen irqs as parameter. I agree it would be >>> broken to do so, but still... Maybe we should add a comment? >> >> How about adding an ASSERT(test_bit(_IRQ_GUEST, &desc->status)) ? > > even better Do you want the change to be in this patch or separately? Cheers,
On Mon, 20 May 2019, Julien Grall wrote: > On 17/04/2019 18:27, Stefano Stabellini wrote: > > On Wed, 17 Apr 2019, Julien Grall wrote: > > > Hi, > > > > > > On 17/04/2019 18:12, Stefano Stabellini wrote: > > > > On Tue, 16 Apr 2019, Julien Grall wrote: > > > > > Hi Stefano, > > > > > > > > > > On 4/16/19 10:51 PM, Stefano Stabellini wrote: > > > > > > On Mon, 28 Jan 2019, 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. The deactivation of the interrupt is done at the end of the > > > > > > > handling. > > > > > > > > > > > > > > This means the _IRQ_PENDING logic is unecessary on Arm as a same > > > > > > > interrupt can never come up while in the loop. So remove it to > > > > > > > simplify the interrupt handle code. > > > > > > > > > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > > > > --- > > > > > > > xen/arch/arm/irq.c | 32 ++++++++++---------------------- > > > > > > > 1 file changed, 10 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; > > > > > > > > > > > > It is a good idea to remove the IRQ_PENDING logic, that is OK. > > > > > > > > > > > > > > > > > > However, are we sure that we want to remove the _IRQ_INPROGRESS > > > > > > check > > > > > > too? IRQ handlers shouldn't be called twice in a row. Given that > > > > > > _IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it > > > > > > seems it > > > > > > would be a good idea to keep the check anyway? > > > > > > > > > > set_active_state is only used by the vGIC to replicate state from of > > > > > the > > > > > virtual interrupt to the physical interrupt. We don't have the virtual > > > > > interrupt in this path (see above). > > > > > > > > > > Any other user (e.g interrupts routed to Xen) would be pretty broken. > > > > > At > > > > > best > > > > > you would break the interrupt flow. At worst, you may never receive > > > > > the > > > > > interrupt again. > > > > > > > > > > So I think we can drop _IRQ_PROGRESS here. > > > > > > > > I gave it a close look. You are right, it is safe to remove the > > > > _IRQ_PROGRESS check here. > > > > > > > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > > > > > > > > > > > The thing that worries me a bit is that technically set_active_state is > > > > part of the gic_hw_operations functions which are not necessarily guest > > > > specific: we haven't written down anywhere that set_active_state cannot > > > > be called passing one of the xen irqs as parameter. I agree it would be > > > > broken to do so, but still... Maybe we should add a comment? > > > > > > How about adding an ASSERT(test_bit(_IRQ_GUEST, &desc->status)) ? > > > > even better > > Do you want the change to be in this patch or separately? In this patch please
Hi Stefano, On 5/20/19 10:04 PM, Stefano Stabellini wrote: > On Mon, 20 May 2019, Julien Grall wrote: >> On 17/04/2019 18:27, Stefano Stabellini wrote: >>> On Wed, 17 Apr 2019, Julien Grall wrote: >>>> Hi, >>>> >>>> On 17/04/2019 18:12, Stefano Stabellini wrote: >>>>> On Tue, 16 Apr 2019, Julien Grall wrote: >>>>>> Hi Stefano, >>>>>> >>>>>> On 4/16/19 10:51 PM, Stefano Stabellini wrote: >>>>>>> On Mon, 28 Jan 2019, 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. The deactivation of the interrupt is done at the end of the >>>>>>>> handling. >>>>>>>> >>>>>>>> This means the _IRQ_PENDING logic is unecessary on Arm as a same >>>>>>>> interrupt can never come up while in the loop. So remove it to >>>>>>>> simplify the interrupt handle code. >>>>>>>> >>>>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>>>>>> --- >>>>>>>> xen/arch/arm/irq.c | 32 ++++++++++---------------------- >>>>>>>> 1 file changed, 10 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; >>>>>>> >>>>>>> It is a good idea to remove the IRQ_PENDING logic, that is OK. >>>>>>> >>>>>>> >>>>>>> However, are we sure that we want to remove the _IRQ_INPROGRESS >>>>>>> check >>>>>>> too? IRQ handlers shouldn't be called twice in a row. Given that >>>>>>> _IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it >>>>>>> seems it >>>>>>> would be a good idea to keep the check anyway? >>>>>> >>>>>> set_active_state is only used by the vGIC to replicate state from of >>>>>> the >>>>>> virtual interrupt to the physical interrupt. We don't have the virtual >>>>>> interrupt in this path (see above). >>>>>> >>>>>> Any other user (e.g interrupts routed to Xen) would be pretty broken. >>>>>> At >>>>>> best >>>>>> you would break the interrupt flow. At worst, you may never receive >>>>>> the >>>>>> interrupt again. >>>>>> >>>>>> So I think we can drop _IRQ_PROGRESS here. >>>>> >>>>> I gave it a close look. You are right, it is safe to remove the >>>>> _IRQ_PROGRESS check here. >>>>> >>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>>>> >>>>> >>>>> The thing that worries me a bit is that technically set_active_state is >>>>> part of the gic_hw_operations functions which are not necessarily guest >>>>> specific: we haven't written down anywhere that set_active_state cannot >>>>> be called passing one of the xen irqs as parameter. I agree it would be >>>>> broken to do so, but still... Maybe we should add a comment? >>>> >>>> How about adding an ASSERT(test_bit(_IRQ_GUEST, &desc->status)) ? >>> >>> even better >> >> Do you want the change to be in this patch or separately? > > In this patch please Ok, I will respin the patch. Cheers,
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);
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. The deactivation of the interrupt is done at the end of the handling. This means the _IRQ_PENDING logic is unecessary on Arm as a same interrupt can never come up while in the loop. So remove it to simplify the interrupt handle code. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/irq.c | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-)