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

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

Hello Andre,

Please see my comments below:

On 23.11.18 14:18, Andre Przywara wrote:
Fundamentally there is a semantic difference between edge and level triggered IRQs: When the guest has handled an *edge* IRQ (EOIed so
the LR's state goes to 0), this is done and dusted, and Xen doesn't
need to care about this anymore until the next IRQ occurs.> For level
triggered IRQs, even though the guest has handled it, we need to
resample the (potentially virtual) IRQ line, as it may come up or down at the *device*'s discretion: the interrupt reason might have
gone away (GPIO condition no longer true), even before we were able
to inject it, or there might be another interrupt reason not yet
handled (incoming serial character while serving a transmit
interrupt). Also typically it's up to the interrupt handler to
confirm handling the interrupt, either explicitly by clearing an
interrupt bit in some status register or implicitly, for instance by
draining a FIFO, say on a serial device. So even though from the
(V)GIC's point of view the interrupt has been processed (EOIed), it
might still be pending.
So, as I understand the intended behavior of a vGIC for the level
interrupt is following cases:
1. in case the interrupt line is still active from HW side, but
   interrupt handler from VM EOIs the interrupt, it should
   be signaled to vCPU by vGIC again
2. in case a peripheral deactivated interrupt line, but VM did not
   activated it yet, this interrupt should be removed from pending for

IMO, case 1 is indirectly supported by old vgic. For HW interrupts its pretty natural: deactivation by VM in VGIC leads to deactivation in GIC, so the interrupt priority is restored and GIC will trap PCPU to reinsert it. This will be seen by VM as immediate IRQ trap after EOI. Also Case 2 is not implemented in the old vgic. It is somehow supported by new vgic, though it also relies on the trap to the hypervisor to commit the update to LRs. But it's rather a problem of GIC arch/implementation, which does not signal CPU nor updates associated LR on level interrupt deassertion.

My intimate "old Xen VGIC" knowledge has been swapped out from my
brain meanwhile, but IIRC Xen treats every IRQ as if it would be an
edge IRQ. Which works if the guest's interrupt handler behaves
correctly. Most IRQ handlers have a loop to iterate over all possible
interrupt reasons and process them, so the line goes indeed down
before they EOI the IRQ.

I've spent some time to look through the new vgic implementation and I have a note about it: It's not clear why are you probing level interrupts on guest->hyp transition. While it targets case 2 described above, it seems to be more relevant to probe the level interrupts right before hyp->guest transition. Because vcpu might be descheduled and while it hangs on scheduler queues interrupt line level has more chances to be changed by peripheral itself. Also I'm pretty scared of new vgic locking scheme with per-irq locks and locking logic i.e. in vgic_queue_irq_unlock() function. Also sorting list in vgic_flush_lr_state() with vgic_irq_cmp() looks very expensive.

But, for sure all that stuff performance should be properly evaluated and measured.

Andrii Anisov.

Xen-devel mailing list



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