linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] irqchip/gic-v2, v3: Enforce ACK/EOI pairing on IRQ retrigger
@ 2020-07-30 17:03 Valentin Schneider
  2020-07-30 17:03 ` [PATCH v2 1/2] irqchip/gic-v2, v3: Implement irq_chip->irq_retrigger() Valentin Schneider
  2020-07-30 17:03 ` [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely Valentin Schneider
  0 siblings, 2 replies; 8+ messages in thread
From: Valentin Schneider @ 2020-07-30 17:03 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: Marc Zyngier, Thomas Gleixner, Lorenzo Pieralisi, Jason Cooper

Hi,

This is v2 of:

  https://lkml.kernel.org/r/20200710145642.28978-1-valentin.schneider@arm.com

the TL;DR being: retriggering of IRQs marked with IRQS_PENDING breaks the
ACK/EOI pairing the GIC spec requires, and this is an attempt to clean it
up; details are in the changelogs.

Testing
=======

I briefly tested this on my Juno r0 by hackishly configuring the RTC
interrupt as edge-sensitive (level interrupts don't get retriggered) and
triggering a wakeup interrupt via

  $ rtcwake -d /dev/rtc0 -s 1 -m mem

along with sprinkling a few printks to make sure things go down the right
way. Note that wakeup from RTC is currently broken on Juno and requires
this patch from Sudeep:

 4df2ef85f0ef ("rtc: pl031: fix set_alarm by adding back call to alarm_irq_enable")

The RTC on my eMAG can't be set up as a wakeup alarm, so I didn't get to
test the retriggering side of things on GICv3 - the rest does seem to still
behave correctly.

Revisions
=========

v1 -> v2
--------

o Trimmed the changelog
o Fixed gic_retrigger() return value (Marc)
o (new patch) Added IRQD_HANDLE_ENFORCE_IRQCTX to all GIC IRQs

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.c | 12 +++++++++++-
 drivers/irqchip/irq-gic.c    | 12 +++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

--
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/2] irqchip/gic-v2, v3: Implement irq_chip->irq_retrigger()
  2020-07-30 17:03 [PATCH v2 0/2] irqchip/gic-v2, v3: Enforce ACK/EOI pairing on IRQ retrigger Valentin Schneider
@ 2020-07-30 17:03 ` Valentin Schneider
  2020-07-30 17:03 ` [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely Valentin Schneider
  1 sibling, 0 replies; 8+ messages in thread
From: Valentin Schneider @ 2020-07-30 17:03 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: Marc Zyngier, Thomas Gleixner, Lorenzo Pieralisi, Jason Cooper

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>
---
 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..0fbcbf55ec48 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..e2b4cae88bce 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
  2020-07-30 17:03 [PATCH v2 0/2] irqchip/gic-v2, v3: Enforce ACK/EOI pairing on IRQ retrigger Valentin Schneider
  2020-07-30 17:03 ` [PATCH v2 1/2] irqchip/gic-v2, v3: Implement irq_chip->irq_retrigger() Valentin Schneider
@ 2020-07-30 17:03 ` Valentin Schneider
  2020-07-30 18:10   ` Marc Zyngier
  1 sibling, 1 reply; 8+ messages in thread
From: Valentin Schneider @ 2020-07-30 17:03 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: Marc Zyngier, Thomas Gleixner, Lorenzo Pieralisi, Jason Cooper

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>
---
 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 0fbcbf55ec48..1a8acf7cd8ac 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 e2b4cae88bce..a91ce1e73bd2 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -983,6 +983,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);
@@ -992,8 +993,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.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
  2020-07-30 17:03 ` [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely Valentin Schneider
@ 2020-07-30 18:10   ` Marc Zyngier
  2020-07-31  0:08     ` Valentin Schneider
  0 siblings, 1 reply; 8+ messages in thread
From: Marc Zyngier @ 2020-07-30 18:10 UTC (permalink / raw)
  To: Valentin Schneider
  Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
	linux-arm-kernel, Jason Cooper

Hi Valentin,

On 2020-07-30 18:03, Valentin Schneider wrote:
> 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>
> ---
>  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 0fbcbf55ec48..1a8acf7cd8ac 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 e2b4cae88bce..a91ce1e73bd2 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -983,6 +983,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);
> @@ -992,8 +993,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;
>  }

I'm OK with this in principle, but this requires additional changes
in the rest of the GIC universe. The ITS driver needs to provide its own
retrigger function for LPIs (queuing an INT command), and any of the
SPI generating widgets that can be stacked on top of a GIC (GICv3-MBI,
GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to gain
directly or indirectly a call to irq_chip_retrigger_hierarchy().

We can probably avoid changing the MSI widgets by teaching the MSI
code about the HW retrigger, but a number of other non-MSI drivers
will need some help...

I'll have a look tomorrow.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
  2020-07-30 18:10   ` Marc Zyngier
@ 2020-07-31  0:08     ` Valentin Schneider
  2020-07-31  8:08       ` Marc Zyngier
  0 siblings, 1 reply; 8+ messages in thread
From: Valentin Schneider @ 2020-07-31  0:08 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
	linux-arm-kernel, Jason Cooper


Hi Marc,

On 30/07/20 19:10, Marc Zyngier wrote:
[...]
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index e2b4cae88bce..a91ce1e73bd2 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -983,6 +983,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);
>> @@ -992,8 +993,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;
>>  }
>
> I'm OK with this in principle, but this requires additional changes
> in the rest of the GIC universe. The ITS driver needs to provide its own
> retrigger function for LPIs (queuing an INT command), and any of the
> SPI generating widgets that can be stacked on top of a GIC (GICv3-MBI,
> GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to gain
> directly or indirectly a call to irq_chip_retrigger_hierarchy().
>

Eep, yes indeed... I didn't see that can was full of worms, though even if
it only really matters for eoimode=0 I think it might still be worth it
(if only to respect the spec).

> We can probably avoid changing the MSI widgets by teaching the MSI
> code about the HW retrigger, but a number of other non-MSI drivers
> will need some help...
>
> I'll have a look tomorrow.
>

For LPIs AFAICT we could directly reuse its_irq_set_irqchip_state() - I see
the VPE side of things already has a HW retrigger callback.

For gicv2m, I *think* we'd want irq_chip_retrigger_hierarchy() on both MSI
domains (which IIUC you suggest might be doable by adding the retrigger as
a default MSI chip op).

I'm not very familiar with the rest of the fauna, so I'll have to do some
reading tomorrow as well; it's probably high time for me to actually read
up on LPIs & ITS while I'm at it...

> Thanks,
>
>          M.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
  2020-07-31  0:08     ` Valentin Schneider
@ 2020-07-31  8:08       ` Marc Zyngier
  2020-07-31 11:27         ` Marc Zyngier
  0 siblings, 1 reply; 8+ messages in thread
From: Marc Zyngier @ 2020-07-31  8:08 UTC (permalink / raw)
  To: Valentin Schneider
  Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
	linux-arm-kernel, Jason Cooper

Hi Valentin,

On 2020-07-31 01:08, Valentin Schneider wrote:
> Hi Marc,
> 
> On 30/07/20 19:10, Marc Zyngier wrote:
> [...]
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index e2b4cae88bce..a91ce1e73bd2 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -983,6 +983,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);
>>> @@ -992,8 +993,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;
>>>  }
>> 
>> I'm OK with this in principle, but this requires additional changes
>> in the rest of the GIC universe. The ITS driver needs to provide its 
>> own
>> retrigger function for LPIs (queuing an INT command), and any of the
>> SPI generating widgets that can be stacked on top of a GIC (GICv3-MBI,
>> GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to gain
>> directly or indirectly a call to irq_chip_retrigger_hierarchy().
>> 
> 
> Eep, yes indeed... I didn't see that can was full of worms, though even 
> if
> it only really matters for eoimode=0 I think it might still be worth it
> (if only to respect the spec).

Well, given that we are using EOImode=0 for all guests at the moment,
there is some value it getting it right! ;-)

