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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 7 May 2013, Ian Campbell wrote:
> 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?

yep


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

I agree, but it's something for a separate patch


> > > 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 am trying to prevent vgic_vcpu_inject_irq from calling vcpu_unblock
on a vcpu that is going down.

Given that VPF_down is set before calling vcpu_sleep_nosync, I think
that the test_bit I added should be enough to prevent the race.

 
> 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 guess it would make sense but keeping in mind that the vcpu is not
actually up until vcpu_wake is called, I don't think it would make a
difference.

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