[PATCH v2 0/4] irqchip/gic: Generalize use of HW-based retriggering

[PATCH v2 0/4] irqchip/gic: Generalize use of HW-based retriggering

[Marc Zyngier]

Valentin recently pointed out that that relying on SW-based retrigger
with any of the GIC interrupt controllers is both inefficient and
slightly broken, as it messes the GIC's own state machine.

For this to work with the hierarchical irqchip model that the GIC
uses, we need check_irq_resend grow an understanding of hierarchies,
something it has been lacking for some time.

The 3 other patches simply implement the HW resend callbacks, and
finally prevent any SW resend for the GICs.

Unless anyone objects, I plan to take this into 5.10.

* From v1:
  - Dropped most of the patches in favour of a teaching the core
    kernel to use irq_chip_retriger_hierarchy()

Marc Zyngier (2):
  genirq: Walk the irq_data hierarchy when resending an interrupt
  irqchip/git-v3-its: Implement irq_retrigger callback for
    device-triggered LPIs

Valentin Schneider (2):
  irqchip/gic-v2, v3: Implement irq_chip->irq_retrigger()
  irqchip/gic-v2, v3: Prevent SW resends entirely

 drivers/irqchip/irq-gic-v3-its.c |  6 ++++++
 drivers/irqchip/irq-gic-v3.c     | 12 +++++++++++-
 drivers/irqchip/irq-gic.c        | 12 +++++++++++-
 kernel/irq/resend.c              | 15 +++++++++++++--
 4 files changed, 41 insertions(+), 4 deletions(-)

-- 
2.28.0

[PATCH v2 1/4] genirq: Walk the irq_data hierarchy when resending an interrupt

[Marc Zyngier]

On resending an interrupt, we only check the topmost irqchip for
a irq_retrigger callback. However, this callback could be implemented
at a lower level. Use irq_chip_retrigger_hierarchy() in this case.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 kernel/irq/resend.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c
index c48ce19a257f..8ccd32a0cc80 100644
--- a/kernel/irq/resend.c
+++ b/kernel/irq/resend.c
@@ -86,6 +86,18 @@ static int irq_sw_resend(struct irq_desc *desc)
 }
 #endif
 
+static int try_retrigger(struct irq_desc *desc)
+{
+	if (desc->irq_data.chip->irq_retrigger)
+		return desc->irq_data.chip->irq_retrigger(&desc->irq_data);
+
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+	return irq_chip_retrigger_hierarchy(&desc->irq_data);
+#else
+	return 0;
+#endif
+}
+
 /*
  * IRQ resend
  *
@@ -113,8 +125,7 @@ int check_irq_resend(struct irq_desc *desc, bool inject)
 
 	desc->istate &= ~IRQS_PENDING;
 
-	if (!desc->irq_data.chip->irq_retrigger ||
-	    !desc->irq_data.chip->irq_retrigger(&desc->irq_data))
+	if (!try_retrigger(desc))
 		err = irq_sw_resend(desc);
 
 	/* If the retrigger was successfull, mark it with the REPLAY bit */
-- 
2.28.0

Re: [PATCH v2 1/4] genirq: Walk the irq_data hierarchy when resending an interrupt

[Valentin Schneider]

Hi Marc,

On 03/09/20 19:32, Marc Zyngier wrote:
> On resending an interrupt, we only check the topmost irqchip for
> a irq_retrigger callback. However, this callback could be implemented
> at a lower level. Use irq_chip_retrigger_hierarchy() in this case.
>

Rookie wording question here; re-reading this I'm questioning which way is
up.

From an irq_data hierarchy PoV, the topmost chip (i.e. last ->parent)
should be the root irqchip. However, the irq_desc we get from irq_to_desc()
ought to hold the irq_data for the lowermost irqchip in that irq_data
hierarchy.

Is it that here by "topmost" you instead mean topmost of the irqchip stack
on top of the root (IOW furthest away from the root)?

> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  kernel/irq/resend.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c
> index c48ce19a257f..8ccd32a0cc80 100644
> --- a/kernel/irq/resend.c
> +++ b/kernel/irq/resend.c
> @@ -86,6 +86,18 @@ static int irq_sw_resend(struct irq_desc *desc)
>  }
>  #endif
>
> +static int try_retrigger(struct irq_desc *desc)
> +{
> +	if (desc->irq_data.chip->irq_retrigger)
> +		return desc->irq_data.chip->irq_retrigger(&desc->irq_data);
> +
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +	return irq_chip_retrigger_hierarchy(&desc->irq_data);
> +#else
> +	return 0;
> +#endif
> +}
> +
>  /*
>   * IRQ resend
>   *
> @@ -113,8 +125,7 @@ int check_irq_resend(struct irq_desc *desc, bool inject)
>
>       desc->istate &= ~IRQS_PENDING;
>
> -	if (!desc->irq_data.chip->irq_retrigger ||
> -	    !desc->irq_data.chip->irq_retrigger(&desc->irq_data))
> +	if (!try_retrigger(desc))
>               err = irq_sw_resend(desc);
>
>       /* If the retrigger was successfull, mark it with the REPLAY bit */

Re: [PATCH v2 1/4] genirq: Walk the irq_data hierarchy when resending an interrupt

[Marc Zyngier]

Hi Valentin,

On Fri, 04 Sep 2020 20:28:38 +0100,
Valentin Schneider <valentin.schneider@arm.com> wrote:
> 
> 
> Hi Marc,
> 
> On 03/09/20 19:32, Marc Zyngier wrote:
> > On resending an interrupt, we only check the topmost irqchip for
> > a irq_retrigger callback. However, this callback could be implemented
> > at a lower level. Use irq_chip_retrigger_hierarchy() in this case.
> >
> 
> Rookie wording question here; re-reading this I'm questioning which way is
> up.
> 
> From an irq_data hierarchy PoV, the topmost chip (i.e. last ->parent)
> should be the root irqchip. However, the irq_desc we get from irq_to_desc()
> ought to hold the irq_data for the lowermost irqchip in that irq_data
> hierarchy.
> 
> Is it that here by "topmost" you instead mean topmost of the irqchip stack
> on top of the root (IOW furthest away from the root)?

That's indeed what I mean, but I agree that the terminology is
confusing, and often used inconsistently (by me included).

<random>
Maybe considering the irqchip stack along a vertical axis is the wrong
thing to do, and that looking at it as a volume would be marginally
better?

How about innermost (close to the CPU) vs outermost (close to the
device)?
</random>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 1/4] genirq: Walk the irq_data hierarchy when resending an interrupt

[Valentin Schneider]

On 05/09/20 10:26, Marc Zyngier wrote:
> <random>
> Maybe considering the irqchip stack along a vertical axis is the wrong
> thing to do, and that looking at it as a volume would be marginally
> better?
>
> How about innermost (close to the CPU) vs outermost (close to the
> device)?
> </random>
>

I guess this is fairly subjective, but the inner/outer thing does click
with me.

I think the "issue" with the top / bottom wording is that existing data
structures (domain / irq_data hierarchy) bias my interpretation of it, but
it's upside down from the irq_chip stack representation. That's not the
case of inner / outer where all I can think of is the actual chip layout
(i.e. as an image of the distance from the CPUs).

Anyway, that's enough psychoanalysis from me, the patches look fine - I
also reran my quick rtcwake test on GICv2 (tests the WAKEUP_ARMED
path). Feel free to add

  Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>

to 1/4 & 3/4.

> Thanks,
>
>       M.

[PATCH v2 2/4] irqchip/gic-v2, v3: Implement irq_chip->irq_retrigger()

[Marc Zyngier]

From: Valentin Schneider <valentin.schneider@arm.com>

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).

When marking an IRQ with IRQS_PENDING, handle_fasteoi_irq() will bail out
and issue an irq_eoi(). Should the IRQ in question be re-enabled,
check_irq_resend() will trigger a SW resend, which will go through the flow
handler again and issue *another* irq_eoi() on the *same* IRQ
activation. This is something the GIC spec clearly describes as a bad idea:
any EOI must match a previous ACK.

