Re: [Xen-devel] [PATCH] gic:vgic: avoid excessive conversions

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andrii,

On 16/11/2018 16:45, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
>   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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel