From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v8 03/10] xen/arm: inflight irqs during migration Date: Thu, 24 Jul 2014 15:48:12 +0100 Message-ID: References: <1405016003-19131-3-git-send-email-stefano.stabellini@eu.citrix.com> <1405601051.31127.9.camel@kazak.uk.xensource.com> <1406129931.21827.24.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="1342847746-616341116-1406212903=:2293" Return-path: In-Reply-To: <1406129931.21827.24.camel@kazak.uk.xensource.com> Content-ID: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: julien.grall@citrix.com, xen-devel@lists.xensource.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org --1342847746-616341116-1406212903=:2293 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: QUOTED-PRINTABLE Content-ID: On Wed, 23 Jul 2014, Ian Campbell wrote: > On Wed, 2014-07-23 at 15:45 +0100, Stefano Stabellini wrote: > > On Thu, 17 Jul 2014, Ian Campbell wrote: > > > On Thu, 2014-07-10 at 19:13 +0100, Stefano Stabellini wrote: > > > > We need to take special care when migrating irqs that are already > > > > inflight from one vcpu to another. See "The effect of changes to an > > > > GICD_ITARGETSR", part of chapter 4.3.12 of the ARM Generic Interrup= t > > > > Controller Architecture Specification. > > > >=20 > > > > The main issue from the Xen point of view is that the lr_pending an= d > > > > inflight lists are per-vcpu. The lock we take to protect them is al= so > > > > per-vcpu. > > > >=20 > > > > In order to avoid issues, if the irq is still lr_pending, we can > > > > immediately move it to the new vcpu for injection. > > > >=20 > > > > Otherwise if it is in a GICH_LR register, set a new flag > > > > GIC_IRQ_GUEST_MIGRATING, so that we can recognize when we receive a= n irq > > > > while the previous one is still inflight (given that we are only de= aling > > > > with hardware interrupts here, it just means that its LR hasn't bee= n > > > > cleared yet on the old vcpu). If GIC_IRQ_GUEST_MIGRATING is set, w= e > > > > only set GIC_IRQ_GUEST_QUEUED and interrupt the old vcpu. To know w= hich > > > > one is the old vcpu, we introduce a new field to pending_irq, calle= d > > > > vcpu_migrate_from. > > > > When clearing the LR on the old vcpu, we take special care of injec= ting > > > > the interrupt into the new vcpu. To do that we need to release the = old > > > > vcpu lock before taking the new vcpu lock. > > >=20 > > > I still think this is an awful lot of complexity and scaffolding for > > > something which is rare on the scale of things and which could be alm= ost > > > trivially handled by requesting a maintenance interrupt for one EOI a= nd > > > completing the move at that point. > >=20 > > Requesting a maintenance interrupt is not as simple as it looks: > > - ATM we don't know how to edit a living GICH_LR register, we would hav= e > > to add a function for that; >=20 > That doesn't sound like a great hardship. Perhaps you can reuse the > setter function anyhow. >=20 > > - if we request a maintenance interrupt then we also need to EOI the > > physical IRQ, that is something that we don't do anymore (unless > > PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI but that is another matter). We woul= d > > need to understand that some physical irqs need to be EOI'ed by Xen and > > some don't. >=20 > I was thinking the maintenance interrupt handler would take care of > this. In that case we would have to resurrect the code to loop over the GICH_EISR* registers from maintenance_interrupt. Anything can be done, I am just pointing out that this alternative approach is not as cheap as it might sound. > > Also requesting a maintenance interrupt would only guarantee that the > > vcpu is interrupted as soon as possible, but it won't save us from > > having to introduce GIC_IRQ_GUEST_MIGRATING. >=20 > I didn't expect GIC_IRQ_GUEST_MIGRATING to go away. If nothing else you > would need it to flag to the maintenance IRQ that it needs to EOI > +complete the migration. >=20 > > It would only let us skip > > adding vcpu_migrate_from and the 5 lines of code in > > vgic_vcpu_inject_irq. >=20 > And the code in gic_update_one_lr I think, and most of > vgic_vcpu_inject-cpu. > And more than the raw lines of code the > *complexity* would be much lower. I don't know about the complexity. One thing is to completely get rid of maintenance interrupts. Another is to get rid of them in most cases but not all. Having to deal both with not having them and with having them, increases complexity, at least in my view. It simpler to think that you have them all the times or never. In any case replying to this email made me realize that there is indeed a lot of unneeded code in this patch, especially given that writing to the physical ITARGETSR is guaranteed to affect pending (non active) irqs. From the ARM ARM: "Software can write to an GICD_ITARGETSR at any time. Any change to a CPU targets field value: [...] Has an effect on any pending interrupts. This means: =E2=80=94 adding a CPU interface to the target list of a pending interrupt= makes that interrupt pending on that CPU interface =E2=80=94 removing a CPU interface from the target list of a pending inter= rupt removes the pending state of that interrupt on that CPU interface." I think we can rely on this behaviour. Thanks to patch #5 we know that we'll be receiving the second physical irq on the old cpu and from then on the next ones always on the new cpu. So we won't need vcpu_migrate_from, the complex ordering of MIGRATING and QUEUED, or the maintenance_interrupt. --1342847746-616341116-1406212903=:2293 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --1342847746-616341116-1406212903=:2293--