Re: [Xen-devel] [PATCH 5/5] xen/arm: Only enable physical IRQs when the guest asks

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-06-25 at 17:19 +0100, Stefano Stabellini wrote:

We really need to get something along these lines into 4.4, as a bug fix
I think. It seems from the below that the issue which this patch tries
to address was actually observed on vexpress, although it doesn't seem
likely to be specific to any platform.


> > @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const 
> > struct dt_irq *irq,
> >          goto out;
> >      }
> >  
> > +    p->desc = desc;
> > +
> >  out:
> >      spin_unlock(&gic.lock);
> >      spin_unlock_irqrestore(&desc->lock, flags);

> > @@ -685,10 +688,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
> > irq, int virtual)
> >  
> >      n->irq = irq;
> >      n->priority = priority;
> > -    if (!virtual)
> > -        n->desc = irq_to_desc(irq);
> > -    else
> > -        n->desc = NULL;
> >  
> >      /* the irq is enabled */
> >      if ( rank->ienable & (1 << (irq % 32)) )
> 
> I don't quite understand why are you changing where the "desc"
> assignement is done.
> If it is a cleanup you shouldn't mix it with a patch like this that is
> supposed to fix a specific issue. Otherwise please explain why you need
> this change.

Was this because gic_router_irq_to_guest has set p->desc as shown above
and therefore the feeling was that it wasn't required here?

I don't think this can be right though, gic_route_irq_to_guest is only
called for SPIs routed via the DTB or the platform specific mapping
hook. In particular it is never called for the various PPIs, such as the
timer PPI and the evtchn PPI.

As it happens the existing PPIs which we pass to the guest are a
virtual, and perhaps we can already rely on n->desc==NULL already in
that case.

TBH, the existing code here is very weird, n->desc should have been set
(possibly on all vcpus) at the time the interrupt was configured/routed,
it certainly shouldn't be updated on every injection. In practice I
think it only happened on the first and was static thereafter so this is
just an odd form of lazy initialisation. If it is in fact changing on
each injection then I've no idea what this code is doing ;-)

Ian.


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