Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Julien Grall <julien.grall@linaro.org>]

On 06/24/2014 07:04 PM, Stefano Stabellini wrote:
> On Tue, 24 Jun 2014, Julien Grall wrote:
>> On 24/06/14 12:38, Stefano Stabellini wrote:
>>>> Sorry for not having catch this earlier. I don't really like the idea to
>>>> extend the rank lock to vgic_{enable/disable}_irqs. The 2 functions
>>>> can be long to execute as it may touch the GIC distributor.
>>>>
>>>> In another way, the rank lock is only taken in the distributor emulation.
>>>>
>>>> Assuming we consider that distributor access may be slow:
>>>
>>> We could end up enabling or disabling the wrong set of interrupts
>>> without this change. I think it is necessary.
>>
>> AFAIU, this lock only protects the rank when we retrieve the target VCPU, the
>> other part of the function will still work without it.
>>
>> What I meant is to call vgic_get_target_vcpu, so the lock will protect only
>> what is necessary and not more.
> 
> I don't think so (unless I misunderstood your suggestion): setting
> ienable and enabling the interrupts need to be atomic.  Otherwise this
> can happen:
> 
>     VCPU0                                       VCPU1
> - rank_lock
> - write icenabler
> - get target vcpus
> - rank_unlock
>                                          - rank_lock
>                                          - write icenable
>                                          - get target vcpus (some are the same)
>                                          - rank_unlock
> 
>                                          - vgic_enable_irqs
> 
> - vgic_enable_irqs
> 
> 
> we now have an inconsistent state: we enabled the irqs written from
> vcpu0 but icenable reflects what was written from vcpu1.

In your example it's not possible because we save the value of ienable
in a temporary variable. So calling to vgic_enable_irqs on 2 different
VCPU with the same range of IRQ won't be a problem.

But... there is a possible race condition between enable and disable. We
need to serialize the access otherwise we may enable/disable by mistake
an IRQ and it's not synced anymore with the register state.

This is issue is also on Xen 4.4. Can you send a single patch which move
the unlock for this branch?

Thanks,

-- 
Julien Grall