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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.


> > > > 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?
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.


> > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > > 
> > > > ---
> > > > 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.
> 
> It's not at all clear which of those changes impact the code which has
> moved.
> 
> Please can you do the code motion separately then or better yet just
> introduce the code in the right place to start with so that the changes
> here are just changes.

OK

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel