Re: [RFC PATCH v2 45/45] irqchip/gic-v5: Add NMI support for IPIs

[Vladimir Murzin <vladimir.murzin@arm.com>]

On 8/7/26 11:17, Marc Zyngier wrote:
> On Mon, 27 Jul 2026 17:34:53 +0100,
> Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>> IPIs are implemented as a logical domain on top of the LPI domain.
>> Therefore, when an IPI is configured as an NMI, update the priority in
>> the parent LPI domain during setup and teardown.
>>
>> Permit irq_supports_nmi() to accept IRQs managed by an NMI-capable
>> parent domain.
>>
>> Finally, guard handle_irq_event_percpu() against calling
>> add_interrupt_randomness() from NMI context.
>>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> ---
>>  drivers/irqchip/irq-gic-v5.c | 23 ++++++++++++++++++++++-
>>  kernel/irq/handle.c          |  3 ++-
>>  kernel/irq/manage.c          | 11 ++++++++---
>>  3 files changed, 32 insertions(+), 5 deletions(-)
>>
>> The patch should definitely be split. The only reason I'm keeping it
>> as a single patch for now is that I'm not yet sure these changes are
>> moving in the right direction...
>>
>>
>> diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
>> index 05b957ffc0b8..787cb8da4e1a 100644
>> --- a/drivers/irqchip/irq-gic-v5.c
>> +++ b/drivers/irqchip/irq-gic-v5.c
>> @@ -700,6 +700,24 @@ static void gicv5_spi_irq_nmi_teardown(struct irq_data *d)
>>  	irq_to_desc(d->irq)->handle_irq = handle_fasteoi_irq;
>>  }
>>  
>> +static int gicv5_ipi_irq_nmi_setup(struct irq_data *d)
>> +{
>> +	if (WARN_ON(!d->parent_data))
>> +		return -EINVAL;
> How can this happen? Shouldn't that be impossible by construction?
> 

Cannot happen, it is just me getting too much addicted to all sorts of checks :) 

>> +
>> +	d = d->parent_data;
>> +
>> +	return gicv5_hwirq_irq_nmi_setup(d->hwirq, GICV5_HWIRQ_TYPE_LPI);
>> +}
>> +
>> +static void gicv5_ipi_irq_nmi_teardown(struct irq_data *d)
>> +{
>> +	if (WARN_ON(!d->parent_data))
>> +		return;
>> +
>> +	d = d->parent_data;
>> +
>> +	gicv5_hwirq_irq_nmi_teardown(d->hwirq, GICV5_HWIRQ_TYPE_LPI);
>>  }
>>  
>>  static struct irq_chip gicv5_ppi_irq_chip = {
>> @@ -749,7 +767,7 @@ static struct irq_chip gicv5_lpi_irq_chip = {
>>  				  IRQCHIP_MASK_ON_SUSPEND,
>>  };
>>  
>> -static const struct irq_chip gicv5_ipi_irq_chip = {
>> +static struct irq_chip gicv5_ipi_irq_chip = {
> If we can't have it const, can we have it as __ro_after_init?
> 

Yes, it was rises by Sashiko as well, already applied __ro_after_init locally

>>  	.name			= "GICv5-IPI",
>>  	.irq_mask		= irq_chip_mask_parent,
>>  	.irq_unmask		= irq_chip_unmask_parent,
>> @@ -757,6 +775,8 @@ static const struct irq_chip gicv5_ipi_irq_chip = {
>>  	.irq_set_affinity	= irq_chip_set_affinity_parent,
>>  	.irq_get_irqchip_state	= irq_chip_get_parent_state,
>>  	.irq_set_irqchip_state	= irq_chip_set_parent_state,
>> +	.irq_nmi_setup		= gicv5_ipi_irq_nmi_setup,
>> +	.irq_nmi_teardown	= gicv5_ipi_irq_nmi_teardown,
>>  	.ipi_send_single	= gicv5_ipi_send_single,
>>  	.flags			= IRQCHIP_SKIP_SET_WAKE	  |
>>  				  IRQCHIP_MASK_ON_SUSPEND,
>> @@ -1213,6 +1233,7 @@ static void gicv5_enable_nmi_support(void)
>>  	gicv5_ppi_irq_chip.flags |= IRQCHIP_SUPPORTS_NMI;
>>  	gicv5_spi_irq_chip.flags |= IRQCHIP_SUPPORTS_NMI;
>>  	gicv5_lpi_irq_chip.flags |= IRQCHIP_SUPPORTS_NMI;
>> +	gicv5_ipi_irq_chip.flags |= IRQCHIP_SUPPORTS_NMI;
>>  }
>>  
>>  static void __init gicv5_smp_init(void)
>> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
>> index b7d52821837b..114fd63b0210 100644
>> --- a/kernel/irq/handle.c
>> +++ b/kernel/irq/handle.c
>> @@ -245,7 +245,8 @@ irqreturn_t handle_irq_event_percpu(struct irq_desc *desc)
>>  
>>  	retval = __handle_irq_event_percpu(desc);
>>  
>> -	add_interrupt_randomness(desc->irq_data.irq);
>> +	if (!in_nmi())
>> +		add_interrupt_randomness(desc->irq_data.irq);
> This also needs to be a separate patch.
> 

Ack!

>>  
>>  	if (!irq_settings_no_debug(desc))
>>  		note_interrupt(desc, retval);
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 7eb07e3bdb4c..0091bb5f7662 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -1370,9 +1370,14 @@ static bool irq_supports_nmi(struct irq_desc *desc)
>>  	struct irq_data *d = irq_desc_get_irq_data(desc);
>>  
>>  #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> -	/* Only IRQs directly managed by the root irqchip can be set as NMI */
>> -	if (d->parent_data)
>> -		return false;
>> +	struct irq_data *data;
>> +	/*
>> +	 * Only IRQs directly managed by the root irqchip can be set
>> +	 * as NMI unless we inherit NMI from parent
>> +	 */
>> +	for (data = d->parent_data; data; data = data->parent_data)
>> +		if (!(data->chip && data->chip->flags & IRQCHIP_SUPPORTS_NMI))
>> +			return false;
> Maybe we should just trust the local irqchip to do the right
> thing. After all, the core code shouldn't be in the business of
> sanitising broken irqchip drivers advertising random crap.
> 

That is indeed bit I was not really sure about, if we decide that we
can trust local irqchip then I assume we just drop the check?

>>  #endif
>>  	/* Don't support NMIs for chips behind a slow bus */
>>  	if (d->chip->irq_bus_lock || d->chip->irq_bus_sync_unlock)
> Thanks,
> 

Thanks!

Vladimir

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