[PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Julien Grall]

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);
 
-- 
2.11.0


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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Julien Grall]

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);
>   
> 

-- 
Julien Grall

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Andrii Anisov]

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);
>   
> 

-- 
Sincerely,
Andrii Anisov.

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

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

[Andrii Anisov]

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);
>   
> 

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Julien Grall]

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,

-- 
Julien Grall

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Andrii Anisov]

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>

-- 
Sincerely,
Andrii Anisov.

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

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

[Andrii Anisov]

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>

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Stefano Stabellini]

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
> 

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

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

[Stefano Stabellini]

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
> 

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Julien Grall]

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,

-- 
Julien Grall

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Stefano Stabellini]

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?

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

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

[Stefano Stabellini]

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?

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Julien Grall]

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,

-- 
Julien Grall

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Stefano Stabellini]

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

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

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

[Stefano Stabellini]

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

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Julien Grall]

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,

-- 
Julien Grall

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Stefano Stabellini]

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

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

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

[Stefano Stabellini]

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

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

Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Julien Grall]

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,

-- 
Julien Grall

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

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

[Julien Grall]

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,

-- 
Julien Grall

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