Re: [Xen-devel] [RFC PATCH v2 03/22] ARM: vGIC: move gic_raise_inflight_irq() into vgic_vcpu_inject_irq()

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 21/07/17 20:59, Andre Przywara wrote:
> Currently there is a gic_raise_inflight_irq(), which serves the very
> special purpose of handling a newly injected interrupt while an older
> one is still handled. This has only one user, in vgic_vcpu_inject_irq().
>
> Now with the introduction of the pending_irq lock this will later on
> result in a nasty deadlock, which can only be solved properly by
> actually embedding the function into the caller (and dropping the lock
> later in-between).
>
> This has the admittedly hideous consequence of needing to export
> gic_update_one_lr(), but this will go away in a later stage of a rework.
> In this respect this patch is more a temporary kludge.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic.c        | 30 +-----------------------------
>  xen/arch/arm/vgic.c       | 11 ++++++++++-
>  xen/include/asm-arm/gic.h |  2 +-
>  3 files changed, 12 insertions(+), 31 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 2c99d71..5bd66a2 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -44,8 +44,6 @@ static DEFINE_PER_CPU(uint64_t, lr_mask);
>
>  #undef GIC_DEBUG
>
> -static void gic_update_one_lr(struct vcpu *v, int i);
> -
>  static const struct gic_hw_operations *gic_hw_ops;
>
>  void register_gic_ops(const struct gic_hw_operations *ops)
> @@ -416,32 +414,6 @@ void gic_remove_irq_from_queues(struct vcpu *v, struct 
> pending_irq *p)
>      gic_remove_from_lr_pending(v, p);
>  }
>
> -void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
> -{
> -    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;
> -
> -    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> -
> -    /* Don't try to update the LR if the interrupt is disabled */
> -    if ( !test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
> -        return;
> -
> -    if ( list_empty(&n->lr_queue) )
> -    {
> -        if ( v == current )
> -            gic_update_one_lr(v, n->lr);
> -    }
> -#ifdef GIC_DEBUG
> -    else
> -        gdprintk(XENLOG_DEBUG, "trying to inject irq=%u into d%dv%d, when it is 
> still lr_pending\n",
> -                 virtual_irq, v->domain->domain_id, v->vcpu_id);
> -#endif
> -}
> -
>  /*
>   * Find an unused LR to insert an IRQ into, starting with the LR given
>   * by @lr. If this new interrupt is a PRISTINE LPI, scan the other LRs to
> @@ -503,7 +475,7 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int 
> virtual_irq,
>      gic_add_to_lr_pending(v, p);
>  }
>
> -static void gic_update_one_lr(struct vcpu *v, int i)
> +void gic_update_one_lr(struct vcpu *v, int i)
>  {
>      struct pending_irq *p;
>      int irq;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 38dacd3..7b122cd 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -536,7 +536,16 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
> virq)
>
>      if ( !list_empty(&n->inflight) )
>      {
> -        gic_raise_inflight_irq(v, virq);
> +        bool update = test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) &&
> +                      list_empty(&n->lr_queue) && (v == current);
> +
> +        if ( update )
> +            gic_update_one_lr(v, n->lr);
> +#ifdef GIC_DEBUG
> +        else
> +            gdprintk(XENLOG_DEBUG, "trying to inject irq=%u into d%dv%d, when it is 
> still lr_pending\n",
> +                     n->irq, v->domain->domain_id, v->vcpu_id);
The previous code was only printing this message when the pending_irq is 
queued. Now you will print any time you don't update the LR.
> +#endif
>          goto out;
>      }
>
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 6203dc5..cf8b8fb 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -237,12 +237,12 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned 
> int virq,
>
>  extern void gic_inject(void);
>  extern void gic_clear_pending_irqs(struct vcpu *v);
> +extern void gic_update_one_lr(struct vcpu *v, int lr);
>  extern int gic_events_need_delivery(void);
>
>  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_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p);
>  extern void gic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
Cheers,

--
Julien Grall

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