Re: [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Commit message and implementation mismatch regarding macro definitions.
- [Medium] Hardware Initialization Order: Unmasking interrupts before routing is configured.
--

--- Patch [4]: [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing ---
commit f99b27fd1b6bc3801803a7e5be3ec664facafb7a
Author: Alex Williamson <alex.williamson@nvidia.com>
Subject: vfio: selftests: igb: Program MSI-X interrupt routing

This commit updates the IGB selftest driver to use the full MSI-X programming
sequence required by the 82576 hardware instead of relying on synchronous
QEMU workarounds. It configures auto-clear and auto-mask for vector 0 and
changes the EICR clear mechanism from read-to-clear to write-to-clear.

> Replace the magic '1' values written to EIMS/EIMC with IGB_EICR_VEC0,
> add the GPIE/EIAC/EIAM macros, and drop the wrong-valued IGB_GPIE_EIAME
> macro (the new definition lives next to IGB_GPIE_MULTIPLE_MSIX).

[Severity: Medium]
This isn't a bug in the code, but the commit message mentions introducing
custom macros like IGB_EICR_VEC0 and dropping IGB_GPIE_EIAME.

However, the implementation uses standard kernel definitions such as
E1000_EICR_RX_QUEUE0 and E1000_GPIE_MSIX_MODE.

Could the commit message be updated to match the actual code?

> diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c

[ ... ]

> @@ -280,11 +280,32 @@ static void igb_init(struct vfio_pci_device *device)
>  	/* Enable MSI-X with 1 vector for the test */
>  	vfio_pci_msix_enable(device, MSIX_VECTOR, 1);
>  
> -	/* Enable auto-masking of interrupts to avoid storms without a real ISR */
> -	igb_write32(igb, E1000_GPIE, E1000_GPIE_EIAME);
> +	/*
> +	 * Program MSI-X interrupt routing per 82576 datasheet:
> +	 *
> +	 * GPIE (section 7.3.2.11, Table 7-47): set Multiple_MSIX (bit 4) to
> +	 * route interrupt causes through IVAR mapping, and EIAME (bit 30)
> +	 * to apply EIAM on MSI-X assertion (without EIAME, EIAM only
> +	 * applies on EICR read/write).
> +	 *
> +	 * EIAC (section 8.8.5): enable auto-clear of EICR for vector 0.
> +	 * Without auto-clear the cause stays set after delivery and the
> +	 * test can see spurious interrupts on the next memcpy batch.
> +	 *
> +	 * EIAM (section 8.8.6): enable auto-mask of EIMS for vector 0 on
> +	 * MSI-X assertion (effective because EIAME is set), so a single
> +	 * interrupt is delivered per memcpy batch even if the cause
> +	 * re-asserts before software re-enables the mask.
> +	 *
> +	 * IVAR (section 7.3.1.2, register definition in 8.8.13): map RX
> +	 * cause 0 to MSI-X vector 0 and mark the entry valid.
> +	 */
> +	igb_write32(igb, E1000_GPIE, E1000_GPIE_MSIX_MODE | E1000_GPIE_EIAME);
> +	igb_write32(igb, E1000_EIAC, E1000_EICR_RX_QUEUE0);
> +	igb_write32(igb, E1000_EIAM, E1000_EICR_RX_QUEUE0);
>  
>  	/* Enable interrupts on vector 0 */
> -	igb_write32(igb, E1000_EIMS, 1);
> +	igb_write32(igb, E1000_EIMS, E1000_EICR_RX_QUEUE0);
>  
>  	/* Map vector 0 to interrupt cause 0 and mark it valid */
>  	igb_write32(igb, E1000_IVAR0, E1000_IVAR_VALID);

[Severity: Medium]
Does unmasking the interrupt via E1000_EIMS before programming the routing
in E1000_IVAR0 risk unpredictable behavior?

If an interrupt condition is met in the small window between these writes, it
seems an interrupt could be triggered before the IVAR mapping is valid.

Should the mapping in IVAR0 be established before unmasking the interrupt?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-igb_v3_b4-v3-0-1272650ca614@google.com?part=4