[PATCH] gic:vgic: avoid excessive conversions

[PATCH] gic:vgic: avoid excessive conversions

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

Avoid excessive conversions between pending_irq and irq number/priority.
This simlifies functions interface and reduces under locks code size.

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---
 xen/arch/arm/gic-vgic.c    | 10 +++-------
 xen/arch/arm/vgic-v3-its.c |  2 +-
 xen/arch/arm/vgic.c        |  6 +++---
 xen/include/asm-arm/gic.h  |  5 ++---
 4 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 990399c..e8e9136 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -73,10 +73,8 @@ void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p)
     list_del_init(&p->lr_queue);
 }
 
-void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
+void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *n)
 {
-    struct pending_irq *n = irq_to_pending(v, virtual_irq);
-
     /* If an LPI has been removed meanwhile, there is nothing left to raise. */
     if ( unlikely(!n) )
         return;
@@ -133,12 +131,10 @@ static unsigned int gic_find_unused_lr(struct vcpu *v,
     return lr;
 }
 
-void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
-        unsigned int priority)
+void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p)
 {
     int i;
     unsigned int nr_lrs = gic_get_nr_lrs();
-    struct pending_irq *p = irq_to_pending(v, virtual_irq);
 
     ASSERT(spin_is_locked(&v->arch.vgic.lock));
 
@@ -227,7 +223,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
              test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
              !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
-            gic_raise_guest_irq(v, irq, p->priority);
+            gic_raise_guest_irq(v, p);
         else {
             list_del_init(&p->inflight);
             /*
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 5b73c4e..193a28f 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -447,7 +447,7 @@ static void update_lpi_vgic_status(struct vcpu *v, struct pending_irq *p)
     {
         if ( !list_empty(&p->inflight) &&
              !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-            gic_raise_guest_irq(v, p->irq, p->lpi_priority);
+            gic_raise_guest_irq(v, p);
     }
     else
         gic_remove_from_lr_pending(v, p);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5a4f082..628a34f 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -403,7 +403,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         p = irq_to_pending(v_target, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
         if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-            gic_raise_guest_irq(v_target, irq, p->priority);
+            gic_raise_guest_irq(v_target, p);
         spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
         if ( p->desc != NULL )
         {
@@ -568,7 +568,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
 
     if ( !list_empty(&n->inflight) )
     {
-        gic_raise_inflight_irq(v, virq);
+        gic_raise_inflight_irq(v, n);
         goto out;
     }
 
@@ -577,7 +577,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
 
     /* the irq is enabled */
     if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
-        gic_raise_guest_irq(v, virq, priority);
+        gic_raise_guest_irq(v, n);
 
     list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
     {
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index fab02f1..1859416 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -252,9 +252,8 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
 extern void gic_clear_pending_irqs(struct vcpu *v);
 
 extern void init_maintenance_interrupt(void);
-extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
-        unsigned int priority);
-extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
+extern void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p);
+extern void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *n);
 
 /* Accept an interrupt from the GIC and dispatch its handler */
 extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
-- 
2.7.4


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

Re: [PATCH] gic:vgic: avoid excessive conversions

[Julien Grall]

Hi Andrii,

On 16/11/2018 16:45, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> Avoid excessive conversions between pending_irq and irq number/priority.
> This simlifies functions interface and reduces under locks code size.
> 
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>   xen/arch/arm/gic-vgic.c    | 10 +++-------
>   xen/arch/arm/vgic-v3-its.c |  2 +-
>   xen/arch/arm/vgic.c        |  6 +++---
>   xen/include/asm-arm/gic.h  |  5 ++---
>   4 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 990399c..e8e9136 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -73,10 +73,8 @@ void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p)
>       list_del_init(&p->lr_queue);
>   }
>   
> -void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
> +void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *n)
>   {
> -    struct pending_irq *n = irq_to_pending(v, virtual_irq);
> -
>       /* If an LPI has been removed meanwhile, there is nothing left to raise. */
>       if ( unlikely(!n) )
>           return;
> @@ -133,12 +131,10 @@ static unsigned int gic_find_unused_lr(struct vcpu *v,
>       return lr;
>   }
>   
> -void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> -        unsigned int priority)
> +void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p)
>   {
>       int i;
>       unsigned int nr_lrs = gic_get_nr_lrs();
> -    struct pending_irq *p = irq_to_pending(v, virtual_irq);
>   
>       ASSERT(spin_is_locked(&v->arch.vgic.lock));
>   
> @@ -227,7 +223,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>           if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
>                test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
>                !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> -            gic_raise_guest_irq(v, irq, p->priority);
> +            gic_raise_guest_irq(v, p);
>           else {
>               list_del_init(&p->inflight);
>               /*
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 5b73c4e..193a28f 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -447,7 +447,7 @@ static void update_lpi_vgic_status(struct vcpu *v, struct pending_irq *p)
>       {
>           if ( !list_empty(&p->inflight) &&
>                !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> -            gic_raise_guest_irq(v, p->irq, p->lpi_priority);
> +            gic_raise_guest_irq(v, p);

The interface is not behaving the same way now. I understand that nobody is 
using the 3 parameters but that's actually a bug with the ITS.

Because the LPI will not be set with expected priority.

>       }
>       else
>           gic_remove_from_lr_pending(v, p);
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 5a4f082..628a34f 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -403,7 +403,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>           p = irq_to_pending(v_target, irq);1651304100e
>           set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>           if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> -            gic_raise_guest_irq(v_target, irq, p->priority);
> +            gic_raise_guest_irq(v_target, p);
>           spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>           if ( p->desc != NULL )
>           {
> @@ -568,7 +568,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
>   
>       if ( !list_empty(&n->inflight) )
>       {
> -        gic_raise_inflight_irq(v, virq);
> +        gic_raise_inflight_irq(v, n);
>           goto out;
>       }
>   
> @@ -577,7 +577,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
>   
>       /* the irq is enabled */
>       if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
> -        gic_raise_guest_irq(v, virq, priority);
> +        gic_raise_guest_irq(v, n);
>   
>       list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
>       {
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index fab02f1..1859416 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -252,9 +252,8 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>   extern void gic_clear_pending_irqs(struct vcpu *v);
>   
>   extern void init_maintenance_interrupt(void);
> -extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
> -        unsigned int priority);
> -extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
> +extern void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p);
> +extern void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *n);

