Re: [Xen-devel] [RFC PATCH v2 01/22] ARM: vGIC: introduce and initialize pending_irq lock

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 21/07/17 20:59, Andre Przywara wrote:
> Currently we protect the pending_irq structure with the corresponding
> VGIC VCPU lock. There are problems in certain corner cases (for
> instance if an IRQ is migrating), so let's introduce a per-IRQ lock,
> which will protect the consistency of this structure independent from
> any VCPU.
> For now this just introduces and initializes the lock, also adds
> wrapper macros to simplify its usage (and help debugging).
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/vgic.c        |  1 +
>  xen/include/asm-arm/vgic.h | 11 +++++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 1e5107b..38dacd3 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -69,6 +69,7 @@ void vgic_init_pending_irq(struct pending_irq *p, unsigned 
> int virq)
>      memset(p, 0, sizeof(*p));
>      INIT_LIST_HEAD(&p->inflight);
>      INIT_LIST_HEAD(&p->lr_queue);
> +    spin_lock_init(&p->lock);
>      p->irq = virq;
>      p->lpi_vcpu_id = INVALID_VCPU_ID;
>  }
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index d4ed23d..1c38b9a 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -90,6 +90,14 @@ struct pending_irq
>       * TODO: when implementing irq migration, taking only the current
>       * vgic lock is not going to be enough. */
>      struct list_head lr_queue;
> +    /* The lock protects the consistency of this structure. A single status bit
> +     * can be read and/or set without holding the lock using the atomic
> +     * set_bit/clear_bit/test_bit functions, however accessing multiple bits or
> +     * relating to other members in this struct requires the lock.
> +     * The list_head members are protected by their corresponding VCPU lock,
> +     * it is not sufficient to hold this pending_irq lock here to query or
> +     * change list order or affiliation. */
Actually, I have on question here. Do the vCPU lock sufficient to 
protect the list_head members. Or do you also mandate the pending_irq to 
be locked as well?
Also, it would be good to have the locking order documented maybe in 
docs/misc?
> +    spinlock_t lock;
>  };
>
>  #define NR_INTERRUPT_PER_RANK   32
> @@ -156,6 +164,9 @@ struct vgic_ops {
>  #define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
>  #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
>
> +#define vgic_irq_lock(p, flags) spin_lock_irqsave(&(p)->lock, flags)
> +#define vgic_irq_unlock(p, flags) spin_unlock_irqrestore(&(p)->lock, flags)
> +
>  #define vgic_lock_rank(v, r, flags)   spin_lock_irqsave(&(r)->lock, flags)
>  #define vgic_unlock_rank(v, r, flags) spin_unlock_irqrestore(&(r)->lock, flags)
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel