[Xen-devel,v7,5/6] xen/arm: physical irq follow virtual irq

Migrate physical irqs to the same physical cpu that is running the vcpu
expected to receive the irqs. That is done when enabling irqs, when the
guest writes to GICD_ITARGETSR and when Xen migrates a vcpu to a
different pcpu.

In case of virq migration, if the virq is inflight and in a GICH_LR
register already, delay migrating the corresponding physical irq until
the virq is EOIed by the guest and the MIGRATING flag has been cleared.
This way we make sure that the pcpu running the old vcpu gets
interrupted with a new irq of the same kind, clearing the GICH_LR sooner.

Introduce a new arch specific function, arch_move_irqs, that is empty on
x86 and implements the vgic irq migration code on ARM.
arch_move_irqs is going to be called by from sched.c.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---

Changes in v7:
- remove checks at the top of gic_irq_set_affinity, add assert instead;
- move irq_set_affinity to irq.c;
- delay setting the affinity of the physical irq when the virq is
MIGRATING until the virq is EOIed by the guest;
- do not set the affinity of MIGRATING irqs from arch_move_irqs.

Changes in v6:
- use vgic_get_target_vcpu instead of _vgic_get_target_vcpu in
arch_move_irqs.

Changes in v5:
- prettify vgic_move_irqs;
- rename vgic_move_irqs to arch_move_irqs;
- introduce helper function irq_set_affinity.
---
 xen/arch/arm/gic-v2.c     |   17 +++++++++++++++--
 xen/arch/arm/gic.c        |    1 +
 xen/arch/arm/irq.c        |    6 ++++++
 xen/arch/arm/vgic.c       |   21 +++++++++++++++++++++
 xen/include/asm-arm/gic.h |    1 +
 xen/include/asm-arm/irq.h |    2 ++
 xen/include/asm-x86/irq.h |    2 ++
 7 files changed, 48 insertions(+), 2 deletions(-)

On 07/03/2014 05:53 PM, Stefano Stabellini wrote:
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 695c232..c3d2853 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -532,9 +532,22 @@ static void gicv2_guest_irq_end(struct irq_desc *desc)
>      /* Deactivation happens in maintenance interrupt / via GICV */
>  }
>  
> -static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
> +static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
>  {
> -    BUG();
> +    volatile unsigned char *bytereg;
> +    unsigned int mask;
> +
> +    ASSERT(!cpumask_empty(cpu_mask));
> +
> +    spin_lock(&gicv2.lock);
> +
> +    mask = gicv2_cpu_mask(cpu_mask);
> +
> +    /* Set target CPU mask (RAZ/WI on uniprocessor) */
> +    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
> +    bytereg[desc->irq] = mask;

The new implemenation of GICv2 is using {read,write}* helpers. Can you
use writeb_relaxed here, please?

[..]

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index b4493a3..69d3040 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -399,6 +399,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
>  
>      if ( list_empty(&p->inflight) )
>      {
> +        irq_set_affinity(p->desc, cpumask_of(new->processor));
>          spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>          return;
>      }
> @@ -407,6 +408,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
>      {
>          list_del_init(&p->lr_queue);
>          list_del_init(&p->inflight);
> +        irq_set_affinity(p->desc, cpumask_of(new->processor));

I think this irq_set_affinity is misplaced. You forgot to handle the
case where the IRQ has been EOIed, and no IRQ has been queued.

Also, it looks like this is done a bit late. the IRQ may fire on the
previous physical CPU again at least once.

This will happen the X-Gene quirk.

Regards,

On Fri, 2014-07-04 at 11:51 +0100, Julien Grall wrote:
> On 07/03/2014 05:53 PM, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 695c232..c3d2853 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -532,9 +532,22 @@ static void gicv2_guest_irq_end(struct irq_desc *desc)
> >      /* Deactivation happens in maintenance interrupt / via GICV */
> >  }
> >  
> > -static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
> > +static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
> >  {
> > -    BUG();
> > +    volatile unsigned char *bytereg;
> > +    unsigned int mask;
> > +
> > +    ASSERT(!cpumask_empty(cpu_mask));
> > +
> > +    spin_lock(&gicv2.lock);
> > +
> > +    mask = gicv2_cpu_mask(cpu_mask);
> > +
> > +    /* Set target CPU mask (RAZ/WI on uniprocessor) */
> > +    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
> > +    bytereg[desc->irq] = mask;
> 
> The new implemenation of GICv2 is using {read,write}* helpers. Can you
> use writeb_relaxed here, please?

Now called writeb_gicd(mask, GICD_ITARGETSR + desc->irq) in staging...

Ian.

On Fri, 4 Jul 2014, Julien Grall wrote:
> On 07/03/2014 05:53 PM, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 695c232..c3d2853 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -532,9 +532,22 @@ static void gicv2_guest_irq_end(struct irq_desc *desc)
> >      /* Deactivation happens in maintenance interrupt / via GICV */
> >  }
> >  
> > -static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
> > +static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
> >  {
> > -    BUG();
> > +    volatile unsigned char *bytereg;
> > +    unsigned int mask;
> > +
> > +    ASSERT(!cpumask_empty(cpu_mask));
> > +
> > +    spin_lock(&gicv2.lock);
> > +
> > +    mask = gicv2_cpu_mask(cpu_mask);
> > +
> > +    /* Set target CPU mask (RAZ/WI on uniprocessor) */
> > +    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
> > +    bytereg[desc->irq] = mask;
> 
> The new implemenation of GICv2 is using {read,write}* helpers. Can you
> use writeb_relaxed here, please?
> 
> [..]

sure



> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index b4493a3..69d3040 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -399,6 +399,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
> >  
> >      if ( list_empty(&p->inflight) )
> >      {
> > +        irq_set_affinity(p->desc, cpumask_of(new->processor));
> >          spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> >          return;
> >      }
> > @@ -407,6 +408,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
> >      {
> >          list_del_init(&p->lr_queue);
> >          list_del_init(&p->inflight);
> > +        irq_set_affinity(p->desc, cpumask_of(new->processor));
> 
> I think this irq_set_affinity is misplaced. You forgot to handle the
> case where the IRQ has been EOIed, and no IRQ has been queued.

If it was EOI'ed and cleared, then it falls into the first case above.
If it was EOI'ed and not yet cleared, then it falls into the last case
below.


> Also, it looks like this is done a bit late. the IRQ may fire on the
> previous physical CPU again at least once.
>
> This will happen the X-Gene quirk.

That's not a problem, the case is handled correctly.