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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
