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

[Andrii Anisov <andrii.anisov@xxxxxxxxx>]

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.

> 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?

> 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.

--
Sincerely,
Andrii Anisov.

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