[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

Title: s/avalability/availability/

On 12/12/2018 16:55, Andrii Anisov wrote:
From: Andrii Anisov <andrii_anisov@xxxxxxxx>

An IRQ with _IRQ_GUEST flag set always has an action.
An IRQ with _IRQ_DISABLED flag cleared always have an action.


Those conditions are not sufficient to ensure desc->action is not NULL. You also need to take the spinlock.

While looking at the code, I noticed an interesting race with the release code. 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?

Those flags checks cover all accesses to desc->action in do_IRQ, > so we can skip 
desc->action check.

"in non-debug build".

Still keep it in place for debug build.

"Keep in place for debug build to help diagnostics potential misconfiguration".

Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
Reviewed-by: Julien Grall <julien.grall@xxxxxxx>

Please don't add a reviewed-by tag until it was explicitly written by the 

  xen/arch/arm/irq.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index d6a0273..4a02cc1 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -209,12 +209,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, 
int is_fiq)
+#ifndef NDEBUG
      if ( !desc->action )
          printk("Unknown %s %#3.3x\n",
                 is_fiq ? "FIQ" : "IRQ", irq);
          goto out;
if ( test_bit(_IRQ_GUEST, &desc->status) )


Julien Grall

Xen-devel mailing list



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