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

[Andrii Anisov <andrii.anisov@xxxxxxxxx>]

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
   VM

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.
--
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel