[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.

Andrii Anisov.

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.