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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.

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