From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v4 08/10] xen/arm: second irq injection while the first irq is still inflight Date: Fri, 21 Mar 2014 16:46:23 +0000 Message-ID: References: <1395232325-19226-8-git-send-email-stefano.stabellini@eu.citrix.com> <1395408173.19839.65.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: <1395408173.19839.65.camel@kazak.uk.xensource.com> 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, jtd@galois.com, xen-devel@lists.xensource.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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. > > 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. > > Signed-off-by: Stefano Stabellini > > > > --- > > Changes in v3: > > - do not use the PENDING and ACTIVE state for HW interrupts; > > - unify the inflight and non-inflight code paths in > > vgic_vcpu_inject_irq. > > --- > > xen/arch/arm/gic.c | 89 +++++++++++++++++++++++++++++---------------------- > > xen/arch/arm/vgic.c | 33 ++++++++++--------- > > 2 files changed, 68 insertions(+), 54 deletions(-) > > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index 3f061cf..128d071 100644 > > --- a/xen/arch/arm/gic.c > > +++ b/xen/arch/arm/gic.c > > @@ -67,6 +67,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id); > > /* Maximum cpu interface per GIC */ > > #define NR_GIC_CPU_IF 8 > > > > +static void _gic_clear_lr(struct vcpu *v, int i); > > Single underbar prefixes are reserved for the compiler. > > gic_clear_one_lr would be an adequate name I think. OK > You could also have done this at the time you introduced gic_clear_lrs, > which would save me now asking: is the split into _gic_clear_lr pure > code motion or are there actual substantive changes in it? It is not pure code motion, the changes are listed in the first paragraph of the commit message.