That code is not going to compile with the new vGIC as pending_irq only exists 
for the current vGIC.

>   
>   /* Accept an interrupt from the GIC and dispatch its handler */
>   extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH] gic:vgic: avoid excessive conversions

[Andrii Anisov]

Hello Julien,


On 16.11.18 19:27, Julien Grall wrote:
>>           if ( !list_empty(&p->inflight) &&
>>                !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>> -            gic_raise_guest_irq(v, p->irq, p->lpi_priority);
>> +            gic_raise_guest_irq(v, p);
>
> The interface is not behaving the same way now. I understand that 
> nobody is using the 3 parameters but that's actually a bug with the ITS.
>
> Because the LPI will not be set with expected priority.
But it is not the issue of the interface, you know. Keeping in mind that 
both an inflight and an lr_pending queues are sorted by p->priority, the 
field p->lpi_priority looks odd anyway.

>> -extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
>> -        unsigned int priority);
>> -extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int 
>> virtual_irq);
>> +extern void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p);
>> +extern void gic_raise_inflight_irq(struct vcpu *v, struct 
>> pending_irq *n);
>
> That code is not going to compile with the new vGIC as pending_irq 
> only exists for the current vGIC.

Yep, I did miss that.

I guess wrapping it with `#ifndef CONFIG_NEW_VGIC` must be enough here. 
Those two functions are not used by, nor defined by new-vgic.


-- 
Sincerely,
Andrii Anisov.


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

Re: [PATCH] gic:vgic: avoid excessive conversions

[Julien Grall]

[-- Attachment #1.1: Type: text/plain, Size: 2000 bytes --]

Sorry for the formatting.

On Mon, 19 Nov 2018, 12:04 Andrii Anisov, <andrii.anisov@gmail.com> wrote:

> Hello Julien,
>
>
> On 16.11.18 19:27, Julien Grall wrote:
> >>           if ( !list_empty(&p->inflight) &&
> >>                !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> >> -            gic_raise_guest_irq(v, p->irq, p->lpi_priority);
> >> +            gic_raise_guest_irq(v, p);
> >
> > The interface is not behaving the same way now. I understand that
> > nobody is using the 3 parameters but that's actually a bug with the ITS.
> >
> > Because the LPI will not be set with expected priority.
> But it is not the issue of the interface, you know. Keeping in mind that
> both an inflight and an lr_pending queues are sorted by p->priority, the
> field p->lpi_priority looks odd anyway.
>

You didn't get my point. You removed a parameter without explaining why it
is fine. In that context, the caller for LPI is using lpi_priority.

With your change, it is becoming issue with the interface because it may
not pass the correct things.

So this not a cleanup and there must be a reason why lpi_priority was
passed instead of priority.


> >> -extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
> >> -        unsigned int priority);
> >> -extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int
> >> virtual_irq);
> >> +extern void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p);
> >> +extern void gic_raise_inflight_irq(struct vcpu *v, struct
> >> pending_irq *n);
> >
> > That code is not going to compile with the new vGIC as pending_irq
> > only exists for the current vGIC.
>
> Yep, I did miss that.
>
> I guess wrapping it with `#ifndef CONFIG_NEW_VGIC` must be enough here.
> Those two functions are not used by, nor defined by new-vgic.


>
> --
> Sincerely,
> Andrii Anisov.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

