xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
@ 2019-06-02 10:26 Julien Grall
  2019-06-02 10:26 ` [Xen-devel] " Julien Grall
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Julien Grall @ 2019-06-02 10:26 UTC (permalink / raw)
  To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini, andrii_anisov

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
  2019-06-02 10:26 [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt Julien Grall
@ 2019-06-02 10:26 ` Julien Grall
  2019-06-10 11:23 ` Andrii Anisov
  2019-06-26 10:36 ` Julien Grall
  2 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2019-06-02 10:26 UTC (permalink / raw)
  To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini, andrii_anisov

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
  2019-06-02 10:26 [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt Julien Grall
  2019-06-02 10:26 ` [Xen-devel] " Julien Grall
@ 2019-06-10 11:23 ` Andrii Anisov
  2019-06-10 19:00   ` Julien Grall
  2019-06-26 10:36 ` Julien Grall
  2 siblings, 1 reply; 6+ messages in thread
From: Andrii Anisov @ 2019-06-10 11:23 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: andre.przywara, sstabellini, andrii_anisov

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
  2019-06-10 11:23 ` Andrii Anisov
@ 2019-06-10 19:00   ` Julien Grall
  2019-07-29 21:01     ` Stefano Stabellini
  0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2019-06-10 19:00 UTC (permalink / raw)
  To: Andrii Anisov, xen-devel; +Cc: andre.przywara, sstabellini, andrii_anisov



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
  2019-06-02 10:26 [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt Julien Grall
  2019-06-02 10:26 ` [Xen-devel] " Julien Grall
  2019-06-10 11:23 ` Andrii Anisov
@ 2019-06-26 10:36 ` Julien Grall
  2 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2019-06-26 10:36 UTC (permalink / raw)
  To: xen-devel; +Cc: andre.przywara, sstabellini, andrii_anisov

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt
  2019-06-10 19:00   ` Julien Grall
@ 2019-07-29 21:01     ` Stefano Stabellini
  0 siblings, 0 replies; 6+ messages in thread
From: Stefano Stabellini @ 2019-07-29 21:01 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, andre.przywara, sstabellini, andrii_anisov, Andrii Anisov

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-07-29 21:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-02 10:26 [PATCH v2] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt Julien Grall
2019-06-02 10:26 ` [Xen-devel] " Julien Grall
2019-06-10 11:23 ` Andrii Anisov
2019-06-10 19:00   ` Julien Grall
2019-07-29 21:01     ` Stefano Stabellini
2019-06-26 10:36 ` Julien Grall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).