| Message ID | 20210629125010.458872-3-valentin.schneider@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/irq-gic: Optimize masking by leveraging EOImode=1 | expand |
On Tue, 29 Jun 2021 13:49:59 +0100, Valentin Schneider <valentin.schneider@arm.com> wrote: > > The newly-added IRQCHIP_AUTOMASKS_FLOW flag requires some additional > bookkeeping around chip->{irq_ack, irq_eoi}() calls. Define wrappers around > those chip callbacks to drive the IRQD_IRQ_FLOW_MASKED state of an IRQ when > the chip has the IRQCHIP_AUTOMASKS_FLOW flag. > > Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> > --- > kernel/irq/chip.c | 16 ++++++++++++++++ > kernel/irq/internals.h | 2 ++ > 2 files changed, 18 insertions(+) > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 21a21baa1366..793dbd8307b9 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -408,6 +408,22 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu) > cpumask_clear_cpu(cpu, desc->percpu_enabled); > } > > +void ack_irq(struct irq_desc *desc) > +{ > + desc->irq_data.chip->irq_ack(&desc->irq_data); > + > + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW) > + irq_state_set_flow_masked(desc); > +} > + > +void eoi_irq(struct irq_desc *desc) > +{ > + desc->irq_data.chip->irq_eoi(&desc->irq_data); > + > + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW) > + irq_state_clr_flow_masked(desc); I just realised that this has a good chance to result in a mess with KVM, and specially the way we let the vGIC deactivate an interrupt directly from the guest, without any SW intervention (the magic HW bit in the LRs). With this, interrupts routed to a guest (such as the timers) will always have the IRQD_IRQ_FLOW_MASKED flag set, which will never be cleared. I wonder whether this have a chance to interact badly with mask/unmask, or with the rest of the flow... Thanks, M.
On 12/08/21 08:49, Marc Zyngier wrote: > On Tue, 29 Jun 2021 13:49:59 +0100, > Valentin Schneider <valentin.schneider@arm.com> wrote: >> +void eoi_irq(struct irq_desc *desc) >> +{ >> + desc->irq_data.chip->irq_eoi(&desc->irq_data); >> + >> + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW) >> + irq_state_clr_flow_masked(desc); > > I just realised that this has a good chance to result in a mess with > KVM, and specially the way we let the vGIC deactivate an interrupt > directly from the guest, without any SW intervention (the magic HW bit > in the LRs). > I didn't think to consider those. It can't ever be simple, can it... > With this, interrupts routed to a guest (such as the timers) will > always have the IRQD_IRQ_FLOW_MASKED flag set, which will never be > cleared. > > I wonder whether this have a chance to interact badly with > mask/unmask, or with the rest of the flow... > Isn't it the other way around? That is, eoi_irq() will clear IRQD_IRQ_FLOW_MASKED regardless of what happens within chip->irq_eoi(), so we would end up with !IRQD_IRQ_FLOW_MASKED even if the (physical) IRQ remains Active (irqd_is_forwarded_to_vcpu() case). This does not entirely match reality (if the IRQ is still Active then it is still "flow-masked"), but AFAICT this doesn't impact our handling of forwarded IRQs: IRQD_IRQ_FLOW_MASKED is only really relevant from ack_irq() to eoi_irq(), and deactivation-from-the-guest (propagated via LR.HW=1) happens after that.
On Thu, 12 Aug 2021 14:36:11 +0100, Valentin Schneider <valentin.schneider@arm.com> wrote: > > On 12/08/21 08:49, Marc Zyngier wrote: > > On Tue, 29 Jun 2021 13:49:59 +0100, > > Valentin Schneider <valentin.schneider@arm.com> wrote: > >> +void eoi_irq(struct irq_desc *desc) > >> +{ > >> + desc->irq_data.chip->irq_eoi(&desc->irq_data); > >> + > >> + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW) > >> + irq_state_clr_flow_masked(desc); > > > > I just realised that this has a good chance to result in a mess with > > KVM, and specially the way we let the vGIC deactivate an interrupt > > directly from the guest, without any SW intervention (the magic HW bit > > in the LRs). > > > > I didn't think to consider those. It can't ever be simple, can it... > > > With this, interrupts routed to a guest (such as the timers) will > > always have the IRQD_IRQ_FLOW_MASKED flag set, which will never be > > cleared. > > > > I wonder whether this have a chance to interact badly with > > mask/unmask, or with the rest of the flow... > > > > Isn't it the other way around? That is, eoi_irq() will clear > IRQD_IRQ_FLOW_MASKED regardless of what happens within chip->irq_eoi(), > so we would end up with !IRQD_IRQ_FLOW_MASKED even if the (physical) IRQ > remains Active (irqd_is_forwarded_to_vcpu() case). Ah, I missed (again) that we always clear the flag, no matter what. > This does not entirely match reality (if the IRQ is still Active then it is > still "flow-masked"), but AFAICT this doesn't impact our handling of > forwarded IRQs: IRQD_IRQ_FLOW_MASKED is only really relevant from ack_irq() > to eoi_irq(), and deactivation-from-the-guest (propagated via LR.HW=1) > happens after that. Right. So we can have an active interrupt that is not flow-masked. That's counter-intuitive, but that's the GIC architecture for you... I'll take the series for a ride in -next. If anything breaks, we should know pretty soon. Thanks, M.
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 21a21baa1366..793dbd8307b9 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -408,6 +408,22 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu) cpumask_clear_cpu(cpu, desc->percpu_enabled); } +void ack_irq(struct irq_desc *desc) +{ + desc->irq_data.chip->irq_ack(&desc->irq_data); + + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW) + irq_state_set_flow_masked(desc); +} + +void eoi_irq(struct irq_desc *desc) +{ + desc->irq_data.chip->irq_eoi(&desc->irq_data); + + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW) + irq_state_clr_flow_masked(desc); +} + static inline void mask_ack_irq(struct irq_desc *desc) { if (desc->irq_data.chip->irq_mask_ack) { diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h index b6c1cceddec0..6d6a621dc74c 100644 --- a/kernel/irq/internals.h +++ b/kernel/irq/internals.h @@ -87,6 +87,8 @@ extern void irq_enable(struct irq_desc *desc); extern void irq_disable(struct irq_desc *desc); extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu); extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu); +extern void ack_irq(struct irq_desc *desc); +extern void eoi_irq(struct irq_desc *desc); extern void mask_irq(struct irq_desc *desc); extern void unmask_irq(struct irq_desc *desc); extern void unmask_threaded_irq(struct irq_desc *desc);
The newly-added IRQCHIP_AUTOMASKS_FLOW flag requires some additional bookkeeping around chip->{irq_ack, irq_eoi}() calls. Define wrappers around those chip callbacks to drive the IRQD_IRQ_FLOW_MASKED state of an IRQ when the chip has the IRQCHIP_AUTOMASKS_FLOW flag. Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> --- kernel/irq/chip.c | 16 ++++++++++++++++ kernel/irq/internals.h | 2 ++ 2 files changed, 18 insertions(+)