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

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

On 12/04/17 15:51, Andre Przywara wrote:
> Hi,
>
> On 12/04/17 11:25, Julien Grall wrote:
>> 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)?
>
> Some of them are never called by LPIs.
> Is it worth the put ASSERTs in there everywhere?

Yes.

> I can copy the table with all call sites and their evaluations from that
> other email into this commit message, if that sounds good.

And yes. A commit message should contain all the details saving us time 
to look in the e-mails...

>
> Fixed the rest.
>
> Cheers,
> Andre.
>
>> 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@arm.com>
>>> ---
>>>  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@lists.xen.org
https://lists.xen.org/xen-devel