[Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

Julien Grall posted 1 patch 4 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20190602102614.22640-1-julien.grall@arm.com
xen/arch/arm/irq.c        | 32 ++++++++++----------------------
xen/include/asm-arm/gic.h |  4 ++++
2 files changed, 14 insertions(+), 22 deletions(-)
[Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
Posted by Julien Grall 4 years, 9 months ago
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);
 }
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
Posted by Andrii Anisov 4 years, 9 months ago
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>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
Posted by Julien Grall 4 years, 9 months ago

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
Posted by Stefano Stabellini 4 years, 8 months ago
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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
Posted by Julien Grall 4 years, 9 months ago
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);
>   }
>   
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel