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

> +{
> +    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?

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.


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

Ian


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