Re: [Xen-devel] [PATCH v6 14/15] xen/arm: clear pending irq queues on do_psci_cpu_on

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-05-03 at 17:20 +0100, Stefano Stabellini wrote:
> On Fri, 3 May 2013, Ian Campbell wrote:
> > On Fri, 2013-05-03 at 11:51 +0100, Stefano Stabellini wrote:
> > > Don't inject irqs to vcpus that are down.
> > > 
> > > Also when (re)activating a vcpu, clear the vgic and gic irq queues: we
> > > don't want to inject any irqs that couldn't be handled by the vcpu right
> > > before going offline.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > ---
> > >  xen/arch/arm/gic.c        |   12 ++++++++++++
> > >  xen/arch/arm/psci.c       |    2 ++
> > >  xen/arch/arm/vgic.c       |   16 ++++++++++++++--
> > >  xen/include/asm-arm/gic.h |    2 ++
> > >  4 files changed, 30 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > index 8a49e12..a213da5 100644
> > > --- a/xen/arch/arm/gic.c
> > > +++ b/xen/arch/arm/gic.c
> > > @@ -576,6 +576,18 @@ static void gic_restore_pending_irqs(struct vcpu *v)
> > >  
> > >  }
> > >  
> > > +void gic_clear_pending_irqs(struct vcpu *v)
> > 
> > Should be vgic_clear_... ?? and be in vgic.c?
> 
> we have both gic_clear_pending_irqs and vgic_clear_pending_irqs (the
> latter calls the former)
> 
> 
> > > +{
> > > +    struct pending_irq *p, *t;
> > > +    unsigned long flags;
> > > +
> > > +    spin_lock_irqsave(&gic.lock, flags);
> > 
> > Given the above is this really the right lock? Should it not be
> > v->arch.vgic.lock?
> 
> No, it needs to be the gic lock (it is protecting gic state).

v->arch.vgic.lr_pending is gic state?

As I say, I think this stuff needs rationalising, it has become a bit
confused as SMP host and guest have been added and as our understanding
of the requirements has expanded etc.

> > Hrm, looking at the code, it seems we use that lock for most other
> > lr_pending manipulations. I don't think that is strictly correct (it
> > seems to be a global big lock protection a mixture of PCPU and VCPU
> > local resources), but at least this code is not making anything worse.
> 
> Right, I noticed that.
> 
>  
> > > +    v->arch.lr_mask = 0;
> > > +    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> > > +        list_del_init(&p->lr_queue);
> > > +    spin_unlock_irqrestore(&gic.lock, flags);
> > > +}
> > > +
> > >  static void gic_inject_irq_start(void)
> > >  {
> > >      register_t hcr = READ_SYSREG(HCR_EL2);
> > > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> > > index 562ef0b..6886094 100644
> > > --- a/xen/arch/arm/psci.c
> > > +++ b/xen/arch/arm/psci.c
> > > @@ -15,6 +15,7 @@
> > >  #include <xen/types.h>
> > >  
> > >  #include <asm/current.h>
> > > +#include <asm/gic.h>
> > >  #include <asm/psci.h>
> > >  
> > >  int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
> > > @@ -53,6 +54,7 @@ int do_psci_cpu_on(uint32_t vcpuid, register_t 
> > > entry_point)
> > >   }
> > >   domain_unlock(d);
> > >  
> > > +    vgic_clear_pending_irqs(v);
> > >      vcpu_wake(v);
> > >  
> > >      return PSCI_SUCCESS;
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 0e9cc4a..f9c1a6b 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -641,6 +641,18 @@ struct pending_irq *irq_to_pending(struct vcpu *v, 
> > > unsigned int irq)
> > >      return n;
> > >  }
> > >  
> > > +void vgic_clear_pending_irqs(struct vcpu *v)
> > > +{
> > > +    struct pending_irq *p, *t;
> > > +    unsigned long flags;
> > > +
> > > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > +    list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, 
> > > inflight )
> > > +        list_del_init(&p->inflight);
> > > +    gic_clear_pending_irqs(v);
> > > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > > +}
> > > +
> > >  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
> > >  {
> > >      int idx = irq >> 2, byte = irq & 0x3;
> > > @@ -652,8 +664,8 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned 
> > > int irq, int virtual)
> > >  
> > >      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > >  
> > > -    /* irq already pending */
> > > -    if (!list_empty(&n->inflight))
> > > +    /* vcpu offline or irq already pending */
> > > +    if (test_bit(_VPF_down, &v->pause_flags) || 
> > > !list_empty(&n->inflight))
> > 
> > Strictly speaking I don't think we need both this check and the
> > clear_pinding_irqs on resume. vgic_vcpu_inject_irq is quite a common
> > path -- is it worth omitting this here?
> 
> A simple test_bit should be fast, besides it is necessary to prevent
> possible races on the do_psci_cpu_off: one vcpu is executing cpu_off
> but it is still running (is_running is set) while the other is sending
> it an irq via vgic_vcpu_inject_irq.

vgic_clear_pending_irqs beats any possible race with a big hammer.

In any case I don't think this check closes the race as you seem to
think, the cpu_off CPU could not have yet set VPF_down.

I don't think this is a race you can win without a big lock around
interrupt injection & cpu up/down, but luckily I don't think it is
necessary since clear_pending_irqs takes care of it.

However I do think you need to call vgic_clear_pending_irqs before you
clear VPF_down in cpu_up (i.e. before the call to arch_set_info_guest).
Otherwise you will drop any interrupts sent while the processor is
coming up.

> I would rather avoid these subtle scenarios altogether.

Ian.


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