Re: [RFC 13/16] gic-vgic: skip irqs locking

[Julien Grall <julien.grall@arm.com>]

Hi,

On 28/11/2018 21:32, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> Those fucntions are called under IRQs disabled already, so avoid

s/fucntions/

> additional flags saving and restore.
> 
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>   xen/arch/arm/gic-vgic.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 5b73bbd..e511f91 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -268,7 +268,6 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>   void vgic_sync_from_lrs(struct vcpu *v)
>   {
>       int i = 0;
> -    unsigned long flags;
>       unsigned int nr_lrs = gic_get_nr_lrs();
>   
>       /* The idle domain has no LRs to be cleared. Since gic_restore_state
> @@ -279,7 +278,7 @@ void vgic_sync_from_lrs(struct vcpu *v)
>   
>       gic_hw_ops->update_hcr_status(GICH_HCR_UIE, false);
>   
> -    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +    spin_lock(&v->arch.vgic.lock);

This chunk relies on patch #1, am I correct? If so, this should be written in 
the commit message that this was introduced recently. This helps to figure out 
whether the patch can be merged before the rest.

>   
>       while ((i = find_next_bit((const unsigned long *) &this_cpu(lr_mask),
>                                 nr_lrs, i)) < nr_lrs ) {
> @@ -287,7 +286,7 @@ void vgic_sync_from_lrs(struct vcpu *v)
>           i++;
>       }
>   
> -    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +    spin_unlock(&v->arch.vgic.lock);
>   }
>   
>   static void gic_restore_pending_irqs(struct vcpu *v)
> @@ -295,11 +294,10 @@ static void gic_restore_pending_irqs(struct vcpu *v)
>       int lr = 0;
>       struct pending_irq *p, *t, *p_r;
>       struct list_head *inflight_r;
> -    unsigned long flags;
>       unsigned int nr_lrs = gic_get_nr_lrs();
>       int lrs = nr_lrs;
>   
> -    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +    spin_lock(&v->arch.vgic.lock);

I would add an ASSERT(!local_is_irq_enabled()) on top to show that this should 
be called with interrupt disabled.

>   
>       if ( list_empty(&v->arch.vgic.lr_pending) )
>           goto out;
> @@ -343,7 +341,7 @@ found:
>       }
>   
>   out:
> -    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +    spin_unlock(&v->arch.vgic.lock);
>   }
>   
>   void gic_clear_pending_irqs(struct vcpu *v)
> 

Cheers,

-- 
Julien Grall

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