> 
>> We can probably avoid changing the MSI widgets by teaching the MSI
>> code about the HW retrigger, but a number of other non-MSI drivers
>> will need some help...
>> 
>> I'll have a look tomorrow.
>> 
> 
> For LPIs AFAICT we could directly reuse its_irq_set_irqchip_state() - I 
> see
> the VPE side of things already has a HW retrigger callback.

Yes, that's the idea (in general, if you implement the PENDING side of
irq_set_irqchip_state(), retrigger comes for free).

> For gicv2m, I *think* we'd want irq_chip_retrigger_hierarchy() on both 
> MSI
> domains (which IIUC you suggest might be doable by adding the retrigger 
> as
> a default MSI chip op).

Yes, that was my idea.

> I'm not very familiar with the rest of the fauna, so I'll have to do 
> some
> reading tomorrow as well; it's probably high time for me to actually 
> read
> up on LPIs & ITS while I'm at it...

Look for anything that performs an interrupt allocation by calling
into the parent with a 3 cell (DT case) fwspec. There is a bunch
of them.

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
  2020-07-31  8:08       ` Marc Zyngier
@ 2020-07-31 11:27         ` Marc Zyngier
  2020-07-31 12:11           ` Valentin Schneider
  0 siblings, 1 reply; 8+ messages in thread
From: Marc Zyngier @ 2020-07-31 11:27 UTC (permalink / raw)
  To: Valentin Schneider
  Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
	linux-arm-kernel, Jason Cooper

On 2020-07-31 09:08, Marc Zyngier wrote:
> Hi Valentin,
> 
> On 2020-07-31 01:08, Valentin Schneider wrote:
>> Hi Marc,
>> 
>> On 30/07/20 19:10, Marc Zyngier wrote:
>> [...]
>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>>> index e2b4cae88bce..a91ce1e73bd2 100644
>>>> --- a/drivers/irqchip/irq-gic.c
>>>> +++ b/drivers/irqchip/irq-gic.c
>>>> @@ -983,6 +983,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);
>>>> @@ -992,8 +993,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;
>>>>  }
>>> 
>>> I'm OK with this in principle, but this requires additional changes
>>> in the rest of the GIC universe. The ITS driver needs to provide its 
>>> own
>>> retrigger function for LPIs (queuing an INT command), and any of the
>>> SPI generating widgets that can be stacked on top of a GIC 
>>> (GICv3-MBI,
>>> GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to 
>>> gain
>>> directly or indirectly a call to irq_chip_retrigger_hierarchy().
>>> 
>> 
>> Eep, yes indeed... I didn't see that can was full of worms, though 
>> even if
>> it only really matters for eoimode=0 I think it might still be worth 
>> it
>> (if only to respect the spec).
> 
> Well, given that we are using EOImode=0 for all guests at the moment,
> there is some value it getting it right! ;-)
> 
>> 
>>> We can probably avoid changing the MSI widgets by teaching the MSI
>>> code about the HW retrigger, but a number of other non-MSI drivers
>>> will need some help...
>>> 
>>> I'll have a look tomorrow.
>>> 
>> 
>> For LPIs AFAICT we could directly reuse its_irq_set_irqchip_state() - 
>> I see
>> the VPE side of things already has a HW retrigger callback.
> 
> Yes, that's the idea (in general, if you implement the PENDING side of
> irq_set_irqchip_state(), retrigger comes for free).
> 
>> For gicv2m, I *think* we'd want irq_chip_retrigger_hierarchy() on both 
>> MSI
>> domains (which IIUC you suggest might be doable by adding the 
>> retrigger as
>> a default MSI chip op).
> 
> Yes, that was my idea.
> 
>> I'm not very familiar with the rest of the fauna, so I'll have to do 
>> some
>> reading tomorrow as well; it's probably high time for me to actually 
>> read
>> up on LPIs & ITS while I'm at it...
> 
> Look for anything that performs an interrupt allocation by calling
> into the parent with a 3 cell (DT case) fwspec. There is a bunch
> of them.

For what it is worth, I have just pushed out a branch[1] containing some
of this rework as well as your patches.

The only tricky part is the GICv4.1 doorbell retriggering, which just
can't be re-injected. It shouldn't matter though. Same for vSGIs, they
never fire on the host.

Thanks,

         M.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gic-retrigger
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
  2020-07-31 11:27         ` Marc Zyngier
