Re: [Xen-devel] [PATCH 2/2] arm/irq: skip action avalability check for non-debug build

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 12 Dec 2018, Julien Grall wrote:
> Title: s/avalability/availability/
> 
> On 12/12/2018 16:55, Andrii Anisov wrote:
> > From: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > 
> > An IRQ with _IRQ_GUEST flag set always has an action.
> > An IRQ with _IRQ_DISABLED flag cleared always have an action.
> 
> s/have/has/
> 
> Those conditions are not sufficient to ensure desc->action is not NULL. You
> also need to take the spinlock.
> 
> While looking at the code, I noticed an interesting race with the release
> code. Guest IRQ are released using the function gic_remove_irq_to_guest. The
> sequence is roughly:
> 
> 1) spin_lock(desc->lock);
> 2) writel(desc->irq, ICENABLER);
> 3) set_bit(_IRQ_DISABLED, &desc->status);
> 4) clear_bit(_IRQ_GUES, &desc->status);
> 5) desc->handler = &no_irq_type;
> 6) spin_unlock(desc->lock);
> 
> Even if 2) will disable the interrupt in the hardware, the interrupt may have
> been received earlier on another CPU and waiting on the lock. As soon as the
> lock is taken, the code will notice the irq disabled (thanks to 3)) and will
> then end the interrupt. The callbak end for no_irq_type is a NOP, therefore
> the interrupt will stay active and the priority will not be dropped.
> 
> Because of that, the CPU will never be able to receive interrupt for guest
> anymore. AFAICT, this can only happen if an interrupt is received while
> destroying the assigned domain.
> 
> I think 5) should be replaced with
> 
> desc->handler = gic_hw_ops->gic_host_irq_type;
> 
> Or we potentially need to update no_irq_type and EOI "spurious interrupt".
> 
> I am not entirely sure which way is the best to address the race. Any
> opinions?

I think that changing the .end function of no_irq_type to be the same as
the end function of the host_irq_type controller is the safest option:
yes no_irq_type means no irqs but if we receive an interrupt we should
still EOI it no matter what.

 
> > Those flags checks cover all accesses to desc->action in do_IRQ, > so we can
> > skip desc->action check.
> 
> "in non-debug build".
> 
> > Still keep it in place for debug build.
> 
> "Keep in place for debug build to help diagnostics potential
> misconfiguration".
> 
> > 
> > Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
> 
> Please don't add a reviewed-by tag until it was explicitly written by the
> reviewer.
> 
> > ---
> >   xen/arch/arm/irq.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index d6a0273..4a02cc1 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -209,12 +209,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int
> > irq, int is_fiq)
> >       spin_lock(&desc->lock);
> >       desc->handler->ack(desc);
> >   +#ifndef NDEBUG
> >       if ( !desc->action )
> >       {
> >           printk("Unknown %s %#3.3x\n",
> >                  is_fiq ? "FIQ" : "IRQ", irq);
> >           goto out;
> >       }
> > +#endif
> >         if ( test_bit(_IRQ_GUEST, &desc->status) )
> >       {
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel