Message ID | 20220208123726.3604198-1-maz@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: arm64: vgic: Read HW interrupt pending state from the HW | expand |
On Tue, Feb 08, 2022 at 12:37:26PM +0000, Marc Zyngier wrote: > It appears that a read access to GIC[DR]_I[CS]PENDRn doesn't always > result in the pending interrupts being accurately reported if they are > mapped to a HW interrupt. This is particularily visible when acking > the timer interrupt and reading the GICR_ISPENDR1 register immediately > after, for example (the interrupt appears as not-pending while it really > is...). > > This is because a HW interrupt has its 'active and pending state' kept > in the *physical* distributor, and not in the virtual one, as mandated > by the spec (this is what allows the direct deactivation). The virtual > distributor only caries the pending and active *states* (note the > plural, as these are two independent and non-overlapping states). > > Fix it by reading the HW state back, either from the timer itself or > from the distributor if necessary. > > Reported-by: Ricardo Koller <ricarkol@google.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/vgic/vgic-mmio.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c > index 7068da080799..49837d3a3ef5 100644 > --- a/arch/arm64/kvm/vgic/vgic-mmio.c > +++ b/arch/arm64/kvm/vgic/vgic-mmio.c > @@ -248,6 +248,8 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu, > IRQCHIP_STATE_PENDING, > &val); > WARN_RATELIMIT(err, "IRQ %d", irq->host_irq); > + } else if (vgic_irq_is_mapped_level(irq)) { > + val = vgic_get_phys_line_level(irq); > } else { > val = irq_is_pending(irq); > } > -- > 2.34.1 > Thanks Marc! Tested this fix with a selftest that we are planning to upstream soon. It fires and handles arch timer IRQs while checking the pending state along the way. Tested-by: Ricardo Koller <ricarkol@google.com> Reviewed-by: Ricardo Koller <ricarkol@google.com>
diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c index 7068da080799..49837d3a3ef5 100644 --- a/arch/arm64/kvm/vgic/vgic-mmio.c +++ b/arch/arm64/kvm/vgic/vgic-mmio.c @@ -248,6 +248,8 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu, IRQCHIP_STATE_PENDING, &val); WARN_RATELIMIT(err, "IRQ %d", irq->host_irq); + } else if (vgic_irq_is_mapped_level(irq)) { + val = vgic_get_phys_line_level(irq); } else { val = irq_is_pending(irq); }
It appears that a read access to GIC[DR]_I[CS]PENDRn doesn't always result in the pending interrupts being accurately reported if they are mapped to a HW interrupt. This is particularily visible when acking the timer interrupt and reading the GICR_ISPENDR1 register immediately after, for example (the interrupt appears as not-pending while it really is...). This is because a HW interrupt has its 'active and pending state' kept in the *physical* distributor, and not in the virtual one, as mandated by the spec (this is what allows the direct deactivation). The virtual distributor only caries the pending and active *states* (note the plural, as these are two independent and non-overlapping states). Fix it by reading the HW state back, either from the timer itself or from the distributor if necessary. Reported-by: Ricardo Koller <ricarkol@google.com> Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/vgic/vgic-mmio.c | 2 ++ 1 file changed, 2 insertions(+)