Re: [PATCH 17/17] irqchip/gic-v3: Implement FEAT_GICv3_NMI support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing mapping for GICD_INMIR to GICD_INMIRnE in convert_offset_index() for ESPIs
- [Low] Comment typo contradicts the implementation regarding expected interrupt state
--

commit d7c99cd0a83704d2374d8fa536ff561f0b757f91
Author: Lorenzo Pieralisi <lpieralisi@kernel.org>

irqchip/gic-v3: Implement FEAT_GICv3_NMI support

The FEAT_GICv3_NMI GIC feature coupled with the CPU FEAT_NMI enables
handling NMI interrupts in HW on aarch64, by adding a superpriority
interrupt to the existing GIC priority scheme.

Link: https://lore.kernel.org/r/20221112151708.175147-15-broonie@kernel.org

> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 111cd882c7a95..6e48dd9990219 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c

[ ... ]

> @@ -467,6 +482,42 @@ static int gic_peek_irq(struct irq_data *d, u32 offset)
>  	return !!(readl_relaxed(base + offset + (index / 32) * 4) & mask);
>  }
>  
> +static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> +
> +static void gic_irq_configure_nmi(struct irq_data *d, bool enable)
> +{
> +	void __iomem *base, *addr;
> +	u32 offset, index, mask, val;
> +
> +	offset = convert_offset_index(d, GICD_INMIR, &index);

[Severity: High]
Does this miss the ESPI mapping for GICD_INMIR?

When configuring an NMI for an Extended SPI (ESPI), convert_offset_index()
is called with GICD_INMIR. However, the ESPI_RANGE switch block in
convert_offset_index() currently lacks a case for GICD_INMIR:

	case ESPI_RANGE:
		*index = d->hwirq - ESPI_BASE_INTID;
		switch (offset) {
		case GICD_ISENABLER:
			return GICD_ISENABLERnE;
		...
		default:
			break;
		}

This will cause the switch to fall through to the default handler, trigger
a WARN_ON(1), reset the index to the raw hardware IRQ number (>= 4096),
and return the baseline GICD_INMIR offset.

Could this result in an out-of-bounds MMIO read-modify-write on an
incorrect register address when calculating addr below?

> +	mask = 1 << (index % 32);
> +
> +	if (gic_irq_in_rdist(d))
> +		base = gic_data_rdist_sgi_base();
> +	else
> +		base = gic_data.dist_base;
> +
> +	addr = base + offset + (index / 32) * 4;

[ ... ]

> @@ -857,6 +915,28 @@ static void __gic_handle_irq_from_irqson(struct pt_regs *regs)
>  	bool is_nmi;
>  	u32 irqnr;
>  
> +	/*
> +	 * We should enter here with interrupts disabled, otherwise we may met
> +	 * a race here with FEAT_NMI/FEAT_GICv3_NMI:
> +	 *
> +	 * [interrupt disabled]
> +	 *                   <- normal interrupt pending, for example timer interrupt
> +	 *                   <- NMI occurs, ISR_EL1.nmi = 1
> +	 * do_el1_interrupt()
> +	 *                   <- NMI withdraw, ISR_EL1.nmi = 0
> +	 *   ISR_EL1.nmi = 0, not an NMI interrupt
> +	 *   gic_handle_irq()
> +	 *     __gic_handle_irq_from_irqson()
> +	 *       irqnr = gic_read_iar() <- Oops, ack and handle an normal interrupt
> +	 *                                 in interrupt disabled context!
> +	 *
> +	 * So if we met this case here, just return from the interrupt context.
> +	 * Since the interrupt is still pending, we can handle it once the
> +	 * interrupt re-enabled and it'll not be missing.
> +	 */
> +	if (!interrupts_enabled(regs))
> +		return;

[Severity: Low]
This isn't a bug, but does this comment contradict the code immediately
below it?

The comment states "We should enter here with interrupts disabled", but the
code immediately returns if interrupts are disabled. Should the comment say
"enabled" instead, or "we could enter here with interrupts disabled"?

> +
>  	irqnr = gic_read_iar();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703100135.2512312-1-ruanjinjie@huawei.com?part=17