[PATCH 0/2] gic-vgic optimizations

[PATCH 0/2] gic-vgic optimizations

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

Here are few patches from RFC series [1] currently approved to
be upstreamed with appropriate changes.

Andrii Anisov (2):
  gic-vgic: Drop an excessive clear_lrs
        Is a patch #5 [2], with a change:
         - Keep LR clear for debug build

  arm/irq: skip action availability check for non-debug build
        Is a patch #11 [3], with a change:
         - Completely remove the check for a non-debug build,
           but preserve for debug.

[1] https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg03328.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg03285.html
[3] https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg03291.html


 xen/arch/arm/gic-vgic.c | 2 ++
 xen/arch/arm/irq.c      | 2 ++
 2 files changed, 4 insertions(+)

-- 
2.7.4


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

[PATCH 1/2] gic-vgic: Drop an excessive clear_lrs

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

This action is excessive because for an invalid LR there is no need
to write another invalid value to a register. So we can skip it here,
saving a peripheral register write.
Keep clearing the LR for the DEBUG build. This would make dumped
invalid LRs be zero. That is more obvious than picking state bits
from a non-zero value.

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-vgic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 990399c..48922f5 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -216,7 +216,9 @@ static void gic_update_one_lr(struct vcpu *v, int i)
     }
     else
     {
+#ifndef NDEBUG
         gic_hw_ops->clear_lr(i);
+#endif
         clear_bit(i, &this_cpu(lr_mask));
 
         if ( p->desc != NULL )
-- 
2.7.4


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

[PATCH 2/2] arm/irq: skip action avalability check for non-debug build

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

An IRQ with _IRQ_GUEST flag set always has an action.
An IRQ with _IRQ_DISABLED flag cleared always have an action.
Those flags checks cover all accesses to desc->action in do_IRQ,
so we can skip desc->action check.
Still keep it in place for debug build.

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index d6a0273..4a02cc1 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -209,12 +209,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
     spin_lock(&desc->lock);
     desc->handler->ack(desc);
 
+#ifndef NDEBUG
     if ( !desc->action )
     {
         printk("Unknown %s %#3.3x\n",
                is_fiq ? "FIQ" : "IRQ", irq);
         goto out;
     }
+#endif
 
     if ( test_bit(_IRQ_GUEST, &desc->status) )
     {
-- 
2.7.4


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

Re: [PATCH 2/2] arm/irq: skip action avalability check for non-debug build

[Julien Grall]

Title: s/avalability/availability/

On 12/12/2018 16:55, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> An IRQ with _IRQ_GUEST flag set always has an action.
> An IRQ with _IRQ_DISABLED flag cleared always have an action.

s/have/has/

Those conditions are not sufficient to ensure desc->action is not NULL. You also 
need to take the spinlock.

While looking at the code, I noticed an interesting race with the release code. 
Guest IRQ are released using the function gic_remove_irq_to_guest. The sequence 
is roughly:

1) spin_lock(desc->lock);
2) writel(desc->irq, ICENABLER);
3) set_bit(_IRQ_DISABLED, &desc->status);
4) clear_bit(_IRQ_GUES, &desc->status);
5) desc->handler = &no_irq_type;
6) spin_unlock(desc->lock);

Even if 2) will disable the interrupt in the hardware, the interrupt may have 
been received earlier on another CPU and waiting on the lock. As soon as the 
lock is taken, the code will notice the irq disabled (thanks to 3)) and will 
then end the interrupt. The callbak end for no_irq_type is a NOP, therefore the 
interrupt will stay active and the priority will not be dropped.

Because of that, the CPU will never be able to receive interrupt for guest 
anymore. AFAICT, this can only happen if an interrupt is received while 
destroying the assigned domain.

I think 5) should be replaced with

desc->handler = gic_hw_ops->gic_host_irq_type;

Or we potentially need to update no_irq_type and EOI "spurious interrupt".

I am not entirely sure which way is the best to address the race. Any opinions?


> Those flags checks cover all accesses to desc->action in do_IRQ, > so we can skip desc->action check.

"in non-debug build".

> Still keep it in place for debug build.

