Messages in this thread Patch in this message | | | From | Valentin Schneider <> | Subject | [RFC PATCH] irqchip/gic: Implement irq_chip->irq_retrigger() | Date | Fri, 10 Jul 2020 15:56:42 +0100 |
| |
While digging around IRQCHIP_EOI_IF_HANDLED and irq/resend.c, it has come to my attention that the IRQ resend situation seems a bit precarious for the GIC(s). Below is a (somewhat structured) dump of my notes/thoughts about that.
IRQCHIP_EOI_IF_HANDLED ======================
If the irqchip doesn't have this flag set, we may issue an irq_eoi() despite not having handled the IRQ in the following cases:
o !irq_may_run() - IRQ may be in progress (handle_irq_event() ongoing) - IRQ is an armed wakeup source (also sets it pending)
o !action or IRQ disabled; in this case it is set as pending.
irq_resend() ============
For the above cases where the IRQ is marked as pending, it means that when we'll go through check_irq_resend() (e.g. when we re-enable the IRQ) we will end up in resend_irqs() which goes through the flow handler again, IOW will issue *another* EOI on the same IRQ.
Now, this is all done via a tasklet, so AFAICT cannot happen in irq context (as defined by in_interrupt() - it can happen at the tail of handling an IRQ if it wasn't nesting).
GIC woes ========
The TL;DR for IRQ handling on the GIC is that we have 3 steps: o Acknowledgement (Ack) o priority drop (PD) o deactivation (D)
The GIC also has an "eoimode" knob that says whether PD and D are split, IOW: o eoimode=0: irq_eoi() does PD + D o eoimode=1: irq_eoi() does D
Regardless of the mode, it is paramount that any PD is o issued on the same CPU that Ack'd the IRQ o issued in reverse order of the Acks
IHI0069D, 4.1.1 Physical CPU interface, Priority drop """ A priority drop must be performed by the same PE that activated the interrupt.
[...]
For each CPU interface, the GIC architecture requires the order of the valid writes to ICC_EOIR0_EL1 and ICC_EOIR1_EL1 to be the exact reverse of the order of the reads from ICC_IAR0_EL1 and ICC_IAR1_EL1 """
IHI0069D, 8.2.9 ICC_EOIR0_EL1, Interrupt Controller End Of Interrupt Register 0 """ A write to this register must correspond to the most recent valid read by this PE from an Interrupt Acknowledge Register, and must correspond to the INTID that was read from ICC_IAR0_EL1, otherwise the system behavior is UNPREDICTABLE. """
For eoimode=1, the PD is hidden from genirq and is contained within the GIC driver. This means that issuing another irq_eoi() will only be re-issuing a D, which I think the GIC can live with (even if issued from a different CPU).
For eoimode=0, it is more troubling, as we break the aforementioned restrictions. That said, IIUC this cannot cause e.g. a bogus running priority reduction due to the tasklet context mentioned above (running priority ought to be idle priority).
Linux hosts will almost always use eoimode=1, so that leaves us with guests running with eoimode=0, and I don't know how common it is (if at all possible) for those to use pm / wakeup IRQs. I suppose that is a reason why this hasn't cropped up before, that or I miserably misunderstood the whole thing.
In any case, the virtual interface follows the same restrictions wrt PD ordering:
IHI0069D 8.3.7 ICV_EOIR0_EL1, Interrupt Controller Virtual End Of Interrupt Register 0 """ A write to this register must correspond to the most recent valid read by this vPE from a Virtual Interrupt Acknowledge Register, and must correspond to the INTID that was read from ICV_IAR0_EL1, otherwise the system behavior is UNPREDICTABLE. """
Change ======
One way to ensure we only get one PD per interrupt activation and maintain the expected ordering is to add the IRQCHIP_EOI_IF_HANDLED flag to all irq-gic chips, but that only really works for eoimode=1; for eoimode=0 that would mean leaving the flow handler without having issued a PD at all.
At the same time, this whole IRQS_PENDING & resend thing feels like something we can handle in hardware: we can leverage irq_chip.irq_retrigger() and use this to mark the interrupt as pending in the GIC itself, in which case we *don't* want to have IRQCHIP_EOI_IF_HANDLED (as the retrigger will lead to another ack+eoi pair).
Implement irq_chip.irq_retrigger() for both GICs.
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> --- drivers/irqchip/irq-gic-v3.c | 7 +++++++ drivers/irqchip/irq-gic.c | 6 ++++++ 2 files changed, 13 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index cc46bc2d634b..c025e8b51464 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -1207,6 +1207,11 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, #define gic_smp_init() do { } while(0) #endif +static int gic_retrigger(struct irq_data *data) +{ + return gic_irq_set_irqchip_state(data, IRQCHIP_STATE_PENDING, true); +} + #ifdef CONFIG_CPU_PM static int gic_cpu_pm_notifier(struct notifier_block *self, unsigned long cmd, void *v) @@ -1242,6 +1247,7 @@ static struct irq_chip gic_chip = { .irq_eoi = gic_eoi_irq, .irq_set_type = gic_set_type, .irq_set_affinity = gic_set_affinity, + .irq_retrigger = gic_retrigger, .irq_get_irqchip_state = gic_irq_get_irqchip_state, .irq_set_irqchip_state = gic_irq_set_irqchip_state, .irq_nmi_setup = gic_irq_nmi_setup, @@ -1258,6 +1264,7 @@ static struct irq_chip gic_eoimode1_chip = { .irq_eoi = gic_eoimode1_eoi_irq, .irq_set_type = gic_set_type, .irq_set_affinity = gic_set_affinity, + .irq_retrigger = gic_retrigger, .irq_get_irqchip_state = gic_irq_get_irqchip_state, .irq_set_irqchip_state = gic_irq_set_irqchip_state, .irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity, diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 00de05abd3c3..33ce025868d0 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -355,6 +355,11 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, } #endif +static int gic_retrigger(struct irq_data *data) +{ + return gic_irq_set_irqchip_state(data, IRQCHIP_STATE_PENDING, true); +} + static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) { u32 irqstat, irqnr; @@ -425,6 +430,7 @@ static const struct irq_chip gic_chip = { .irq_unmask = gic_unmask_irq, .irq_eoi = gic_eoi_irq, .irq_set_type = gic_set_type, + .irq_retrigger = gic_retrigger, .irq_get_irqchip_state = gic_irq_get_irqchip_state, .irq_set_irqchip_state = gic_irq_set_irqchip_state, .flags = IRQCHIP_SET_TYPE_MASKED | -- 2.27.0
| |