Re: [PATCH v4 08/10] xen/arm: second irq injection while the first irq is still inflight

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

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 <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 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.