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

[Marc Zyngier <maz@kernel.org>]

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