[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/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



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