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

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

On 12/04/17 11:13, Julien Grall wrote:
> 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?

I thought a bit more about this and I think vgic_vcpu_inject_irq will 
not be protected correctly for LPI.

Unlike SPIs, LPIs don't have active state in the GIC so theoretically a 
new interrupt can come up right after handling the first one. Although, 
it might have been possible that the vLPI was moved from vCPU A to vCPU 
B and still in the LRs (not yet handled by the guest or not yet cleaned).

So there is a race between gic_update_one_lr and vgic_vcpu_inject, both 
will take a different lock (resp. old and new) which may lead to a list 
corruption.

I am not sure how to protect this case as this could happen because of a 
"DISCARD -> MAPTI", "MOVI", "MOVALL"

>
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  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@lists.xen.org
https://lists.xen.org/xen-devel