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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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)

?

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

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

Ian.


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