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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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


> 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.
I would rather avoid these subtle scenarios altogether.

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