Re: [PATCH v2 04/10] selftests/vfio: igb: Program MSI-X interrupt routing

[Alex Williamson <alex.williamson@nvidia.com>]

On Wed, 27 May 2026 00:19:08 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] igb_send_msi() was overlooked by the patch, causing a state regression due to auto-masking and leaving a magic number unreplaced.
> --
> 
> commit 059a9f7e058b59c775b2202ffd0af3e078efbd8a
> Author: Alex Williamson <alex.williamson@nvidia.com>
> 
> selftests/vfio: igb: Program MSI-X interrupt routing
> 
> This commit programs the full MSI-X routing sequence for the igb driver in the
> vfio selftests. It enables auto-clear and auto-mask for vector 0 and replaces
> magic numbers with appropriate macros to match hardware behavior.
> 
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > index c0f9cefb23e59..a8f2072b68200 100644
> > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > @@ -271,11 +271,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, IGB_GPIE, IGB_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, IGB_GPIE, IGB_GPIE_MULTIPLE_MSIX | IGB_GPIE_EIAME);
> > +	igb_write32(igb, IGB_EIAC, IGB_EICR_VEC0);
> > +	igb_write32(igb, IGB_EIAM, IGB_EICR_VEC0);  
> 
> [Severity: Medium]
> Will this auto-masking configuration break software interrupt generation in
> igb_send_msi()?
> 
> When igb_send_msi() writes to IGB_EICS to generate a software interrupt,
> the hardware asserts the MSI-X message and auto-masks IGB_EIMS based on
> this EIAM configuration.
> 
> Because igb_send_msi() does not explicitly unmask the interrupt before
> triggering it, will consecutive calls to igb_send_msi() silently fail to
> deliver the interrupt if there is no intervening unmask?

7.3.2.10  Extended Interrupt Auto Mask Enable Register (EIAM)

  Each bit set in this register enables clearing of the corresponding
  bit in the extended mask register following read or write-to-clear to
  EICR. It also enables setting of the corresponding bit in the
  extended mask register following a write-to-set to EICS.

Table 7-47

  EIAME: When set, upon firing of an MSI-X message, mask bits set in
  EIAM associated with this message are cleared. Otherwise, EIAM is
  used only upon read or write of EICR/EICS registers.

7.3.2.7  Extended Interrupt Cause Set Register (EICS)

  This registers allows to set the bits of EICR by software, by writing
  a 1b in the corresponding bits in EICS. Used usually to rearm
  interrupts the software didn't have time to handle in the current
  interrupt routine.

With EIAM[VEC0]=1 and EIMS[VEC0]=1 (set at init):

 1. igb_send_msi() writes EICS[VEC0]=1 → sets EICR[VEC0]

 2. Per 7.3.2.10, the write-to-set to EICS makes EIAM set EIMS[VEC0]m
    so the mask is armed by the very same operation, regardless of its
    prior state.

 3. EICR[VEC0]=1 && EIMS[VEC0]=1 → MSI-X message fires (per 7.3.2.8:
   "Interrupts appear on PCIe only if the interrupt cause bit is a one
   and the corresponding interrupt mask bit is a one").

 4. On assertion, with GPIE.EIAME=1, EIAM clears EIMS[VEC0] (one-shot throttle) and
   EIAC clears EICR[VEC0].

 5. Next igb_send_msi() → step 2 re-sets EIMS[VEC0] again. Self-arming.
    No external unmask required.

The auto-clear (step 4) that the sashiko focused on is exactly undone by
the auto-set on the next EICS write (step 2). The two halves of EIAM
are what make the software-MSI path self-contained.

Verified on hardware with back-to-back send_msi().  False positive.

> tools/testing/selftests/vfio/lib/drivers/igb/igb.c:igb_send_msi() {
> ...
> 	igb_write32(igb, IGB_EICS, 1);
> ...
> }
> 
> Also, does the hardcoded '1' in igb_send_msi() need to be replaced
>   with IGB_EICR_VEC0 to match the other magic number replacements in
>   this commit?

Yes, this should have included:

diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index a8f2072b6820..b4f75965e18d 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -424,7 +424,7 @@ static void igb_send_msi(struct vfio_pci_device *device)
 {
 	struct igb *igb = to_igb_state(device);
 
-	igb_write32(igb, IGB_EICS, 1);
+	igb_write32(igb, IGB_EICS, IGB_EICR_VEC0);
 }
 
 const struct vfio_pci_driver_ops igb_ops = {

This should be added on commit or respin.  Thanks,

Alex