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

[Julien Grall <julien.grall@xxxxxxx>]

On 12/12/2018 17:59, Andrii Anisov wrote:
> On 12.12.18 19:49, Julien Grall wrote:
> > Those conditions are not sufficient to ensure desc->action is not NULL. You 
> > also need to take the spinlock.
> Agree. Should I describe it as following?
>
> > > Under desc->lock taken:
> > > An IRQ with _IRQ_GUEST flag set always has an action.
> > > An IRQ with _IRQ_DISABLED flag cleared always have an action.
This looks better.

> > While looking at the code, I noticed an interesting race with the release code.
> As I understand the race in not directly linked to this patch. Is it correct?
That's correct. I actually noticed when checking whether the commit message was 
matching the behavior.
> > 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?
> Let me spend a bit more time to look into that
>
> Other wording and grammatical nits will be addressed as suggested.
--
Julien Grall

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