| Message ID | 20200624195811.435857-7-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/arm64: Turning IPIs into normal interrupts | expand |
On 24/06/20 20:58, Marc Zyngier wrote: > Change the way we deal with GICv3 SGIs by turning them into proper > IRQs, and calling into the arch code to register the interrupt range > instead of a callback. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > drivers/irqchip/irq-gic-v3.c | 81 +++++++++++++++++++----------------- > 1 file changed, 43 insertions(+), 38 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index 19b294ed48ba..d275e9b9533d 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -36,6 +36,8 @@ > #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0) > #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1) > > +#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1) > + Nit: this piqued my interest but ended up being just a define shuffle; As a member of the git speleologists' guild, I'd be overjoyed with having a small notion of that in the changelog. > struct redist_region { > void __iomem *redist_base; > phys_addr_t phys_base; > @@ -657,38 +659,14 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs > if ((irqnr >= 1020 && irqnr <= 1023)) > return; > > - /* Treat anything but SGIs in a uniform way */ > - if (likely(irqnr > 15)) { > - int err; > - > - if (static_branch_likely(&supports_deactivate_key)) > - gic_write_eoir(irqnr); > - else > - isb(); > - > - err = handle_domain_irq(gic_data.domain, irqnr, regs); > - if (err) { > - WARN_ONCE(true, "Unexpected interrupt received!\n"); > - gic_deactivate_unhandled(irqnr); > - } > - return; > - } > - if (irqnr < 16) { > + if (static_branch_likely(&supports_deactivate_key)) > gic_write_eoir(irqnr); > - if (static_branch_likely(&supports_deactivate_key)) > - gic_write_dir(irqnr); > -#ifdef CONFIG_SMP > - /* > - * Unlike GICv2, we don't need an smp_rmb() here. > - * The control dependency from gic_read_iar to > - * the ISB in gic_write_eoir is enough to ensure > - * that any shared data read by handle_IPI will > - * be read after the ACK. > - */ Isn't that still relevant? Also, while staring at this it dawned on me that IPI's don't need the eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll get a gic_eoi_irq() just before calling into the irqaction. Dunno how much we care about it. > - handle_IPI(irqnr, regs); > -#else > - WARN_ONCE(true, "Unexpected SGI received!\n"); > -#endif > + else > + isb(); > + > + if (handle_domain_irq(gic_data.domain, irqnr, regs)) { > + WARN_ONCE(true, "Unexpected interrupt received!\n"); > + gic_deactivate_unhandled(irqnr); > } > } >
On 2020-06-25 19:25, Valentin Schneider wrote: > On 24/06/20 20:58, Marc Zyngier wrote: >> Change the way we deal with GICv3 SGIs by turning them into proper >> IRQs, and calling into the arch code to register the interrupt range >> instead of a callback. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> drivers/irqchip/irq-gic-v3.c | 81 >> +++++++++++++++++++----------------- >> 1 file changed, 43 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/irqchip/irq-gic-v3.c >> b/drivers/irqchip/irq-gic-v3.c >> index 19b294ed48ba..d275e9b9533d 100644 >> --- a/drivers/irqchip/irq-gic-v3.c >> +++ b/drivers/irqchip/irq-gic-v3.c >> @@ -36,6 +36,8 @@ >> #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0) >> #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1) >> >> +#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1) >> + > > Nit: this piqued my interest but ended up being just a define shuffle; > As a > member of the git speleologists' guild, I'd be overjoyed with having a > small notion of that in the changelog. Fair enough. > >> struct redist_region { >> void __iomem *redist_base; >> phys_addr_t phys_base; >> @@ -657,38 +659,14 @@ static asmlinkage void __exception_irq_entry >> gic_handle_irq(struct pt_regs *regs >> if ((irqnr >= 1020 && irqnr <= 1023)) >> return; >> >> - /* Treat anything but SGIs in a uniform way */ >> - if (likely(irqnr > 15)) { >> - int err; >> - >> - if (static_branch_likely(&supports_deactivate_key)) >> - gic_write_eoir(irqnr); >> - else >> - isb(); >> - >> - err = handle_domain_irq(gic_data.domain, irqnr, regs); >> - if (err) { >> - WARN_ONCE(true, "Unexpected interrupt received!\n"); >> - gic_deactivate_unhandled(irqnr); >> - } >> - return; >> - } >> - if (irqnr < 16) { >> + if (static_branch_likely(&supports_deactivate_key)) >> gic_write_eoir(irqnr); >> - if (static_branch_likely(&supports_deactivate_key)) >> - gic_write_dir(irqnr); >> -#ifdef CONFIG_SMP >> - /* >> - * Unlike GICv2, we don't need an smp_rmb() here. >> - * The control dependency from gic_read_iar to >> - * the ISB in gic_write_eoir is enough to ensure >> - * that any shared data read by handle_IPI will >> - * be read after the ACK. >> - */ > > Isn't that still relevant? It is. It is just that there is no really good place to put it. I may end-up just leaving it where it is. > Also, while staring at this it dawned on me that IPI's don't need the > eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll > get a > gic_eoi_irq() just before calling into the irqaction. Dunno how much we > care about it. That's interesting. This ISB is a leftover from the loop we had before the pseudo-NMI code, where we had to make sure the write to EOIR was ordered with the read from IAR. Given that we have an exception return right after the interrupt handling, I *think* we could get rid of it (but that would need mode checking on broken systems such as TX1...). I don't think this is specific to IPIs though. Thanks, M.
On 30/06/20 11:15, Marc Zyngier wrote: > On 2020-06-25 19:25, Valentin Schneider wrote: >> Also, while staring at this it dawned on me that IPI's don't need the >> eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll >> get a >> gic_eoi_irq() just before calling into the irqaction. Dunno how much we >> care about it. > > That's interesting. This ISB is a leftover from the loop we had before > the pseudo-NMI code, where we had to make sure the write to EOIR was > ordered with the read from IAR. > > Given that we have an exception return right after the interrupt > handling, I *think* we could get rid of it (but that would need > mode checking on broken systems such as TX1...). I don't think > this is specific to IPIs though. > If I got this one right: 39a06b67c2c1 ("irqchip/gic: Ensure we have an ISB between ack and ->handle_irq") you're describing case 2, which is indeed gone on gic-v3. However IIUC we also want an ISB between poking IAR and calling into the irqaction (case 1) - we get just that with IPIs due to the early gic_eoi_irq(), but we don't for the other flows. > Thanks, > > M.
On 2020-07-02 14:23, Valentin Schneider wrote: > On 30/06/20 11:15, Marc Zyngier wrote: >> On 2020-06-25 19:25, Valentin Schneider wrote: >>> Also, while staring at this it dawned on me that IPI's don't need the >>> eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll >>> get a >>> gic_eoi_irq() just before calling into the irqaction. Dunno how much >>> we >>> care about it. >> >> That's interesting. This ISB is a leftover from the loop we had before >> the pseudo-NMI code, where we had to make sure the write to EOIR was >> ordered with the read from IAR. >> >> Given that we have an exception return right after the interrupt >> handling, I *think* we could get rid of it (but that would need >> mode checking on broken systems such as TX1...). I don't think >> this is specific to IPIs though. >> > > If I got this one right: > > 39a06b67c2c1 ("irqchip/gic: Ensure we have an ISB between ack and > ->handle_irq") > > you're describing case 2, which is indeed gone on gic-v3. However IIUC > we > also want an ISB between poking IAR and calling into the irqaction > (case 1) > - we get just that with IPIs due to the early gic_eoi_irq(), but we > don't > for the other flows. You just made me realise something amazing: I've started to forget about all this crap. Which is wonderful! ;-) More seriously, you are absolutely right. If we wanted to address this, we'd probably have to give IPIs their own irqchip so that they get their own eoi callback. Not sure that's worth it. M.
On 02/07/20 14:48, Marc Zyngier wrote: > On 2020-07-02 14:23, Valentin Schneider wrote: >> On 30/06/20 11:15, Marc Zyngier wrote: >>> On 2020-06-25 19:25, Valentin Schneider wrote: >>>> Also, while staring at this it dawned on me that IPI's don't need the >>>> eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll >>>> get a >>>> gic_eoi_irq() just before calling into the irqaction. Dunno how much >>>> we >>>> care about it. >>> >>> That's interesting. This ISB is a leftover from the loop we had before >>> the pseudo-NMI code, where we had to make sure the write to EOIR was >>> ordered with the read from IAR. >>> >>> Given that we have an exception return right after the interrupt >>> handling, I *think* we could get rid of it (but that would need >>> mode checking on broken systems such as TX1...). I don't think >>> this is specific to IPIs though. >>> >> >> If I got this one right: >> >> 39a06b67c2c1 ("irqchip/gic: Ensure we have an ISB between ack and >> ->handle_irq") >> >> you're describing case 2, which is indeed gone on gic-v3. However IIUC >> we >> also want an ISB between poking IAR and calling into the irqaction >> (case 1) >> - we get just that with IPIs due to the early gic_eoi_irq(), but we >> don't >> for the other flows. > > You just made me realise something amazing: I've started to forget about > all this crap. Which is wonderful! ;-) > :) > More seriously, you are absolutely right. If we wanted to address this, > we'd probably have to give IPIs their own irqchip so that they get their > own eoi callback. Not sure that's worth it. > I was initially thinking of something like if (static_branch_likely(&supports_deactivate_key)) gic_write_eoir(irqnr); else if (__get_intid_range(irqnr) != SGI_RANGE) isb(); which is not particularly pretty, so maybe we should just slap this to the isb(): /* Superfluous for SGIs, but who cares */ > M.
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 19b294ed48ba..d275e9b9533d 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -36,6 +36,8 @@ #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0) #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1) +#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1) + struct redist_region { void __iomem *redist_base; phys_addr_t phys_base; @@ -657,38 +659,14 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs if ((irqnr >= 1020 && irqnr <= 1023)) return; - /* Treat anything but SGIs in a uniform way */ - if (likely(irqnr > 15)) { - int err; - - if (static_branch_likely(&supports_deactivate_key)) - gic_write_eoir(irqnr); - else - isb(); - - err = handle_domain_irq(gic_data.domain, irqnr, regs); - if (err) { - WARN_ONCE(true, "Unexpected interrupt received!\n"); - gic_deactivate_unhandled(irqnr); - } - return; - } - if (irqnr < 16) { + if (static_branch_likely(&supports_deactivate_key)) gic_write_eoir(irqnr); - if (static_branch_likely(&supports_deactivate_key)) - gic_write_dir(irqnr); -#ifdef CONFIG_SMP - /* - * Unlike GICv2, we don't need an smp_rmb() here. - * The control dependency from gic_read_iar to - * the ISB in gic_write_eoir is enough to ensure - * that any shared data read by handle_IPI will - * be read after the ACK. - */ - handle_IPI(irqnr, regs); -#else - WARN_ONCE(true, "Unexpected SGI received!\n"); -#endif + else + isb(); + + if (handle_domain_irq(gic_data.domain, irqnr, regs)) { + WARN_ONCE(true, "Unexpected interrupt received!\n"); + gic_deactivate_unhandled(irqnr); } } @@ -1136,11 +1114,11 @@ static void gic_send_sgi(u64 cluster_id, u16 tlist, unsigned int irq) gic_write_sgi1r(val); } -static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) +static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask) { int cpu; - if (WARN_ON(irq >= 16)) + if (WARN_ON(d->hwirq >= 16)) return; /* @@ -1154,7 +1132,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) u16 tlist; tlist = gic_compute_target_list(&cpu, mask, cluster_id); - gic_send_sgi(cluster_id, tlist, irq); + gic_send_sgi(cluster_id, tlist, d->hwirq); } /* Force the above writes to ICC_SGI1R_EL1 to be executed */ @@ -1163,10 +1141,24 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) static void __init gic_smp_init(void) { - set_smp_cross_call(gic_raise_softirq); + struct irq_fwspec sgi_fwspec = { + .fwnode = gic_data.fwnode, + .param_count = 1, + }; + int base_sgi; + cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING, "irqchip/arm/gicv3:starting", gic_starting_cpu, NULL); + + /* Register all 8 non-secure SGIs */ + base_sgi = __irq_domain_alloc_irqs(gic_data.domain, -1, 8, + NUMA_NO_NODE, &sgi_fwspec, + false, NULL); + if (WARN_ON(base_sgi <= 0)) + return; + + set_smp_ipi_range(base_sgi, 8); } static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, @@ -1215,6 +1207,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, } #else #define gic_set_affinity NULL +#define gic_ipi_send_mask NULL #define gic_smp_init() do { } while(0) #endif @@ -1257,6 +1250,7 @@ static struct irq_chip gic_chip = { .irq_set_irqchip_state = gic_irq_set_irqchip_state, .irq_nmi_setup = gic_irq_nmi_setup, .irq_nmi_teardown = gic_irq_nmi_teardown, + .ipi_send_mask = gic_ipi_send_mask, .flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND, @@ -1274,6 +1268,7 @@ static struct irq_chip gic_eoimode1_chip = { .irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity, .irq_nmi_setup = gic_irq_nmi_setup, .irq_nmi_teardown = gic_irq_nmi_teardown, + .ipi_send_mask = gic_ipi_send_mask, .flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND, @@ -1289,6 +1284,12 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, switch (__get_intid_range(hw)) { case SGI_RANGE: + irq_set_percpu_devid(irq); + irq_domain_set_info(d, irq, hw, chip, d->host_data, + handle_percpu_devid_fasteoi_ipi, + NULL, NULL); + break; + case PPI_RANGE: case EPPI_RANGE: irq_set_percpu_devid(irq); @@ -1318,13 +1319,17 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, return 0; } -#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1) - static int gic_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type) { + if (fwspec->param_count == 1 && fwspec->param[0] < 16) { + *hwirq = fwspec->param[0]; + *type = IRQ_TYPE_EDGE_RISING; + return 0; + } + if (is_of_node(fwspec->fwnode)) { if (fwspec->param_count < 3) return -EINVAL; @@ -1656,9 +1661,9 @@ static int __init gic_init_bases(void __iomem *dist_base, gic_update_rdist_properties(); - gic_smp_init(); gic_dist_init(); gic_cpu_init(); + gic_smp_init(); gic_cpu_pm_init(); if (gic_dist_supports_lpis()) {
Change the way we deal with GICv3 SGIs by turning them into proper IRQs, and calling into the arch code to register the interrupt range instead of a callback. Signed-off-by: Marc Zyngier <maz@kernel.org> --- drivers/irqchip/irq-gic-v3.c | 81 +++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 38 deletions(-)