[-- Attachment #1.2: Type: text/html, Size: 3109 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH] gic:vgic: avoid excessive conversions

[Andrii Anisov]

On 19.11.18 15:54, Julien Grall wrote:
> You didn't get my point. You removed a parameter without explaining 
> why it is fine.

It is a pure optimization. If you look through the code, you can see 
that callers of these functions already have a correspondent `struct 
pending_irq` pointer. But they pass irq number (and priority) to 
gic_raise_guest_irq/gic_raise_inflight_irq so that they should calculate 
the pointer again, what is suboptimal.


> In that context, the caller for LPI is using lpi_priority.
What in its turn is not taken into consideration by the current 
`gic_raise_guest_irq` function implementation. Even clearly ignored, 
because the priority abstraction is only needed by 
`gic_add_to_lr_pending` to insert a pending irq into `lr_queue`, but it 
does ordering by `p->priority` only.

> With your change, it is becoming issue with the interface because it 
> may not pass the correct things.

Now the interface of `gic_raise_guest_irq` function corresponds to its 
implementation in addition to optimization.


> So this not a cleanup and there must be a reason why lpi_priority was 
> passed instead of priority.

Moreover, if one would like or plan to honor lpi_priority in 
`gic_raise_guest_irq`, he can freely extract it from a pending_irq 
structure. It is still there.


-- 

Sincerely,
Andrii Anisov.


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

Re: [PATCH] gic:vgic: avoid excessive conversions

[Andrii Anisov]

Hello Andre,


I'm going to change "gic_raise_guest_irq()" function interface.

Could you please comment my understanding of vgic-v3-its.c code below? 
So that I could fix it alongside the function interface change.

On 16.11.18 18:45, Andrii Anisov wrote:
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 5b73c4e..193a28f 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -447,7 +447,7 @@ static void update_lpi_vgic_status(struct vcpu *v, struct pending_irq *p)
>       {
>           if ( !list_empty(&p->inflight) &&
>                !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )

As I understand, the intention here is to reinsert an irq to 
`lr_pending` queue with an updated priority, just in case the irq is not 
yet visible to guests. You try to pass a `p->lpi_priority` to the 
`gic_raise_guest_irq` as a new priority. But, with the current 
implementation, that parameter is clearly ignored by the function. 
Moreover, it just could not be honored there, because 
`gic_raise_guest_irq` touches only `lr_pending` queue, while both 
`lr_pending` and `inflight_irqs` are sorted by `p->priority` and you 
can't change it while updating only one queue.

I guess here the irq should be removed from both queues first, then 
`p->priority` updated to `p->lpi_priority`, then irq should be 
reinserted to both queues.

What do you think about that?

> -            gic_raise_guest_irq(v, p->irq, p->lpi_priority);
> +            gic_raise_guest_irq(v, p);
>       }
>       else
The call below looks excessive to me. We can reach this branch in case 
`p->inflight` is empty or it is not empty, but irq is visible to guest. 
In both those cases irq can not be on lr_pending queue, so calling this 
function is just a wasting cpu cycles.
>           gic_remove_from_lr_pending(v, p);

-- 
Sincerely,
Andrii Anisov.


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

Re: [PATCH] gic:vgic: avoid excessive conversions

[Andrii Anisov]

Andre,

Could you please comment on the patch and below.

On 20.11.18 13:09, Andrii Anisov wrote:
> Hello Andre,
> 
> 
> I'm going to change "gic_raise_guest_irq()" function interface.
> 
> Could you please comment my understanding of vgic-v3-its.c code below? So that I could fix it alongside the function interface change.
> 
> On 16.11.18 18:45, Andrii Anisov wrote:
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 5b73c4e..193a28f 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -447,7 +447,7 @@ static void update_lpi_vgic_status(struct vcpu *v, struct pending_irq *p)
>>       {
>>           if ( !list_empty(&p->inflight) &&
>>                !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> 
> As I understand, the intention here is to reinsert an irq to `lr_pending` queue with an updated priority, just in case the irq is not yet visible to guests. You try to pass a `p->lpi_priority` to the `gic_raise_guest_irq` as a new priority. But, with the current implementation, that parameter is clearly ignored by the function. Moreover, it just could not be honored there, because `gic_raise_guest_irq` touches only `lr_pending` queue, while both `lr_pending` and `inflight_irqs` are sorted by `p->priority` and you can't change it while updating only one queue.
> 
> I guess here the irq should be removed from both queues first, then `p->priority` updated to `p->lpi_priority`, then irq should be reinserted to both queues.
> 
> What do you think about that?
> 
>> -            gic_raise_guest_irq(v, p->irq, p->lpi_priority);
>> +            gic_raise_guest_irq(v, p);
>>       }
>>       else
> The call below looks excessive to me. We can reach this branch in case `p->inflight` is empty or it is not empty, but irq is visible to guest. In both those cases irq can not be on lr_pending queue, so calling this function is just a wasting cpu cycles.
>>           gic_remove_from_lr_pending(v, p);
> 

-- 
Sincerely,
Andrii Anisov.

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