Implement irq_chip.irq_retrigger() for the GIC chips by setting the GIC
pending bit of the relevant IRQ. After being called by check_irq_resend(),
this will eventually trigger a *new* interrupt which we will handle as usual.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20200730170321.31228-2-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 324f280ff606..b507bc7c5cda 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 a27ba2cc1dce..e92ee2b6d7a5 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -347,6 +347,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;
@@ -417,6 +422,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.28.0

[PATCH v2 3/4] irqchip/git-v3-its: Implement irq_retrigger callback for device-triggered LPIs

[Marc Zyngier]

It is pretty easy to provide a retrigger callback for the ITS,
as it we already have the required support in terms of
irq_set_irqchip_state().

Note that this only works for device-generated LPIs, and not
the GICv4 doorbells, which should never have to be retriggered
anyway.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 95f097448f97..2808545a963e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1720,6 +1720,11 @@ static int its_irq_set_irqchip_state(struct irq_data *d,
 	return 0;
 }
 
+static int its_irq_retrigger(struct irq_data *d)
+{
+	return !its_irq_set_irqchip_state(d, IRQCHIP_STATE_PENDING, true);
+}
+
 /*
  * Two favourable cases:
  *
@@ -1971,6 +1976,7 @@ static struct irq_chip its_irq_chip = {
 	.irq_set_affinity	= its_set_affinity,
 	.irq_compose_msi_msg	= its_irq_compose_msi_msg,
 	.irq_set_irqchip_state	= its_irq_set_irqchip_state,
+	.irq_retrigger		= its_irq_retrigger,
 	.irq_set_vcpu_affinity	= its_irq_set_vcpu_affinity,
 };
 
-- 
2.28.0

[PATCH v2 4/4] irqchip/gic-v2, v3: Prevent SW resends entirely

[Marc Zyngier]

From: Valentin Schneider <valentin.schneider@arm.com>

The GIC irqchips can now use a HW resend when a retrigger is invoked by
check_irq_resend(). However, should the HW resend fail, check_irq_resend()
will still attempt to trigger a SW resend, which is still a bad idea for
the GICs.

Prevent this from happening by setting IRQD_HANDLE_ENFORCE_IRQCTX on all
GIC IRQs. Technically per-cpu IRQs do not need this, as their flow handlers
never set IRQS_PENDING, but this aligns all IRQs wrt context enforcement:
this also forces all GIC IRQ handling to happen in IRQ context (as defined
by in_irq()).

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20200730170321.31228-3-valentin.schneider@arm.com
---
 drivers/irqchip/irq-gic-v3.c | 5 ++++-
 drivers/irqchip/irq-gic.c    | 6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index b507bc7c5cda..4e9387aafed8 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1279,6 +1279,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 			      irq_hw_number_t hw)
 {
 	struct irq_chip *chip = &gic_chip;
+	struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
 
 	if (static_branch_likely(&supports_deactivate_key))
 		chip = &gic_eoimode1_chip;
@@ -1296,7 +1297,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 		irq_domain_set_info(d, irq, hw, chip, d->host_data,
 				    handle_fasteoi_irq, NULL, NULL);
 		irq_set_probe(irq);
-		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
+		irqd_set_single_target(irqd);
 		break;
 
 	case LPI_RANGE:
@@ -1310,6 +1311,8 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 		return -EPERM;
 	}
 
+	/* Prevents SW retriggers which mess up the ACK/EOI ordering */
+	irqd_set_handle_enforce_irqctx(irqd);
 	return 0;
 }
 
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index e92ee2b6d7a5..b59bcef69bf3 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -975,6 +975,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 				irq_hw_number_t hw)
 {
 	struct gic_chip_data *gic = d->host_data;
+	struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
 
 	if (hw < 32) {
 		irq_set_percpu_devid(irq);
@@ -984,8 +985,11 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 		irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
 				    handle_fasteoi_irq, NULL, NULL);
 		irq_set_probe(irq);
-		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
+		irqd_set_single_target(irqd);
 	}
+
+	/* Prevents SW retriggers which mess up the ACK/EOI ordering */
+	irqd_set_handle_enforce_irqctx(irqd);
 	return 0;
 }
 
-- 
2.28.0