@ 2020-07-31 12:11           ` Valentin Schneider
  0 siblings, 0 replies; 8+ messages in thread
From: Valentin Schneider @ 2020-07-31 12:11 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
	linux-arm-kernel, Jason Cooper


On 31/07/20 12:27, Marc Zyngier wrote:
>> Look for anything that performs an interrupt allocation by calling
>> into the parent with a 3 cell (DT case) fwspec. There is a bunch
>> of them.
>
> For what it is worth, I have just pushed out a branch[1] containing some
> of this rework as well as your patches.
>

Brilliant, thanks for taking a shot at this! I'll try to look for stragglers.

> The only tricky part is the GICv4.1 doorbell retriggering, which just
> can't be re-injected. It shouldn't matter though. Same for vSGIs, they
> never fire on the host.
>
> Thanks,
>
>          M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gic-retrigger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-31 12:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-30 17:03 [PATCH v2 0/2] irqchip/gic-v2, v3: Enforce ACK/EOI pairing on IRQ retrigger Valentin Schneider
2020-07-30 17:03 ` [PATCH v2 1/2] irqchip/gic-v2, v3: Implement irq_chip->irq_retrigger() Valentin Schneider
2020-07-30 17:03 ` [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely Valentin Schneider
2020-07-30 18:10   ` Marc Zyngier
2020-07-31  0:08     ` Valentin Schneider
2020-07-31  8:08       ` Marc Zyngier
2020-07-31 11:27         ` Marc Zyngier
2020-07-31 12:11           ` Valentin Schneider

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).