From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi Date: Sat, 21 Jun 2014 16:26:11 +0100 Message-ID: References: <1403271316-21635-1-git-send-email-stefano.stabellini@eu.citrix.com> <53A440FD.6070303@linaro.org> <53A4496A.4080009@linaro.org> <53A46863.2040701@linaro.org> <53A59DDC.50005@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53A59DDC.50005@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: xen-devel@lists.xensource.com, apatel@apm.com, Ian Campbell , psawargaonkar@apm.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Sat, 21 Jun 2014, Julien Grall wrote: > 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. I don't think this case can happen: the maintenance interrupt is synchronous, so it can happen either before the migration, or after the vcpu has been fully migrated and its execution has been resumed. GICC_DIR is written in response to a maintenance interrupt being fired, so GICC_DIR will be written either before the vcpu has been migrated or after.