[PATCH v2] gic: drop interrupts enabling on interrupts processing

[PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

This reduces the number of context switches in case we have coming guest
interrupts from different sources at a high rate. What is likely for
multimedia use-cases.
Having irqs unlocked here makes us go through trap path again in case we
have a new guest interrupt arrived (even with the same priority, after
`desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
cycles wasting. We will catch them all in the `gic_interrupt() function
loop anyway. And the guest irqs arrival prioritization is meaningless
here, it is only effective at guest's level.

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---

Changes:

    in v2: Drop irq enabling for lpi processing as well.


---
 xen/arch/arm/gic.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6cc7dec..113655a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
 
         if ( likely(irq >= 16 && irq < 1020) )
         {
-            local_irq_enable();
             isb();
             do_IRQ(regs, irq, is_fiq);
-            local_irq_disable();
         }
         else if ( is_lpi(irq) )
         {
-            local_irq_enable();
             isb();
             gic_hw_ops->do_LPI(irq);
-            local_irq_disable();
         }
         else if ( unlikely(irq < 16) )
         {
-- 
2.7.4


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

[Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

This reduces the number of context switches in case we have coming guest
interrupts from different sources at a high rate. What is likely for
multimedia use-cases.
Having irqs unlocked here makes us go through trap path again in case we
have a new guest interrupt arrived (even with the same priority, after
`desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
cycles wasting. We will catch them all in the `gic_interrupt() function
loop anyway. And the guest irqs arrival prioritization is meaningless
here, it is only effective at guest's level.

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---

Changes:

    in v2: Drop irq enabling for lpi processing as well.


---
 xen/arch/arm/gic.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6cc7dec..113655a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
 
         if ( likely(irq >= 16 && irq < 1020) )
         {
-            local_irq_enable();
             isb();
             do_IRQ(regs, irq, is_fiq);
-            local_irq_disable();
         }
         else if ( is_lpi(irq) )
         {
-            local_irq_enable();
             isb();
             gic_hw_ops->do_LPI(irq);
-            local_irq_disable();
         }
         else if ( unlikely(irq < 16) )
         {
-- 
2.7.4


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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

(+ Andre)

Hi,

Title: Interrupts are still unmasked when executing action for interrupt 
routed to Xen. So you need to be more specific. How about
"xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()"?

On 5/27/19 10:29 AM, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> This reduces the number of context switches in case we have coming guest
> interrupts from different sources at a high rate. What is likely for

s/What/This/

> multimedia use-cases.
> Having irqs unlocked here makes us go through trap path again in case we

what do you mean by "unlocked"?

> have a new guest interrupt arrived (even with the same priority, after
> `desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
> cycles wasting.
after `desc->....`. This is just a waste a processor cycle as we will 
catch them all in the function gic_interrupt() loop.

  We will catch them all in the `gic_interrupt() function
> loop anyway. And the guest irqs arrival prioritization is meaningless
> here, it is only effective at guest's level.

I am not sure why you speak about guest prioritization here. The main 
issue would be an interrupt to Xen (i.e timer) that would get delayed 
because of longer period without interrupt enabled. I would also not 
rule out the possibility to prioritize guest interrupt at hardware level.

I know we have been discussing on the problem in the past, but a summary 
in the commit message is quite important to not miss out all the problems.

The real problem here is for interrupt routed to guest the interrupt 
will be kept unmasked when calling desc->handler->end(desc). This will 
result to receive the next interrupt as soon as desc->handler->end(desc) 
is called.

In the case of interrupt routed to Xen, interrupts will be kept enabled 
while executing the action but then disabled before calling 
desc->handler->end(desc).

It would be fine to keep the interrupts masked for interrupts routed to 
the guest because vgic_inject_irq(...) will be masking the interrupt in 
most of the cases.

The code below looks good to me. I am happy to help rewording the commit 
message if necessary.

While looking at the code, I noticed that in the new vgic vgic_get_irq() 
looks unsafe to be called with interrupt unmasked. This is because one 
of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq. 
Andre, what do you think?

> 
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
> 
> Changes:
> 
>      in v2: Drop irq enabling for lpi processing as well.
> 
> 
> ---
>   xen/arch/arm/gic.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6cc7dec..113655a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>   
>           if ( likely(irq >= 16 && irq < 1020) )
>           {
> -            local_irq_enable();
>               isb();
>               do_IRQ(regs, irq, is_fiq);
> -            local_irq_disable();
>           }
>           else if ( is_lpi(irq) )
>           {
> -            local_irq_enable();
>               isb();
>               gic_hw_ops->do_LPI(irq);
> -            local_irq_disable();
>           }
>           else if ( unlikely(irq < 16) )
>           {
> 

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

(+ Andre)

Hi,

Title: Interrupts are still unmasked when executing action for interrupt 
routed to Xen. So you need to be more specific. How about
"xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()"?

On 5/27/19 10:29 AM, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> This reduces the number of context switches in case we have coming guest
> interrupts from different sources at a high rate. What is likely for

s/What/This/

> multimedia use-cases.
> Having irqs unlocked here makes us go through trap path again in case we

what do you mean by "unlocked"?

> have a new guest interrupt arrived (even with the same priority, after
> `desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
> cycles wasting.
after `desc->....`. This is just a waste a processor cycle as we will 
catch them all in the function gic_interrupt() loop.

  We will catch them all in the `gic_interrupt() function
> loop anyway. And the guest irqs arrival prioritization is meaningless
> here, it is only effective at guest's level.

I am not sure why you speak about guest prioritization here. The main 
issue would be an interrupt to Xen (i.e timer) that would get delayed 
because of longer period without interrupt enabled. I would also not 
rule out the possibility to prioritize guest interrupt at hardware level.

I know we have been discussing on the problem in the past, but a summary 
in the commit message is quite important to not miss out all the problems.

The real problem here is for interrupt routed to guest the interrupt 
will be kept unmasked when calling desc->handler->end(desc). This will 
result to receive the next interrupt as soon as desc->handler->end(desc) 
is called.

In the case of interrupt routed to Xen, interrupts will be kept enabled 
while executing the action but then disabled before calling 
desc->handler->end(desc).

It would be fine to keep the interrupts masked for interrupts routed to 
the guest because vgic_inject_irq(...) will be masking the interrupt in 
most of the cases.

The code below looks good to me. I am happy to help rewording the commit 
message if necessary.

While looking at the code, I noticed that in the new vgic vgic_get_irq() 
looks unsafe to be called with interrupt unmasked. This is because one 
of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq. 
Andre, what do you think?

> 
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
> 
> Changes:
> 
>      in v2: Drop irq enabling for lpi processing as well.
> 
> 
> ---
>   xen/arch/arm/gic.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6cc7dec..113655a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>   
>           if ( likely(irq >= 16 && irq < 1020) )
>           {
> -            local_irq_enable();
>               isb();
>               do_IRQ(regs, irq, is_fiq);
> -            local_irq_disable();
>           }
>           else if ( is_lpi(irq) )
>           {
> -            local_irq_enable();
>               isb();
>               gic_hw_ops->do_LPI(irq);
> -            local_irq_disable();
>           }
>           else if ( unlikely(irq < 16) )
>           {
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

Hello Julien,

On 28.05.19 20:07, Julien Grall wrote:
> Title: Interrupts are still unmasked when executing action for interrupt routed to Xen. So you need to be more specific. How about
> "xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()"?

Looks good.

> 
> On 5/27/19 10:29 AM, Andrii Anisov wrote:
>> From: Andrii Anisov <andrii_anisov@epam.com>
>>
>> This reduces the number of context switches in case we have coming guest
>> interrupts from different sources at a high rate. What is likely for
> 
> s/What/This/
> 
>> multimedia use-cases.
>> Having irqs unlocked here makes us go through trap path again in case we
> 
> what do you mean by "unlocked"?

It must be "enabled".

>> have a new guest interrupt arrived (even with the same priority, after
>> `desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
>> cycles wasting.
> after `desc->....`. This is just a waste a processor cycle as we will catch them all in the function gic_interrupt() loop.
> 
>   We will catch them all in the `gic_interrupt() function
>> loop anyway. And the guest irqs arrival prioritization is meaningless
>> here, it is only effective at guest's level.
> 
> I am not sure why you speak about guest prioritization here.

I'm trying to say about guest interrupts prioritization in HW. But I can drop it from the commit message.

> The main issue would be an interrupt to Xen (i.e timer) that would get delayed because of longer period without interrupt enabled.

Here we will process it on the next loop. This should not be much longer than existing vgic_inject_irq() interrupts disabled period.

> I would also not rule out the possibility to prioritize guest interrupt at hardware level.> 
> I know we have been discussing on the problem in the past,

Now I'm trying to pick the worthy bits from [1].
BTW, do you hear about plans for the new vgic? Some time ago it was said that new vgic implementation going to replace the old one, and optimizing the old is worthless. But as I see, there are no updates into that area yet.

> but a summary in the commit message is quite important to not miss out all the problems.

> The real problem here is for interrupt routed to guest the interrupt will be kept unmasked when calling desc->handler->end(desc). This will result to receive the next interrupt as soon as desc->handler->end(desc) is called.
> 
> In the case of interrupt routed to Xen, interrupts will be kept enabled while executing the action but then disabled before calling desc->handler->end(desc).
> 
> It would be fine to keep the interrupts masked for interrupts routed to the guest because vgic_inject_irq(...) will be masking the interrupt in most of the cases.
> 
> The code below looks good to me. I am happy to help rewording the commit message if necessary.

It's good to hear. I'm ready to reword the commit message as required to get the stuff upstreamed.
I'd discuss the wordings here. With changes suggested by you, the commit title and message would be following:

     xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()

     This reduces the number of context switches in case we have coming guest
     interrupts from different sources at a high rate. That is likely for
     multimedia use-cases.
     Having irqs enabled here makes us go through trap path again in case we
     have a new guest interrupt arrived (even with the same or lower priority,
     after `desc->handler->end(desc)` in `do_IRQ()`), that is just a processor
     cycles wasting as we will catch them all in the `gic_interrupt() function
     loop.

[1] https://lists.xenproject.org/archives/html/xen-devel/2018-12/msg02297.html

-- 
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

Hello Julien,

On 28.05.19 20:07, Julien Grall wrote:
> Title: Interrupts are still unmasked when executing action for interrupt routed to Xen. So you need to be more specific. How about
> "xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()"?

Looks good.

> 
> On 5/27/19 10:29 AM, Andrii Anisov wrote:
>> From: Andrii Anisov <andrii_anisov@epam.com>
>>
>> This reduces the number of context switches in case we have coming guest
>> interrupts from different sources at a high rate. What is likely for
> 
> s/What/This/
> 
>> multimedia use-cases.
>> Having irqs unlocked here makes us go through trap path again in case we
> 
> what do you mean by "unlocked"?

It must be "enabled".

>> have a new guest interrupt arrived (even with the same priority, after
>> `desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
>> cycles wasting.
> after `desc->....`. This is just a waste a processor cycle as we will catch them all in the function gic_interrupt() loop.
> 
>   We will catch them all in the `gic_interrupt() function
>> loop anyway. And the guest irqs arrival prioritization is meaningless
>> here, it is only effective at guest's level.
> 
> I am not sure why you speak about guest prioritization here.

I'm trying to say about guest interrupts prioritization in HW. But I can drop it from the commit message.

> The main issue would be an interrupt to Xen (i.e timer) that would get delayed because of longer period without interrupt enabled.

Here we will process it on the next loop. This should not be much longer than existing vgic_inject_irq() interrupts disabled period.

> I would also not rule out the possibility to prioritize guest interrupt at hardware level.> 
> I know we have been discussing on the problem in the past,

Now I'm trying to pick the worthy bits from [1].
BTW, do you hear about plans for the new vgic? Some time ago it was said that new vgic implementation going to replace the old one, and optimizing the old is worthless. But as I see, there are no updates into that area yet.

> but a summary in the commit message is quite important to not miss out all the problems.

> The real problem here is for interrupt routed to guest the interrupt will be kept unmasked when calling desc->handler->end(desc). This will result to receive the next interrupt as soon as desc->handler->end(desc) is called.
> 
> In the case of interrupt routed to Xen, interrupts will be kept enabled while executing the action but then disabled before calling desc->handler->end(desc).
> 
> It would be fine to keep the interrupts masked for interrupts routed to the guest because vgic_inject_irq(...) will be masking the interrupt in most of the cases.
> 
> The code below looks good to me. I am happy to help rewording the commit message if necessary.

It's good to hear. I'm ready to reword the commit message as required to get the stuff upstreamed.
I'd discuss the wordings here. With changes suggested by you, the commit title and message would be following:

     xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()

     This reduces the number of context switches in case we have coming guest
     interrupts from different sources at a high rate. That is likely for
     multimedia use-cases.
     Having irqs enabled here makes us go through trap path again in case we
     have a new guest interrupt arrived (even with the same or lower priority,
     after `desc->handler->end(desc)` in `do_IRQ()`), that is just a processor
     cycles wasting as we will catch them all in the `gic_interrupt() function
     loop.

[1] https://lists.xenproject.org/archives/html/xen-devel/2018-12/msg02297.html

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

On 29/05/2019 11:31, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> On 28.05.19 20:07, Julien Grall wrote:
>> Title: Interrupts are still unmasked when executing action for interrupt 
>> routed to Xen. So you need to be more specific. How about
>> "xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()"?
> 
> Looks good.
> 
>>
>> On 5/27/19 10:29 AM, Andrii Anisov wrote:
>>> From: Andrii Anisov <andrii_anisov@epam.com>
>>>
>>> This reduces the number of context switches in case we have coming guest
>>> interrupts from different sources at a high rate. What is likely for
>>
>> s/What/This/
>>
>>> multimedia use-cases.
>>> Having irqs unlocked here makes us go through trap path again in case we
>>
>> what do you mean by "unlocked"?
> 
> It must be "enabled".
> 
>>> have a new guest interrupt arrived (even with the same priority, after
>>> `desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
>>> cycles wasting.
>> after `desc->....`. This is just a waste a processor cycle as we will catch 
>> them all in the function gic_interrupt() loop.
>>
>>   We will catch them all in the `gic_interrupt() function
>>> loop anyway. And the guest irqs arrival prioritization is meaningless
>>> here, it is only effective at guest's level.
>>
>> I am not sure why you speak about guest prioritization here.
> 
> I'm trying to say about guest interrupts prioritization in HW. But I can drop it 
> from the commit message.
> 
>> The main issue would be an interrupt to Xen (i.e timer) that would get delayed 
>> because of longer period without interrupt enabled.
> 
> Here we will process it on the next loop. This should not be much longer than 
> existing vgic_inject_irq() interrupts disabled period.

This should be explained in the commit message.

> 
>> I would also not rule out the possibility to prioritize guest interrupt at 
>> hardware level.> I know we have been discussing on the problem in the past,
> 
> Now I'm trying to pick the worthy bits from [1].
> BTW, do you hear about plans for the new vgic? Some time ago it was said that 
> new vgic implementation going to replace the old one, and optimizing the old is 
> worthless. But as I see, there are no updates into that area yet.

We need help to make it happen.

> 
>> but a summary in the commit message is quite important to not miss out all the 
>> problems.
> 
>> The real problem here is for interrupt routed to guest the interrupt will be 
>> kept unmasked when calling desc->handler->end(desc). This will result to 
>> receive the next interrupt as soon as desc->handler->end(desc) is called.
>>
>> In the case of interrupt routed to Xen, interrupts will be kept enabled while 
>> executing the action but then disabled before calling desc->handler->end(desc).
>>
>> It would be fine to keep the interrupts masked for interrupts routed to the 
>> guest because vgic_inject_irq(...) will be masking the interrupt in most of 
>> the cases.
>>
>> The code below looks good to me. I am happy to help rewording the commit 
>> message if necessary.
> 
> It's good to hear. I'm ready to reword the commit message as required to get the 
> stuff upstreamed.
> I'd discuss the wordings here. With changes suggested by you, the commit title 
> and message would be following:

It would have been nice to at least fix up the commit message with the typoes 
(and rewording) I mentioned in my previous e-mail.

> 
>      xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()
> 
>      This reduces the number of context switches in case we have coming guest

context switches is a bit confusing here. Do you mean trap?

Also s/coming/incoming/ or better "in case guest interrupts are received at high 
rate".

>      interrupts from different sources at a high rate. That is likely for
>      multimedia use-cases.
>      Having irqs enabled here makes us go through trap path again in case we
>      have a new guest interrupt arrived (even with the same or lower priority,
>      after `desc->handler->end(desc)` in `do_IRQ()`), that is just a processor
>      cycles wasting as we will catch them all in the `gic_interrupt() function
>      loop.

Your commit message needs to explained why this is fine to keep the interrupt 
masked a bit longer. I wrote the explanation in my previous e-mail so you can 
borrow the rationale from there.

Cheers,

> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2018-12/msg02297.html
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

On 29/05/2019 11:31, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> On 28.05.19 20:07, Julien Grall wrote:
>> Title: Interrupts are still unmasked when executing action for interrupt 
>> routed to Xen. So you need to be more specific. How about
>> "xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()"?
> 
> Looks good.
> 
>>
>> On 5/27/19 10:29 AM, Andrii Anisov wrote:
>>> From: Andrii Anisov <andrii_anisov@epam.com>
>>>
>>> This reduces the number of context switches in case we have coming guest
>>> interrupts from different sources at a high rate. What is likely for
>>
>> s/What/This/
>>
>>> multimedia use-cases.
>>> Having irqs unlocked here makes us go through trap path again in case we
>>
>> what do you mean by "unlocked"?
> 
> It must be "enabled".
> 
>>> have a new guest interrupt arrived (even with the same priority, after
>>> `desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
>>> cycles wasting.
>> after `desc->....`. This is just a waste a processor cycle as we will catch 
>> them all in the function gic_interrupt() loop.
>>
>>   We will catch them all in the `gic_interrupt() function
>>> loop anyway. And the guest irqs arrival prioritization is meaningless
>>> here, it is only effective at guest's level.
>>
>> I am not sure why you speak about guest prioritization here.
> 
> I'm trying to say about guest interrupts prioritization in HW. But I can drop it 
> from the commit message.
> 
>> The main issue would be an interrupt to Xen (i.e timer) that would get delayed 
>> because of longer period without interrupt enabled.
> 
> Here we will process it on the next loop. This should not be much longer than 
> existing vgic_inject_irq() interrupts disabled period.

This should be explained in the commit message.

> 
>> I would also not rule out the possibility to prioritize guest interrupt at 
>> hardware level.> I know we have been discussing on the problem in the past,
> 
> Now I'm trying to pick the worthy bits from [1].
> BTW, do you hear about plans for the new vgic? Some time ago it was said that 
> new vgic implementation going to replace the old one, and optimizing the old is 
> worthless. But as I see, there are no updates into that area yet.

We need help to make it happen.

> 
>> but a summary in the commit message is quite important to not miss out all the 
>> problems.
> 
>> The real problem here is for interrupt routed to guest the interrupt will be 
>> kept unmasked when calling desc->handler->end(desc). This will result to 
>> receive the next interrupt as soon as desc->handler->end(desc) is called.
>>
>> In the case of interrupt routed to Xen, interrupts will be kept enabled while 
>> executing the action but then disabled before calling desc->handler->end(desc).
>>
>> It would be fine to keep the interrupts masked for interrupts routed to the 
>> guest because vgic_inject_irq(...) will be masking the interrupt in most of 
>> the cases.
>>
>> The code below looks good to me. I am happy to help rewording the commit 
>> message if necessary.
> 
> It's good to hear. I'm ready to reword the commit message as required to get the 
> stuff upstreamed.
> I'd discuss the wordings here. With changes suggested by you, the commit title 
> and message would be following:

It would have been nice to at least fix up the commit message with the typoes 
(and rewording) I mentioned in my previous e-mail.

> 
>      xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()
> 
>      This reduces the number of context switches in case we have coming guest

context switches is a bit confusing here. Do you mean trap?

Also s/coming/incoming/ or better "in case guest interrupts are received at high 
rate".

>      interrupts from different sources at a high rate. That is likely for
>      multimedia use-cases.
>      Having irqs enabled here makes us go through trap path again in case we
>      have a new guest interrupt arrived (even with the same or lower priority,
>      after `desc->handler->end(desc)` in `do_IRQ()`), that is just a processor
>      cycles wasting as we will catch them all in the `gic_interrupt() function
>      loop.

Your commit message needs to explained why this is fine to keep the interrupt 
masked a bit longer. I wrote the explanation in my previous e-mail so you can 
borrow the rationale from there.

Cheers,

> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2018-12/msg02297.html
> 

-- 
Julien Grall

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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

On 29.05.19 18:32, Julien Grall wrote:
> It would have been nice to at least fix up the commit message with the typoes (and rewording) I mentioned in my previous e-mail.
> Your commit message needs to explained why this is fine to keep the interrupt masked a bit longer. I wrote the explanation in my previous e-mail so you can borrow the rationale from there.
xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()

Having irqs enabled here leaves a room for trapping and going through the trap
path again if we have a new guest interrupt arrived (even with the same or
lower priority, after `desc->handler->end(desc)` in `do_IRQ()`).
Keeping interrupts disabled during guest interrupts processing allows as
avoiding excessive traps (and wasting cpu cycles for trap path) while the new
interrupts would be processed in the loop anyway. Processing guest interrupts by
the loop should not introduce significant additional latency because
vgic_inject_irq(...) already masking the interrupts in most of the cases.


-- 
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

On 29.05.19 18:32, Julien Grall wrote:
> It would have been nice to at least fix up the commit message with the typoes (and rewording) I mentioned in my previous e-mail.
> Your commit message needs to explained why this is fine to keep the interrupt masked a bit longer. I wrote the explanation in my previous e-mail so you can borrow the rationale from there.
xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()

Having irqs enabled here leaves a room for trapping and going through the trap
path again if we have a new guest interrupt arrived (even with the same or
lower priority, after `desc->handler->end(desc)` in `do_IRQ()`).
Keeping interrupts disabled during guest interrupts processing allows as
avoiding excessive traps (and wasting cpu cycles for trap path) while the new
interrupts would be processed in the loop anyway. Processing guest interrupts by
the loop should not introduce significant additional latency because
vgic_inject_irq(...) already masking the interrupts in most of the cases.


-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

Hi Andrii,

On 30/05/2019 17:12, Andrii Anisov wrote:
> On 29.05.19 18:32, Julien Grall wrote:
>> It would have been nice to at least fix up the commit message with the typoes 
>> (and rewording) I mentioned in my previous e-mail.
>> Your commit message needs to explained why this is fine to keep the interrupt 
>> masked a bit longer. I wrote the explanation in my previous e-mail so you can 
>> borrow the rationale from there.
> xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()
> 
> Having irqs enabled here leaves a room for trapping and going through the trap

Please avoid "here" in commit message if you haven't defined where is the issue.

> path again if we have a new guest interrupt arrived (even with the same or

I don't understand the "new guest interrupt arrived".

> lower priority, after `desc->handler->end(desc)` in `do_IRQ()`).
> Keeping interrupts disabled during guest interrupts processing allows as

Missing word because "allows" and "as"?

> avoiding excessive traps (and wasting cpu cycles for trap path) while the new
> interrupts would be processed in the loop anyway. Processing guest interrupts by
> the loop should not introduce significant additional latency because

I am always worry when I see the word "should not" associated with "latency" 
because often it is actually the contrary (see the recent attempt to optimize 
the old vGIC). If you don't have number, then you should detail the rationale here.

The more I think about it, the more I feel it would just be best to mask the 
interrupt just before dropping the priority. But I am happy to consider this if 
you have some ground to back the approach (they should be part of the commit 
message).

> vgic_inject_irq(...) already masking the interrupts in most of the cases.

Here my take on the commit message:

gic_interrupt() was implemented using a loop to limit the cost of the trap if 
there are multiple interrupts pending.

At the moment, interrupts are unmasked by gic_interrupt() before calling 
do_{IRQ, LPI}(). In the case of handling an interrupt routed to guests, its 
priority will be dropped, via desc->handler->end() called from do_irq(), with 
interrupt unmasked.

In other words:
     - Until the priority is dropped, only higher priority interrupt can be 
received. Today, only Xen interrupts have higher priority.
     - As soon as priority is dropped, any interrupt can be received.

This means the purpose of the loop in gic_interrupt() is defeated as all 
interrupts may get trapped earlier. To reinstate the purpose of the loop (and 
prevent the trap), interrupts should be masked when dropping the priority.

For interrupts routed to Xen, priority will always be dropped with interrupts 
masked. So the issue is not present. However, it means that we are pointless try 
to mask the interrupts.

To avoid conflicting behavior between interrupt handling, gic_interrupt() is now 
keeping interrupts masked and defer the decision to do_{LPI, IRQ}.

[ Details to be added once you give more ground ]

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

Hi Andrii,

On 30/05/2019 17:12, Andrii Anisov wrote:
> On 29.05.19 18:32, Julien Grall wrote:
>> It would have been nice to at least fix up the commit message with the typoes 
>> (and rewording) I mentioned in my previous e-mail.
>> Your commit message needs to explained why this is fine to keep the interrupt 
>> masked a bit longer. I wrote the explanation in my previous e-mail so you can 
>> borrow the rationale from there.
> xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()
> 
> Having irqs enabled here leaves a room for trapping and going through the trap

Please avoid "here" in commit message if you haven't defined where is the issue.

> path again if we have a new guest interrupt arrived (even with the same or

I don't understand the "new guest interrupt arrived".

> lower priority, after `desc->handler->end(desc)` in `do_IRQ()`).
> Keeping interrupts disabled during guest interrupts processing allows as

Missing word because "allows" and "as"?

> avoiding excessive traps (and wasting cpu cycles for trap path) while the new
> interrupts would be processed in the loop anyway. Processing guest interrupts by
> the loop should not introduce significant additional latency because

I am always worry when I see the word "should not" associated with "latency" 
because often it is actually the contrary (see the recent attempt to optimize 
the old vGIC). If you don't have number, then you should detail the rationale here.

The more I think about it, the more I feel it would just be best to mask the 
interrupt just before dropping the priority. But I am happy to consider this if 
you have some ground to back the approach (they should be part of the commit 
message).

> vgic_inject_irq(...) already masking the interrupts in most of the cases.

Here my take on the commit message:

gic_interrupt() was implemented using a loop to limit the cost of the trap if 
there are multiple interrupts pending.

At the moment, interrupts are unmasked by gic_interrupt() before calling 
do_{IRQ, LPI}(). In the case of handling an interrupt routed to guests, its 
priority will be dropped, via desc->handler->end() called from do_irq(), with 
interrupt unmasked.

In other words:
     - Until the priority is dropped, only higher priority interrupt can be 
received. Today, only Xen interrupts have higher priority.
     - As soon as priority is dropped, any interrupt can be received.

This means the purpose of the loop in gic_interrupt() is defeated as all 
interrupts may get trapped earlier. To reinstate the purpose of the loop (and 
prevent the trap), interrupts should be masked when dropping the priority.

For interrupts routed to Xen, priority will always be dropped with interrupts 
masked. So the issue is not present. However, it means that we are pointless try 
to mask the interrupts.

To avoid conflicting behavior between interrupt handling, gic_interrupt() is now 
keeping interrupts masked and defer the decision to do_{LPI, IRQ}.

[ Details to be added once you give more ground ]

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Stefano Stabellini]

On Fri, 31 May 2019, Julien Grall wrote:
> Hi Andrii,
> 
> On 30/05/2019 17:12, Andrii Anisov wrote:
> > On 29.05.19 18:32, Julien Grall wrote:
> > > It would have been nice to at least fix up the commit message with the
> > > typoes (and rewording) I mentioned in my previous e-mail.
> > > Your commit message needs to explained why this is fine to keep the
> > > interrupt masked a bit longer. I wrote the explanation in my previous
> > > e-mail so you can borrow the rationale from there.
> > xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()
> > 
> > Having irqs enabled here leaves a room for trapping and going through the
> > trap
> 
> Please avoid "here" in commit message if you haven't defined where is the
> issue.
> 
> > path again if we have a new guest interrupt arrived (even with the same or
> 
> I don't understand the "new guest interrupt arrived".
> 
> > lower priority, after `desc->handler->end(desc)` in `do_IRQ()`).
> > Keeping interrupts disabled during guest interrupts processing allows as
> 
> Missing word because "allows" and "as"?
> 
> > avoiding excessive traps (and wasting cpu cycles for trap path) while the
> > new
> > interrupts would be processed in the loop anyway. Processing guest
> > interrupts by
> > the loop should not introduce significant additional latency because
> 
> I am always worry when I see the word "should not" associated with "latency"
> because often it is actually the contrary (see the recent attempt to optimize
> the old vGIC). If you don't have number, then you should detail the rationale
> here.
> 
> The more I think about it, the more I feel it would just be best to mask the
> interrupt just before dropping the priority. But I am happy to consider this
> if you have some ground to back the approach (they should be part of the
> commit message).
> 
> > vgic_inject_irq(...) already masking the interrupts in most of the cases.
> 
> Here my take on the commit message:
> 
> gic_interrupt() was implemented using a loop to limit the cost of the trap if
> there are multiple interrupts pending.
> 
> At the moment, interrupts are unmasked by gic_interrupt() before calling
> do_{IRQ, LPI}(). In the case of handling an interrupt routed to guests, its
> priority will be dropped, via desc->handler->end() called from do_irq(), with
> interrupt unmasked.
> 
> In other words:
>     - Until the priority is dropped, only higher priority interrupt can be
> received. Today, only Xen interrupts have higher priority.
>     - As soon as priority is dropped, any interrupt can be received.
> 
> This means the purpose of the loop in gic_interrupt() is defeated as all
> interrupts may get trapped earlier. To reinstate the purpose of the loop (and
> prevent the trap), interrupts should be masked when dropping the priority.
> 
> For interrupts routed to Xen, priority will always be dropped with interrupts
> masked. So the issue is not present. However, it means that we are pointless
> try to mask the interrupts.
> 
> To avoid conflicting behavior between interrupt handling, gic_interrupt() is
> now keeping interrupts masked and defer the decision to do_{LPI, IRQ}.

This is a really well written commit message, and together with the
patch it looks fine to me.

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Stefano Stabellini]

On Fri, 31 May 2019, Julien Grall wrote:
> Hi Andrii,
> 
> On 30/05/2019 17:12, Andrii Anisov wrote:
> > On 29.05.19 18:32, Julien Grall wrote:
> > > It would have been nice to at least fix up the commit message with the
> > > typoes (and rewording) I mentioned in my previous e-mail.
> > > Your commit message needs to explained why this is fine to keep the
> > > interrupt masked a bit longer. I wrote the explanation in my previous
> > > e-mail so you can borrow the rationale from there.
> > xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()
> > 
> > Having irqs enabled here leaves a room for trapping and going through the
> > trap
> 
> Please avoid "here" in commit message if you haven't defined where is the
> issue.
> 
> > path again if we have a new guest interrupt arrived (even with the same or
> 
> I don't understand the "new guest interrupt arrived".
> 
> > lower priority, after `desc->handler->end(desc)` in `do_IRQ()`).
> > Keeping interrupts disabled during guest interrupts processing allows as
> 
> Missing word because "allows" and "as"?
> 
> > avoiding excessive traps (and wasting cpu cycles for trap path) while the
> > new
> > interrupts would be processed in the loop anyway. Processing guest
> > interrupts by
> > the loop should not introduce significant additional latency because
> 
> I am always worry when I see the word "should not" associated with "latency"
> because often it is actually the contrary (see the recent attempt to optimize
> the old vGIC). If you don't have number, then you should detail the rationale
> here.
> 
> The more I think about it, the more I feel it would just be best to mask the
> interrupt just before dropping the priority. But I am happy to consider this
> if you have some ground to back the approach (they should be part of the
> commit message).
> 
> > vgic_inject_irq(...) already masking the interrupts in most of the cases.
> 
> Here my take on the commit message:
> 
> gic_interrupt() was implemented using a loop to limit the cost of the trap if
> there are multiple interrupts pending.
> 
> At the moment, interrupts are unmasked by gic_interrupt() before calling
> do_{IRQ, LPI}(). In the case of handling an interrupt routed to guests, its
> priority will be dropped, via desc->handler->end() called from do_irq(), with
> interrupt unmasked.
> 
> In other words:
>     - Until the priority is dropped, only higher priority interrupt can be
> received. Today, only Xen interrupts have higher priority.
>     - As soon as priority is dropped, any interrupt can be received.
> 
> This means the purpose of the loop in gic_interrupt() is defeated as all
> interrupts may get trapped earlier. To reinstate the purpose of the loop (and
> prevent the trap), interrupts should be masked when dropping the priority.
> 
> For interrupts routed to Xen, priority will always be dropped with interrupts
> masked. So the issue is not present. However, it means that we are pointless
> try to mask the interrupts.
> 
> To avoid conflicting behavior between interrupt handling, gic_interrupt() is
> now keeping interrupts masked and defer the decision to do_{LPI, IRQ}.

This is a really well written commit message, and together with the
patch it looks fine to me.

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

Hello Julien,

On 31.05.19 20:11, Julien Grall wrote:

> Here my take on the commit message:
> 
> gic_interrupt() was implemented using a loop to limit the cost of the trap if there are multiple interrupts pending.
> 
> At the moment, interrupts are unmasked by gic_interrupt() before calling do_{IRQ, LPI}(). In the case of handling an interrupt routed to guests, its priority will be dropped, via desc->handler->end() called from do_irq(), with interrupt unmasked.
> 
> In other words:
>      - Until the priority is dropped, only higher priority interrupt can be received. Today, only Xen interrupts have higher priority.
>      - As soon as priority is dropped, any interrupt can be received.
> 
> This means the purpose of the loop in gic_interrupt() is defeated as all interrupts may get trapped earlier. To reinstate the purpose of the loop (and prevent the trap), interrupts should be masked when dropping the priority.
> 
> For interrupts routed to Xen, priority will always be dropped with interrupts masked. So the issue is not present. However, it means that we are pointless try to mask the interrupts.
> 
> To avoid conflicting behavior between interrupt handling, gic_interrupt() is now keeping interrupts masked and defer the decision to do_{LPI, IRQ}.

It is OK with me.

Are you waiting from me more of

> [ Details to be added once you give more ground ]

?

-- 
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

On 6/10/19 4:49 PM, Andrii Anisov wrote:
> Hello Julien,

Hi Andrii,

> 
> On 31.05.19 20:11, Julien Grall wrote:
> 
>> Here my take on the commit message:
>>
>> gic_interrupt() was implemented using a loop to limit the cost of the 
>> trap if there are multiple interrupts pending.
>>
>> At the moment, interrupts are unmasked by gic_interrupt() before 
>> calling do_{IRQ, LPI}(). In the case of handling an interrupt routed 
>> to guests, its priority will be dropped, via desc->handler->end() 
>> called from do_irq(), with interrupt unmasked.
>>
>> In other words:
>>      - Until the priority is dropped, only higher priority interrupt 
>> can be received. Today, only Xen interrupts have higher priority.
>>      - As soon as priority is dropped, any interrupt can be received.
>>
>> This means the purpose of the loop in gic_interrupt() is defeated as 
>> all interrupts may get trapped earlier. To reinstate the purpose of 
>> the loop (and prevent the trap), interrupts should be masked when 
>> dropping the priority.
>>
>> For interrupts routed to Xen, priority will always be dropped with 
>> interrupts masked. So the issue is not present. However, it means that 
>> we are pointless try to mask the interrupts.
>>
>> To avoid conflicting behavior between interrupt handling, 
>> gic_interrupt() is now keeping interrupts masked and defer the 
>> decision to do_{LPI, IRQ}.
> 
> It is OK with me.
> 
> Are you waiting from me more of

I was. But I also had time to think about the commit message and I would 
be happy to commit with just what it is currently written.

I have now applied to my staging branch with my acked-by. I will commit 
it tonight.

Thank you for the patch,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

On 10.06.19 22:51, Julien Grall wrote:
> I was. But I also had time to think about the commit message and I would be happy to commit with just what it is currently written.
> 
> I have now applied to my staging branch with my acked-by. I will commit it tonight.

Great. Thank you.

> 
> Thank you for the patch,

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

On 29.05.19 18:32, Julien Grall wrote:
>> BTW, do you hear about plans for the new vgic? Some time ago it was said that new vgic implementation going to replace the old one, and optimizing the old is worthless. But as I see, there are no updates into that area yet.
> 
> We need help to make it happen.
I'm not sure I'll have spare time soon, but what kind of help you need? Do you have a TODO list?

-- 
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov]

On 29.05.19 18:32, Julien Grall wrote:
>> BTW, do you hear about plans for the new vgic? Some time ago it was said that new vgic implementation going to replace the old one, and optimizing the old is worthless. But as I see, there are no updates into that area yet.
> 
> We need help to make it happen.
I'm not sure I'll have spare time soon, but what kind of help you need? Do you have a TODO list?

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

Hi,

On 30/05/2019 17:14, Andrii Anisov wrote:
> 
> 
> On 29.05.19 18:32, Julien Grall wrote:
>>> BTW, do you hear about plans for the new vgic? Some time ago it was said that 
>>> new vgic implementation going to replace the old one, and optimizing the old 
>>> is worthless. But as I see, there are no updates into that area yet.
>>
>> We need help to make it happen.
> I'm not sure I'll have spare time soon, but what kind of help you need? Do you 
> have a TODO list?

 From the top of my head the major blocker is GICv3 (+ ITS) support. It mostly 
need to be ported from Linux.

There were also a couple of concern regarding using ordering the list while 
folding (can't remember if this was addressed in Linux).

@Andre, I thought we capture that in Xen Project jira but I can't find it. Did 
you keep a list?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

Hi,

On 30/05/2019 17:14, Andrii Anisov wrote:
> 
> 
> On 29.05.19 18:32, Julien Grall wrote:
>>> BTW, do you hear about plans for the new vgic? Some time ago it was said that 
>>> new vgic implementation going to replace the old one, and optimizing the old 
>>> is worthless. But as I see, there are no updates into that area yet.
>>
>> We need help to make it happen.
> I'm not sure I'll have spare time soon, but what kind of help you need? Do you 
> have a TODO list?

 From the top of my head the major blocker is GICv3 (+ ITS) support. It mostly 
need to be ported from Linux.

There were also a couple of concern regarding using ordering the list while 
folding (can't remember if this was addressed in Linux).

@Andre, I thought we capture that in Xen Project jira but I can't find it. Did 
you keep a list?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andre Przywara]

On Fri, 31 May 2019 18:16:52 +0100
Julien Grall <julien.grall@arm.com> wrote:

> Hi,
> 
> On 30/05/2019 17:14, Andrii Anisov wrote:
> > 
> > 
> > On 29.05.19 18:32, Julien Grall wrote:  
> >>> BTW, do you hear about plans for the new vgic? Some time ago it was said that 
> >>> new vgic implementation going to replace the old one, and optimizing the old 
> >>> is worthless. But as I see, there are no updates into that area yet.  
> >>
> >> We need help to make it happen.  
> > I'm not sure I'll have spare time soon, but what kind of help you need? Do you 
> > have a TODO list?  
> 
>  From the top of my head the major blocker is GICv3 (+ ITS) support. It mostly 
> need to be ported from Linux.
> 
> There were also a couple of concern regarding using ordering the list while 
> folding (can't remember if this was addressed in Linux).
> 
> @Andre, I thought we capture that in Xen Project jira but I can't find it. Did 
> you keep a list?

I thought Jira as well, but apparently we never actually did this :-(

So yes, GICv3 support is the showstopper here, this would allow us to make the new VGIC the default, since we would have feature parity.

Also we would need to check the git log in Linux for the individual VGIC files to port over fixes, if applicable. Due to the different coding style and renamed identifiers, comparing the files does not really help.

Cheers,
Andre.

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andre Przywara]

On Fri, 31 May 2019 18:16:52 +0100
Julien Grall <julien.grall@arm.com> wrote:

> Hi,
> 
> On 30/05/2019 17:14, Andrii Anisov wrote:
> > 
> > 
> > On 29.05.19 18:32, Julien Grall wrote:  
> >>> BTW, do you hear about plans for the new vgic? Some time ago it was said that 
> >>> new vgic implementation going to replace the old one, and optimizing the old 
> >>> is worthless. But as I see, there are no updates into that area yet.  
> >>
> >> We need help to make it happen.  
> > I'm not sure I'll have spare time soon, but what kind of help you need? Do you 
> > have a TODO list?  
> 
>  From the top of my head the major blocker is GICv3 (+ ITS) support. It mostly 
> need to be ported from Linux.
> 
> There were also a couple of concern regarding using ordering the list while 
> folding (can't remember if this was addressed in Linux).
> 
> @Andre, I thought we capture that in Xen Project jira but I can't find it. Did 
> you keep a list?

I thought Jira as well, but apparently we never actually did this :-(

So yes, GICv3 support is the showstopper here, this would allow us to make the new VGIC the default, since we would have feature parity.

Also we would need to check the git log in Linux for the individual VGIC files to port over fixes, if applicable. Due to the different coding style and renamed identifiers, comparing the files does not really help.

Cheers,
Andre.

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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andre Przywara]

On Tue, 28 May 2019 18:07:19 +0100
Julien Grall <julien.grall@arm.com> wrote:

[ ... ]

> While looking at the code, I noticed that in the new vgic vgic_get_irq() 
> looks unsafe to be called with interrupt unmasked. This is because one 
> of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq. 
> Andre, what do you think?

I think you are right. In vgic_inject_irq(), right after the call to vgic_get_irq(), we use spin_lock_irqsave() on the irq_lock, so using the same irqsave version on the lpi_list_lock seems needed. But this is somewhat theoretical at the moment, as I think we will never LPIs through the new VGIC at the moment.

Cheers,
Andre.

> > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> > ---
> > 
> > Changes:
> > 
> >      in v2: Drop irq enabling for lpi processing as well.
> > 
> > 
> > ---
> >   xen/arch/arm/gic.c | 4 ----
> >   1 file changed, 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 6cc7dec..113655a 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
> >   
> >           if ( likely(irq >= 16 && irq < 1020) )
> >           {
> > -            local_irq_enable();
> >               isb();
> >               do_IRQ(regs, irq, is_fiq);
> > -            local_irq_disable();
> >           }
> >           else if ( is_lpi(irq) )
> >           {
> > -            local_irq_enable();
> >               isb();
> >               gic_hw_ops->do_LPI(irq);
> > -            local_irq_disable();
> >           }
> >           else if ( unlikely(irq < 16) )
> >           {
> >   
> 
> Cheers,
> 


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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andre Przywara]

On Tue, 28 May 2019 18:07:19 +0100
Julien Grall <julien.grall@arm.com> wrote:

[ ... ]

> While looking at the code, I noticed that in the new vgic vgic_get_irq() 
> looks unsafe to be called with interrupt unmasked. This is because one 
> of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq. 
> Andre, what do you think?

I think you are right. In vgic_inject_irq(), right after the call to vgic_get_irq(), we use spin_lock_irqsave() on the irq_lock, so using the same irqsave version on the lpi_list_lock seems needed. But this is somewhat theoretical at the moment, as I think we will never LPIs through the new VGIC at the moment.

Cheers,
Andre.

> > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> > ---
> > 
> > Changes:
> > 
> >      in v2: Drop irq enabling for lpi processing as well.
> > 
> > 
> > ---
> >   xen/arch/arm/gic.c | 4 ----
> >   1 file changed, 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 6cc7dec..113655a 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
> >   
> >           if ( likely(irq >= 16 && irq < 1020) )
> >           {
> > -            local_irq_enable();
> >               isb();
> >               do_IRQ(regs, irq, is_fiq);
> > -            local_irq_disable();
> >           }
> >           else if ( is_lpi(irq) )
> >           {
> > -            local_irq_enable();
> >               isb();
> >               gic_hw_ops->do_LPI(irq);
> > -            local_irq_disable();
> >           }
> >           else if ( unlikely(irq < 16) )
> >           {
> >   
> 
> Cheers,
> 


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

Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

On 31/05/2019 18:25, Andre Przywara wrote:
> On Tue, 28 May 2019 18:07:19 +0100
> Julien Grall <julien.grall@arm.com> wrote:
> 
> [ ... ]
> 
>> While looking at the code, I noticed that in the new vgic vgic_get_irq()
>> looks unsafe to be called with interrupt unmasked. This is because one
>> of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq.
>> Andre, what do you think?
> 
> I think you are right. In vgic_inject_irq(), right after the call to vgic_get_irq(), we use spin_lock_irqsave() on the irq_lock, so using the same irqsave version on the lpi_list_lock seems needed. But this is somewhat theoretical at the moment, as I think we will never LPIs through the new VGIC at the moment.

That's correct, we probably want to add that in the list of TODOs for the new 
vGIC :).

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Julien Grall]

On 31/05/2019 18:25, Andre Przywara wrote:
> On Tue, 28 May 2019 18:07:19 +0100
> Julien Grall <julien.grall@arm.com> wrote:
> 
> [ ... ]
> 
>> While looking at the code, I noticed that in the new vgic vgic_get_irq()
>> looks unsafe to be called with interrupt unmasked. This is because one
>> of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq.
>> Andre, what do you think?
> 
> I think you are right. In vgic_inject_irq(), right after the call to vgic_get_irq(), we use spin_lock_irqsave() on the irq_lock, so using the same irqsave version on the lpi_list_lock seems needed. But this is somewhat theoretical at the moment, as I think we will never LPIs through the new VGIC at the moment.

That's correct, we probably want to add that in the list of TODOs for the new 
vGIC :).

Cheers,

-- 
Julien Grall

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