"Keep in place for debug build to help diagnostics potential misconfiguration".

> 
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> Reviewed-by: Julien Grall <julien.grall@arm.com>

Please don't add a reviewed-by tag until it was explicitly written by the reviewer.

> ---
>   xen/arch/arm/irq.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index d6a0273..4a02cc1 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -209,12 +209,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>       spin_lock(&desc->lock);
>       desc->handler->ack(desc);
>   
> +#ifndef NDEBUG
>       if ( !desc->action )
>       {
>           printk("Unknown %s %#3.3x\n",
>                  is_fiq ? "FIQ" : "IRQ", irq);
>           goto out;
>       }
> +#endif
>   
>       if ( 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 2/2] arm/irq: skip action avalability check for non-debug build

[Andrii Anisov]

On 12.12.18 19:49, Julien Grall wrote:

> Please don't add a reviewed-by tag until it was explicitly written by the reviewer.
My bad, I mixed it with #5.


-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH 2/2] arm/irq: skip action avalability check for non-debug build

[Andrii Anisov]

On 12.12.18 19:49, Julien Grall wrote:
> Those conditions are not sufficient to ensure desc->action is not NULL. You also need to take the spinlock.
Agree. Should I describe it as following?

>> Under desc->lock taken:
>> An IRQ with _IRQ_GUEST flag set always has an action.
>> An IRQ with _IRQ_DISABLED flag cleared always have an action.


> While looking at the code, I noticed an interesting race with the release code.
As I understand the race in not directly linked to this patch. Is it correct?

> Guest IRQ are released using the function gic_remove_irq_to_guest. The sequence is roughly:
> 
> 1) spin_lock(desc->lock);
> 2) writel(desc->irq, ICENABLER);
> 3) set_bit(_IRQ_DISABLED, &desc->status);
> 4) clear_bit(_IRQ_GUES, &desc->status);
> 5) desc->handler = &no_irq_type;
> 6) spin_unlock(desc->lock);
> 
> Even if 2) will disable the interrupt in the hardware, the interrupt may have been received earlier on another CPU and waiting on the lock. As soon as the lock is taken, the code will notice the irq disabled (thanks to 3)) and will then end the interrupt. The callbak end for no_irq_type is a NOP, therefore the interrupt will stay active and the priority will not be dropped.
> 
> Because of that, the CPU will never be able to receive interrupt for guest anymore. AFAICT, this can only happen if an interrupt is received while destroying the assigned domain.
> 
> I think 5) should be replaced with
> 
> desc->handler = gic_hw_ops->gic_host_irq_type;
> 
> Or we potentially need to update no_irq_type and EOI "spurious interrupt".
> 
> I am not entirely sure which way is the best to address the race. Any opinions?
Let me spend a bit more time to look into that

Other wording and grammatical nits will be addressed as suggested.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH 2/2] arm/irq: skip action avalability check for non-debug build

[Julien Grall]

On 12/12/2018 17:59, Andrii Anisov wrote:
> On 12.12.18 19:49, Julien Grall wrote:
>> Those conditions are not sufficient to ensure desc->action is not NULL. You 
>> also need to take the spinlock.
> Agree. Should I describe it as following?
> 
>>> Under desc->lock taken:
>>> An IRQ with _IRQ_GUEST flag set always has an action.
>>> An IRQ with _IRQ_DISABLED flag cleared always have an action.

This looks better.

> 
> 
>> While looking at the code, I noticed an interesting race with the release code.
> As I understand the race in not directly linked to this patch. Is it correct?

That's correct. I actually noticed when checking whether the commit message was 
matching the behavior.

