From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 08/10] xen/arm: second irq injection while the first irq is still inflight Date: Mon, 24 Mar 2014 12:57:44 +0000 Message-ID: <1395665864.6294.24.camel@kazak.uk.xensource.com> References: <1395232325-19226-8-git-send-email-stefano.stabellini@eu.citrix.com> <1395408173.19839.65.camel@kazak.uk.xensource.com> <1395421482.25521.36.camel@kazak.uk.xensource.com> 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, jtd@galois.com, xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On Mon, 2014-03-24 at 12:54 +0000, Stefano Stabellini wrote: > On Fri, 21 Mar 2014, Ian Campbell wrote: > > On Fri, 2014-03-21 at 16:46 +0000, Stefano Stabellini wrote: > > > On Fri, 21 Mar 2014, Ian Campbell wrote: > > > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote: > > > > > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq > > > > > while the first one is still active. > > > > > If the first irq is already pending (not active), just clear > > > > > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is > > > > > already visible by the guest. > > > > > If the irq has already been EOI'ed then just clear the GICH_LR right > > > > > away and move the interrupt to lr_pending so that it is going to be > > > > > reinjected by gic_restore_pending_irqs on return to guest. > > > > > > > > > > If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_PENDING > > > > > and send an SGI. The target cpu is going to be interrupted and call > > > > > gic_clear_lrs, that is going to take the same actions. > > > > > > > > > > Unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq. > > > > > > > > > > Do not call vgic_vcpu_inject_irq from gic_inject if > > > > > evtchn_upcall_pending is set. If we remove that call, we don't need to > > > > > special case evtchn_irq in vgic_vcpu_inject_irq anymore. > > > > > > > > That's the consequence of removing it, but what is the rationale for why > > > > it is OK? > > > > > > Because with this patch we are able to inject a second interrupt while > > > the first one is still in progress. > > > > IOW gic_inject of the evtchn just works even if the evtchn is already > > pending? > > > > Don't we then have a problem with a potentialy spurious evtchn upcall? > > > > Event chn A raised > > -> IRQ inject > > -> GUest takes IRQ > > -> Guest enters evtchn handling loop > > -> handles A > > Event chn B raised > > -> IRQ becomes pendign again > > -> handles B > > -> Finishes evtchn handling loop > > -> unmasks interrupt > > -> Guest takes anotheer IRQ > > -> Nothing to do (B already dealt with) > > > > ? > > No, because vcpu_mark_events_pending only calls vgic_vcpu_inject_irq if > evtchn_upcall_pending is not set. > > We only inject a second evtchn_irq if the guest actually needs a second > notification. OK, thanks for the explanation. > > > > > We also need to force the first injection of evtchn_irq (call > > > > > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending > > > > > is already set by common code on vcpu creation. > > > > > > > > This is because the common code expects that the guest is moving from > > > > sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, but > > > > on ARM we start off that way anyway. > > > > > > > > I suppose it's a minor wrinkle, but I wonder if we can get rid of it... > > > > > > Do you mean getting rid of evtchn_upcall_pending being set at vcpu > > > creation? Wouldn't that cause possible undesirable side effects to > > > guests that came to rely on it somehow? It would be an ABI change. > > > > I mean precisely for the boot cpu when it is brought online, there isn't > > much sense in immediately taking an interrupt when that cpu enables > > them. > > > > The reason for setting it right now is only for the case of a cpu moving > > its vcpu info, to ensure it can't miss anything. > > What about secondary vcpus? Should we keep the spurious injection for > them? Not sure. No? > In any case I agree with you that the current behaviour is not nice, > however I am worried about changing a guest visible interface like this > one, that would affect x86 guests too. Oh, I certainly wouldn't change this for x86! Or maybe I would change it but only for cpus which are not online at the time when the init happens (which is effectively the difference between the x86 and arm cases)