Re: [Xen-devel] [RFC 04/16] vgic: move irq_to_pending out of lock

[Julien Grall <julien.grall@xxxxxxx>]

On 28/11/2018 21:31, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> For GICV2 pending_irq allocation is not concurrent, so reduce
> some code under lock.
So this is reverting part of 5f66da659060563df8481a86c017f07455095045
"ARM: vGIC: move irq_to_pending() calls under the VGIC VCPU lock".

The vGIC is meant to be common, so I don't think the patch would be correct for 
GICv3.
However, this function can only be called with SPI (PPI/SGI can't migrate). All 
but one SPI are associated to a physical interrupts. So I am not entirely 
convinced this will have any benefits on GICv2. Do you see any contention on the 
lock?
> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> ---
>   xen/arch/arm/vgic.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a64633f..37857b1 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -267,17 +267,16 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, 
> unsigned int irq)
>       ASSERT(!is_lpi(irq));
>   #endif
>   
> -    spin_lock_irqsave(&old->arch.vgic.lock, flags);
> -
>       p = irq_to_pending(old, irq);
>   
>       /* nothing to do for virtual interrupts */
>       if ( p->desc == NULL )
>       {
> -        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>           return true;
>       }
>   
> +    spin_lock_irqsave(&old->arch.vgic.lock, flags);
> +
>       /* migration already in progress, no need to do anything */
>       if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
>       {
> @@ -569,8 +568,6 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, 
> unsigned int virq,
>           return;
>       }
>   
> -    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> -
>       n = irq_to_pending(v, virq);
>   #ifdef CONFIG_GICV3
>       /* If an LPI has been removed, there is nothing to inject here. */
> @@ -581,6 +578,8 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, 
> unsigned int virq,
>       }
>   #endif
>   
> +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +
>       set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>   
>       if ( !list_empty(&n->inflight) )
Cheers,

--
Julien Grall

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