> 
>> Guest IRQ are released using the function gic_remove_irq_to_guest. The 
>> sequence is roughly:
>>
>> 1) spin_lock(desc->lock);
>> 2) writel(desc->irq, ICENABLER);
>> 3) set_bit(_IRQ_DISABLED, &desc->status);
>> 4) clear_bit(_IRQ_GUES, &desc->status);
>> 5) desc->handler = &no_irq_type;
>> 6) spin_unlock(desc->lock);
>>
>> Even if 2) will disable the interrupt in the hardware, the interrupt may have 
>> been received earlier on another CPU and waiting on the lock. As soon as the 
>> lock is taken, the code will notice the irq disabled (thanks to 3)) and will 
>> then end the interrupt. The callbak end for no_irq_type is a NOP, therefore 
>> the interrupt will stay active and the priority will not be dropped.
>>
>> Because of that, the CPU will never be able to receive interrupt for guest 
>> anymore. AFAICT, this can only happen if an interrupt is received while 
>> destroying the assigned domain.
>>
>> I think 5) should be replaced with
>>
>> desc->handler = gic_hw_ops->gic_host_irq_type;
>>
>> Or we potentially need to update no_irq_type and EOI "spurious interrupt".
>>
>> I am not entirely sure which way is the best to address the race. Any opinions?
> Let me spend a bit more time to look into that
> 
> Other wording and grammatical nits will be addressed as suggested.
> 

-- 
Julien Grall

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

Re: [PATCH 2/2] arm/irq: skip action avalability check for non-debug build

[Stefano Stabellini]

On Wed, 12 Dec 2018, Julien Grall wrote:
> Title: s/avalability/availability/
> 
> On 12/12/2018 16:55, Andrii Anisov wrote:
> > From: Andrii Anisov <andrii_anisov@epam.com>
> > 
> > An IRQ with _IRQ_GUEST flag set always has an action.
> > An IRQ with _IRQ_DISABLED flag cleared always have an action.
> 
> s/have/has/
> 
> Those conditions are not sufficient to ensure desc->action is not NULL. You
> also need to take the spinlock.
> 
> While looking at the code, I noticed an interesting race with the release
> code. Guest IRQ are released using the function gic_remove_irq_to_guest. The
> sequence is roughly:
> 
> 1) spin_lock(desc->lock);
> 2) writel(desc->irq, ICENABLER);
> 3) set_bit(_IRQ_DISABLED, &desc->status);
> 4) clear_bit(_IRQ_GUES, &desc->status);
> 5) desc->handler = &no_irq_type;
> 6) spin_unlock(desc->lock);
> 
> Even if 2) will disable the interrupt in the hardware, the interrupt may have
> been received earlier on another CPU and waiting on the lock. As soon as the
> lock is taken, the code will notice the irq disabled (thanks to 3)) and will
> then end the interrupt. The callbak end for no_irq_type is a NOP, therefore
> the interrupt will stay active and the priority will not be dropped.
> 
> Because of that, the CPU will never be able to receive interrupt for guest
> anymore. AFAICT, this can only happen if an interrupt is received while
> destroying the assigned domain.
> 
> I think 5) should be replaced with
> 
> desc->handler = gic_hw_ops->gic_host_irq_type;
> 
> Or we potentially need to update no_irq_type and EOI "spurious interrupt".
> 
> I am not entirely sure which way is the best to address the race. Any
> opinions?

I think that changing the .end function of no_irq_type to be the same as
the end function of the host_irq_type controller is the safest option:
yes no_irq_type means no irqs but if we receive an interrupt we should
still EOI it no matter what.

 
> > Those flags checks cover all accesses to desc->action in do_IRQ, > so we can
> > skip desc->action check.
> 
> "in non-debug build".
> 
> > Still keep it in place for debug build.
> 
> "Keep in place for debug build to help diagnostics potential
> misconfiguration".
> 
> > 
> > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> > Reviewed-by: Julien Grall <julien.grall@arm.com>
> 
> Please don't add a reviewed-by tag until it was explicitly written by the
> reviewer.
> 
> > ---
> >   xen/arch/arm/irq.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index d6a0273..4a02cc1 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -209,12 +209,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int
> > irq, int is_fiq)
> >       spin_lock(&desc->lock);
> >       desc->handler->ack(desc);
> >   +#ifndef NDEBUG
> >       if ( !desc->action )
> >       {
> >           printk("Unknown %s %#3.3x\n",
> >                  is_fiq ? "FIQ" : "IRQ", irq);
> >           goto out;
> >       }
> > +#endif
> >         if ( 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