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

[Alex Williamson <alex@shazbot.org>]

On Wed, 08 Jul 2026 23:33:40 +0000
sashiko-bot@kernel.org wrote:

> 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?

Again the paragraph can mostly go away, the only thing left seems to be
replacing 1 with E1000_EICR_RX_QUEUE0.

> > 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?

Yes, this can change to:

  -     /* Enable interrupts on vector 0 */
  -     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);
  +
  +     /* Enable interrupts on vector 0 once routing is established */
  +     igb_write32(igb, E1000_EIMS, E1000_EICR_RX_QUEUE0);

Thanks,
Alex