Re: [Xen-devel] [PATCH v8 02/27] ARM: VGIC: move irq_to_pending() calls under the VGIC VCPU lock

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 12/04/17 01:44, Andre Przywara wrote:
> So far irq_to_pending() is just a convenience function to lookup
> in statically allocated arrays. This will change with LPIs, which are
> more dynamic.
> So move the irq_to_pending() call under the VGIC VCPU lock, so we
> only use this pointer while holding the lock.
That's a call for an ASSERT in irq_to_pending. And you would have notice 
that not all irq_to_pending will then be protected by the vGIC lock (see 
vgic_migrate_irq for instance).
Also, please explain why the vgic lock as technically irq_to_pending is 
lock agnostic... And LPI structure will be per domain. So how do you 
expect the locking to work?
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic.c  | 5 ++++-
>  xen/arch/arm/vgic.c | 8 +++++---
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index da19130..dcb1783 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -402,10 +402,13 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, 
> struct pending_irq *n)
>
>  void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
>  {
> -    struct pending_irq *p = irq_to_pending(v, virtual_irq);
> +    struct pending_irq *p;
>      unsigned long flags;
>
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +
> +    p = irq_to_pending(v, virtual_irq);
> +
>      if ( !list_empty(&p->lr_queue) )
>          list_del_init(&p->lr_queue);
>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 83569b0..c953f13 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -466,14 +466,16 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>  {
>      uint8_t priority;
> -    struct pending_irq *iter, *n = irq_to_pending(v, virq);
> +    struct pending_irq *iter, *n;
>      unsigned long flags;
>      bool running;
>
> -    priority = vgic_get_virq_priority(v, virq);;
> -
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>
> +    n = irq_to_pending(v, virq);
> +
> +    priority = vgic_get_virq_priority(v, virq);
The commit message only speak about moving "irq_to_pending. So why the 
priority has been moved to?
This will introduce a lock ordering issue (rank lock vs vgic lock) 
between vgic_vcpu_inject_irq and ITARGET/IROUTER emulation.
The former is taking vgic lock first and then rank whilst the latter is 
doing the invert (see vgic_migrate_irq).
> +
>      /* vcpu offline */
>      if ( test_bit(_VPF_down, &v->pause_flags) )
>      {
Cheers,

--
Julien Grall

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