On Wed, Aug 30, 2017 at 12:13:32PM +0200, Auger Eric wrote: > Hi Christoffer, > > On 30/08/2017 11:20, Christoffer Dall wrote: > > On Wed, Aug 30, 2017 at 10:19:38AM +0200, Auger Eric wrote: > >> Hi Christoffer, > >> > >> On 29/08/2017 11:39, Christoffer Dall wrote: > >>> Level-triggered mapped IRQs are special because we only observe rising > >>> edges as input to the VGIC, and we don't set the EOI flag and therefore > >>> are not told when the level goes down, so that we can re-queue a new > >>> interrupt when the level goes up. > >>> > >>> One way to solve this problem is to side-step the logic of the VGIC and > >>> special case the validation in the injection path, but it has the > >>> unfortunate drawback of having to peak into the physical GIC state > >>> whenever we want to know if the interrupt is pending on the virtual > >>> distributor. > >>> > >>> Instead, we can maintain the current semantics of a level triggered > >>> interrupt by sort of treating it as an edge-triggered interrupt, > >>> following from the fact that we only observe an asserting edge. This > >>> requires us to be a bit careful when populating the LRs and when folding > >>> the state back in though: > >>> > >>> * We lower the line level when populating the LR, so that when > >>> subsequently observing an asserting edge, the VGIC will do the right > >>> thing. > >>> > >>> * If the guest never acked the interrupt while running (for example if > >>> it had masked interrupts at the CPU level while running), we have > >>> to preserve the pending state of the LR and move it back to the > >>> line_level field of the struct irq when folding LR state. > >>> > >>> If the guest never acked the interrupt while running, but changed the > >>> device state and lowered the line (again with interrupts masked) then > >>> we need to observe this change in the line_level. > >>> > >>> Both of the above situations are solved by sampling the physical line > >>> and set the line level when folding the LR back. > >>> > >>> * Finally, if the guest never acked the interrupt while running and > >>> sampling the line reveals that the device state has changed and the > >>> line has been lowered, we must clear the physical active state, since > >>> we will otherwise never be told when the interrupt becomes asserted > >>> again. > >>> > >>> This has the added benefit of making the timer optimization patches > >>> (https://lists.cs.columbia.edu/pipermail/kvmarm/2017-July/026343.html) a > >>> bit simpler, because the timer code doesn't have to clear the active > >>> state on the sync anymore. It also potentially improves the performance > >>> of the timer implementation because the GIC knows the state or the LR > >>> and only needs to clear the > >>> active state when the pending bit in the LR is still set, where the > >>> timer has to always clear it when returning from running the guest with > >>> an injected timer interrupt. > >>> > >>> Signed-off-by: Christoffer Dall <cdall@xxxxxxxxxx> > >>> --- > >>> virt/kvm/arm/vgic/vgic-v2.c | 29 +++++++++++++++++++++++++++++ > >>> virt/kvm/arm/vgic/vgic-v3.c | 29 +++++++++++++++++++++++++++++ > >>> virt/kvm/arm/vgic/vgic.c | 23 +++++++++++++++++++++++ > >>> virt/kvm/arm/vgic/vgic.h | 7 +++++++ > >>> 4 files changed, 88 insertions(+) > >>> > >>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > >>> index e4187e5..f7c5cb5 100644 > >>> --- a/virt/kvm/arm/vgic/vgic-v2.c > >>> +++ b/virt/kvm/arm/vgic/vgic-v2.c > >>> @@ -104,6 +104,26 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu) > >>> irq->pending_latch = false; > >>> } > >>> > >>> + /* > >>> + * Level-triggered mapped IRQs are special because we only > >>> + * observe rising edges as input to the VGIC. > >>> + * > >>> + * If the guest never acked the interrupt we have to sample > >>> + * the physical line and set the line level, because the > >>> + * device state could have changed or we simply need to > >>> + * process the still pending interrupt later. > >>> + * > >>> + * If this causes us to lower the level, we have to also clear > >>> + * the physical active state, since we will otherwise never be > >>> + * told when the interrupt becomes asserted again. > >>> + */ > >>> + if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) { > >>> + irq->line_level = vgic_irq_line_level(irq); > >>> + > >>> + if (!irq->line_level) > >>> + vgic_irq_clear_phys_active(irq); > >>> + } > >>> + > >>> spin_unlock(&irq->irq_lock); > >>> vgic_put_irq(vcpu->kvm, irq); > >>> } > >>> @@ -161,6 +181,15 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr) > >>> val |= GICH_LR_EOI; > >>> } > >>> > >>> + /* > >>> + * Level-triggered mapped IRQs are special because we only observe > >>> + * rising edges as input to the VGIC. We therefore lower the line > >>> + * level here, so that we can take new virtual IRQs. See > >>> + * vgic_v2_fold_lr_state for more info. > >>> + */ > >>> + if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) > >>> + irq->line_level = false; > >>> + > >>> /* The GICv2 LR only holds five bits of priority. */ > >>> val |= (irq->priority >> 3) << GICH_LR_PRIORITY_SHIFT; > >>> > >>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > >>> index 96ea597..e377036 100644 > >>> --- a/virt/kvm/arm/vgic/vgic-v3.c > >>> +++ b/virt/kvm/arm/vgic/vgic-v3.c > >>> @@ -94,6 +94,26 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu) > >>> irq->pending_latch = false; > >>> } > >>> > >>> + /* > >>> + * Level-triggered mapped IRQs are special because we only > >>> + * observe rising edges as input to the VGIC. > >>> + * > >>> + * If the guest never acked the interrupt we have to sample > >>> + * the physical line and set the line level, because the > >>> + * device state could have changed or we simply need to > >>> + * process the still pending interrupt later. > >>> + * > >>> + * If this causes us to lower the level, we have to also clear > >>> + * the physical active state, since we will otherwise never be > >>> + * told when the interrupt becomes asserted again. > >>> + */ > >>> + if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT)) { > >>> + irq->line_level = vgic_irq_line_level(irq); > >> > >> I don't see anything related to the GICD_ISPENDING/ICPENDING. > > > > vgic_irq_line_level() reads GICD_ISPEND. Not sure what you mean? > > > >> In the > >> last thread, we said we were obliged to set the pending bit on the > >> physical distributor to avoid the guest deactivating a non active > >> physical IRQ. > > > > Did we? I don't remember this, and this doesn't make sense to me. The > > only constraint I think we have is that when setting the HW bit in the > > LR, the interrupt must be active on the physical side, so that a > > deactivate by the guest is also a deactivate on the physical side. > > That what I understood from this thread: > https://lkml.org/lkml/2017/7/25/534 Right, but that was specific to how we handle the guest writing to PEND/ACT on the virtual distributor. > > At the moment the LR HW bit is set whatever the source of the IRQ I > think, HW driven or ISPENDRn driven (we only test irq->hw) > Ah, I didn't actually consider that we could special-case the pending_latch for a HW interrupt. I suppose we should actually consider that if someone tries to map an edge-triggered SPI. Let me have a look at that. > > > > >> If we still intend to do so, with that addition, aren't we > >> likely to deactivate the physical IRQ before the guest? > > > > Do you mean if it's pending+active in the LR? That can never happen for > > a mapped (HW bit set) interrupt. > > > > So this particular code sequence means that > > (val & ICH_LR_STATE) == ICH_LR_PENDING_BIT > > > > and we never clear the physical active state while the virtual interrupt > > is active. > > No my scenario was: > IRQ originates from a ISPENDRn. virtual IRQ from a write to the VGIC ISPENDRn, correct? > I Thought we set the physical > distributor state accordingly. pIRQ is active. vIRQ is pending. On fold > we detect the line is low and we deactivate the pIRQ. pending_latch is > still true. The vIRQ will be processed but its physical IRQ has been > deactivated. > As long as we properly handle pending_latch and guest writes to the VGIC PEND/ACT registers, I don't see a problem with this code. > > There is an interesting point in terms of handling guest accesses to the > > virtual distributor GICD_ISPENDR, GICD_ICPENDR, GICD_ISACTIVER, and > > GICD_ICACTIVER where we must also propogate those changes to the > > physical side. > > > > Was this what you meant? > yes I think we also need to address this to get the whole picture. > Yes, I'll respin. Thanks, -Christoffer
