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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 11 Dec 2013, Julien Grall wrote:
> On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
> > From: Julien Grall <julien.grall@xxxxxxxxxx>
> > 
> > Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
> > Enable IRQs when the guest requests it, not unconditionally at boot time.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > Changes in v2:
> > - protect startup and shutdown with gic and desc locks.
> > ---
> >   xen/arch/arm/gic.c  |   46 ++++++++++++++++++++++++++--------------------
> >   xen/arch/arm/vgic.c |    6 ++----
> >   2 files changed, 28 insertions(+), 24 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 5e60c5a..62330dd 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -133,16 +133,26 @@ static void gic_irq_enable(struct irq_desc *desc)
> >   {
> >       int irq = desc->irq;
> > 
> > +    spin_lock(&desc->lock);
> > +    spin_lock(&gic.lock);
> >       /* Enable routing */
> >       GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
> > +    desc->status &= ~IRQ_DISABLED;
> > +    spin_unlock(&gic.lock);
> > +    spin_unlock(&desc->lock);
> >   }
> 
> I think we should have something like that:
> 
> desc->status &= ...
> dsb
> GICD[...] = ...
> 
> As soon as the interrupt is enabled in the distributor, it can be fired. With
> your solution, another CPU (and even this one because the interrupt is not
> disabled), can receive the interrupt. But it won't work because the IRQ is
> marked as disabled.
> 
> So you also should disable interrupt here.

I think that you are right on both points. I'll make the changes.

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