From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi Date: Sat, 21 Jun 2014 15:59:40 +0100 Message-ID: <53A59DDC.50005@linaro.org> References: <1403271316-21635-1-git-send-email-stefano.stabellini@eu.citrix.com> <53A440FD.6070303@linaro.org> <53A4496A.4080009@linaro.org> <53A46863.2040701@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" 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: xen-devel@lists.xensource.com, apatel@apm.com, Ian Campbell , psawargaonkar@apm.com List-Id: xen-devel@lists.xenproject.org On 21/06/14 15:43, Stefano Stabellini wrote: > On Fri, 20 Jun 2014, Julien Grall wrote: >> On 20 Jun 2014 18:41, "Stefano Stabellini" wrote: >>> >>> On Fri, 20 Jun 2014, Julien Grall wrote: >>>> On 20/06/14 17:48, Stefano Stabellini wrote: >>>>> >>>>>> Your patch "physical irq follow virtual irq" won't even solve this problem >>>>>> if >>>>>> the maintenance interrupt is request to EOI the IRQ. >>>>> >>>>> I don't understand this statement. >>>>> The maintenance interrupt is requested to make sure that the vcpu is >>>>> interrupted as soon as it EOIs the virtual irq, so that >>>>> gic_update_one_lr can run. >>>> >>>> I agree with that but ... the following step can happen >>>> >>>> 1) Xen receives the interrupt >>>> 2) Xen writes EOIR >>>> 3) Xen inject the IRQ to the guest >>>> 4) The guest will receive the IRQ (i.e read IAR) >>>> 5) Xen migrates the VCPU to another physical CPU >>>> 6) The guest writes into GIC_DIR and a maintenance IRQ is fired. >>>> >>>> In a such case, the IRQ will be EOIed to another physical CPU. If we assume >>>> that we should always write to GICC_DIR of the physical CPU that receive the >>>> interrupt, then even your patch "physcial irq follow virtual irq" won't solve >>>> the problem. >>> >>> That's true. >>> >>> >>> >>>>>> The VGIC lock is per-vcpu. >>>>>> >>>>>> If this is happening while we migrate, nothing protect p->lr and the >>>>>> different >>>>>> bit anymore. >>>>> >>>>> The patch series >>>>> alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com takes >>>>> care of vcpu migration and locking. >>>>> >>>>> >>>>>> Indeed, the vgic_inject_irq will use the new VCPU. This can happen as soon >>>>>> as >>>>>> we EOI the IRQ. >>>>> >>>>> Yes, but at that point we have also already migrated the corresponding >>>>> physical irq to the new pcpu. >>>> >>>> Even tho we the migration has been done, the 2 clear bit are not atomic. Let's >>>> imagine that the IRQ has fired between the two clear bit. In this case we may >>>> clear the ACTIVE bit by mistake. >>>> >>>> I don't see anything in this code which prevents a such configuration. >>> >>> Which 2 clear bit? >> >> GUEST_VISIBLE and GUEST_ACTIVE see the code after eoi_irq. > > The code is protected by the vgic lock. If the IRQ has been migrating while it's injected, the maintenance interrupt will use the previous VCPU and vgic_inject_irq will use the new one. 2 different lock protecting the same thing we but not mutually exclusive. As soon as we write in GICC_DIR, this case can happen. -- Julien Grall