Re: [Xen-devel] [PATCH v8 03/27] ARM: GIC: Add checks for NULL pointer pending_irq's

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 12/04/17 01:44, Andre Przywara wrote:
> For LPIs the struct pending_irq's are dynamically allocated and the
> pointers will be stored in a radix tree. Since an LPI can be "unmapped"
> at any time, teach the VGIC how to handle with irq_to_pending() returning
> a NULL pointer.
> We just do nothing in this case or clean up the LR if the virtual LPI
> number was still in an LR.
Why not all the irq_to_pending call are not protected (such as 
vgic_irq_to_migrate)? This is a call to forget to check NULL if we 
decide to use the function in the future.
Also, I would like a comment on top of irq_to_pending stating this can 
return NULL as you change the semantic of the function.
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic.c  | 37 ++++++++++++++++++++++++++++++++-----
>  xen/arch/arm/vgic.c |  6 ++++++
>  2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index dcb1783..62ae3b8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -408,9 +408,15 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int 
> virtual_irq)
>      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);
> +    /*
> +     * If an LPIs has been removed meanwhile, it has been cleaned up
> +     * already, so nothing to remove here.
> +     */
> +    if ( p )
> +    {
> +        if ( !list_empty(&p->lr_queue) )
> +            list_del_init(&p->lr_queue);
> +    }
This could be simplified with:

if ( p && !list_empty(&p->lr_queue) )
  list_del_init(...);

Also, you probably want a likely(p).

>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>  }
>
> @@ -418,6 +424,10 @@ 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 ( !n )
if ( unlikely(!n) )

> +        return;
> +
>      ASSERT(spin_is_locked(&v->arch.vgic.lock));
>
>      if ( list_empty(&n->lr_queue) )
> @@ -437,6 +447,11 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int 
> virtual_irq,
>  {
>      int i;
>      unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
> +    struct pending_irq *p = irq_to_pending(v, virtual_irq);
> +
> +    if ( !p )
if ( unlikely(!p) )

> +        /* An unmapped LPI does not need to be raised. */
> +        return;
Please move this check after the ASSERT to keep the check on all the paths.

>      ASSERT(spin_is_locked(&v->arch.vgic.lock));
>
> @@ -445,12 +460,12 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int 
> virtual_irq,
>          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
>          if (i < nr_lrs) {
>              set_bit(i, &this_cpu(lr_mask));
> -            gic_set_lr(i, irq_to_pending(v, virtual_irq), GICH_LR_PENDING);
> +            gic_set_lr(i, p, GICH_LR_PENDING);
>              return;
>          }
>      }
>
> -    gic_add_to_lr_pending(v, irq_to_pending(v, virtual_irq));
> +    gic_add_to_lr_pending(v, p);
>  }
>
>  static void gic_update_one_lr(struct vcpu *v, int i)
> @@ -464,7 +479,19 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>
>      gic_hw_ops->read_lr(i, &lr_val);
>      irq = lr_val.virq;
> +
4th time I am saying this.... Spurious line.

>      p = irq_to_pending(v, irq);
> +    /* An LPI might have been unmapped, in which case we just clean up here. */
> +    if ( !p )
unlikely(!p)

> +    {
> +        ASSERT(is_lpi(irq));
> +
> +        gic_hw_ops->clear_lr(i);
> +        clear_bit(i, &this_cpu(lr_mask));
> +
> +        return;
> +    }
> +
>      if ( lr_val.state & GICH_LR_ACTIVE )
>      {
>          set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index c953f13..b9fc837 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -473,6 +473,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
> virq)
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>
>      n = irq_to_pending(v, virq);
> +    /* If an LPI has been removed, there is nothing to inject here. */
> +    if ( !n )
unlikely(...)

> +    {
> +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +        return;
> +    }
>
>      priority = vgic_get_virq_priority(v, virq);
Cheers,

--
Julien Grall

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