| Message ID | 20210525173255.620606-10-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, 25 May 2021 18:32:54 +0100, Valentin Schneider <valentin.schneider@arm.com> wrote: > > Now that the proper infrastructure is in place, convert the irq-gic chip to > use handle_strict_flow_irq() along with IRQCHIP_AUTOMASKS_FLOW. > > For EOImode=1, the Priority Drop is moved from gic_handle_irq() into > chip->irq_ack(). This effectively pushes the EOI write down into > ->handle_irq(), but doesn't change its ordering wrt the irqaction > handling. > > The EOImode=1 irqchip also gains IRQCHIP_EOI_THREADED, which allows the > ->irq_eoi() call to be deferred to the tail of ONESHOT IRQ threads. This > means a threaded ONESHOT IRQ can now be handled entirely without a single > chip->irq_mask() call. > > EOImode=0 handling remains unchanged. > > Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> > --- > drivers/irqchip/irq-gic.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index b1d9c22caf2e..4919478c3e41 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -344,8 +344,6 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) > if (unlikely(irqnr >= 1020)) > break; > > - if (static_branch_likely(&supports_deactivate_key)) > - writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI); > isb(); > > /* > @@ -1012,7 +1010,9 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, > break; > default: > irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data, > - handle_fasteoi_irq, NULL, NULL); > + static_branch_likely(&supports_deactivate_key) ? > + handle_strict_flow_irq : handle_fasteoi_irq, > + NULL, NULL); > irq_set_probe(irq); > irqd_set_single_target(irqd); > break; > @@ -1116,8 +1116,16 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev, > > if (use_eoimode1) { > gic->chip.irq_mask = gic_eoimode1_mask_irq; > + gic->chip.irq_ack = gic_eoi_irq; > gic->chip.irq_eoi = gic_eoimode1_eoi_irq; > gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity; > + > + /* > + * eoimode0 shouldn't expose FLOW_MASK because the priority > + * drop is undissociable from the deactivation, and we do need > + * the priority drop to happen within the flow handler. > + */ > + gic->chip.flags |= IRQCHIP_AUTOMASKS_FLOW | IRQCHIP_EOI_THREADED; > } > > if (gic == &gic_data[0]) { How about GICv2M, GICv3-MBI, and the collection of widget that build a domain on top of a GIC domain? I'm worried that they now all need updating one way or another... M.
On 27/05/21 13:21, Marc Zyngier wrote: > On Tue, 25 May 2021 18:32:54 +0100, > Valentin Schneider <valentin.schneider@arm.com> wrote: >> @@ -1116,8 +1116,16 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev, >> >> if (use_eoimode1) { >> gic->chip.irq_mask = gic_eoimode1_mask_irq; >> + gic->chip.irq_ack = gic_eoi_irq; >> gic->chip.irq_eoi = gic_eoimode1_eoi_irq; >> gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity; >> + >> + /* >> + * eoimode0 shouldn't expose FLOW_MASK because the priority >> + * drop is undissociable from the deactivation, and we do need >> + * the priority drop to happen within the flow handler. >> + */ >> + gic->chip.flags |= IRQCHIP_AUTOMASKS_FLOW | IRQCHIP_EOI_THREADED; >> } >> >> if (gic == &gic_data[0]) { > > How about GICv2M, GICv3-MBI, and the collection of widget that build a > domain on top of a GIC domain? I'm worried that they now all need > updating one way or another... > Hmph, that's a good point. It's been a while since I've last stared at the v2m, I'll go try to page that back in. > M. > > -- > Without deviation from the norm, progress is not possible.
On 01/06/21 11:25, Valentin Schneider wrote: > On 27/05/21 13:21, Marc Zyngier wrote: >> On Tue, 25 May 2021 18:32:54 +0100, >> Valentin Schneider <valentin.schneider@arm.com> wrote: >>> @@ -1116,8 +1116,16 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev, >>> >>> if (use_eoimode1) { >>> gic->chip.irq_mask = gic_eoimode1_mask_irq; >>> + gic->chip.irq_ack = gic_eoi_irq; >>> gic->chip.irq_eoi = gic_eoimode1_eoi_irq; >>> gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity; >>> + >>> + /* >>> + * eoimode0 shouldn't expose FLOW_MASK because the priority >>> + * drop is undissociable from the deactivation, and we do need >>> + * the priority drop to happen within the flow handler. >>> + */ >>> + gic->chip.flags |= IRQCHIP_AUTOMASKS_FLOW | IRQCHIP_EOI_THREADED; >>> } >>> >>> if (gic == &gic_data[0]) { >> >> How about GICv2M, GICv3-MBI, and the collection of widget that build a >> domain on top of a GIC domain? I'm worried that they now all need >> updating one way or another... >> > > Hmph, that's a good point. It's been a while since I've last stared at the > v2m, I'll go try to page that back in. > It's taken me a while to get back to this, apologies. Here's where I'm at: At the very least these need the +.irq_ack() treatment, same as the ITS chips. We can get around this by giving msi_domain_update_chip_ops() some invoke-first-non-NULL default callbacks, as you've suggested in: http://lore.kernel.org/r/87y2c0s748.wl-maz@kernel.org Now, looking at this made me think about which irq_chip flags are being used where, and, well... PCI-MSI IRQs are deemed 'oneshot safe', but platform-MSI ones aren't. So for instance, if a GICv2M pMSI IRQ gets force-threaded, we'll make it IRQS_ONESHOT. However, this is still just a glorified SPI as all mask, ack and eoi operations will be the root chip's, so we should be able to apply the eoimode=1 automask trickery to it. This won't happen with the current patches, since the ->chip we'll seeing in handle_strict_flow_irq() will be gicv2m_pmsi_irq_chip. We *could* give that one the required flags, but what actually matters for the automask thing are the flags of first chip in the hiearachy that has "proper" ack+eoi callbacks. I don't see a nice way of handling this right now... >> M. >> >> -- >> Without deviation from the norm, progress is not possible.
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index b1d9c22caf2e..4919478c3e41 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -344,8 +344,6 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) if (unlikely(irqnr >= 1020)) break; - if (static_branch_likely(&supports_deactivate_key)) - writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI); isb(); /* @@ -1012,7 +1010,9 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, break; default: irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data, - handle_fasteoi_irq, NULL, NULL); + static_branch_likely(&supports_deactivate_key) ? + handle_strict_flow_irq : handle_fasteoi_irq, + NULL, NULL); irq_set_probe(irq); irqd_set_single_target(irqd); break; @@ -1116,8 +1116,16 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev, if (use_eoimode1) { gic->chip.irq_mask = gic_eoimode1_mask_irq; + gic->chip.irq_ack = gic_eoi_irq; gic->chip.irq_eoi = gic_eoimode1_eoi_irq; gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity; + + /* + * eoimode0 shouldn't expose FLOW_MASK because the priority + * drop is undissociable from the deactivation, and we do need + * the priority drop to happen within the flow handler. + */ + gic->chip.flags |= IRQCHIP_AUTOMASKS_FLOW | IRQCHIP_EOI_THREADED; } if (gic == &gic_data[0]) {
Now that the proper infrastructure is in place, convert the irq-gic chip to use handle_strict_flow_irq() along with IRQCHIP_AUTOMASKS_FLOW. For EOImode=1, the Priority Drop is moved from gic_handle_irq() into chip->irq_ack(). This effectively pushes the EOI write down into ->handle_irq(), but doesn't change its ordering wrt the irqaction handling. The EOImode=1 irqchip also gains IRQCHIP_EOI_THREADED, which allows the ->irq_eoi() call to be deferred to the tail of ONESHOT IRQ threads. This means a threaded ONESHOT IRQ can now be handled entirely without a single chip->irq_mask() call. EOImode=0 handling remains unchanged. Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> --- drivers/irqchip/irq-gic.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)