From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs Date: Tue, 24 Jun 2014 19:20:11 +0100 Message-ID: <53A9C15B.9070401@linaro.org> References: <1403541463-23734-1-git-send-email-stefano.stabellini@eu.citrix.com> <53A866CC.5030201@linaro.org> <53A96A0F.3020006@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: julien.grall@citrix.com, xen-devel@lists.xensource.com, Ian.Campbell@citrix.com List-Id: xen-devel@lists.xenproject.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