Re: [PATCH v8 1/6] vfio: selftests: igb: Add driver for Intel 82576 device

On Wed, Jul 29, 2026 at 3:45 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] Hardware Testing Failure / Invalid Loopback Configuration
> - [Medium] Hardware Configuration / Frame Padding failure for short packets
> - [Low] Misleading comment about interrupt delivery during memcpy
> --
>
> --- Patch [1]: [PATCH v8 1/6] vfio: selftests: igb: Add driver for Intel 82576 device ---
> commit e83f78e00ae490d2604290c3e38f2458c48dd9d4
> Author: Josh Hilke <[email protected]>
>
> vfio: selftests: igb: Add driver for Intel 82576 device
>
> This commit adds a VFIO selftest driver for the Intel 82576 Gigabit Ethernet
> controller. It uses advanced descriptors and programs MSI-X routing to test
> DMA loopback via QEMU's igb emulation and physical hardware.
>
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > new file mode 100644
> > index 0000000000000..6fc6e7c0e7235
> > --- /dev/null
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
>
> [ ... ]
>
> > +static void igb_phy_setup_autoneg(struct igb *igb)
> > +{
> > +     int timeout_ms = 1000;
> > +     bool success = false;
> > +     u16 phy_status;
> > +     int ret;
> > +     int i;
> > +
> > +     /* Trigger auto-negotiation */
> > +     ret = igb_write_phy(igb, MII_BMCR,
> > +                         BMCR_ANENABLE | BMCR_ANRESTART);
>
> [Severity: Medium]
> Will this auto-negotiation time out and fail the test via VFIO_ASSERT_TRUE
> when running on physical 82576 hardware without an active external link
> partner?

This is addressed in patch 2, but I'm going to squash patch 2 into
this patch to avoid introducing bugs.

> > +     VFIO_ASSERT_EQ(ret, 0, "Failed to write PHY control register");
> > +
> > +     for (i = 0; i < timeout_ms; i++) {
> > +             if (igb_read_phy(igb, MII_BMSR, &phy_status) == 0) {
> > +                     success = !!(phy_status & BMSR_ANEGCOMPLETE);
> > +                     if (success)
> > +                             break;
> > +             }
> > +             usleep(1000);
> > +     }
> > +
> > +     VFIO_ASSERT_TRUE(success, "Auto-negotiation did not complete in time");
> > +}
>
> [ ... ]
>
> > +static void igb_init(struct vfio_pci_device *device)
> > +{
>
> [ ... ]
>
> > +     /* Enable Receiver and Transmitter */
> > +     rctl = E1000_RCTL_EN |       /* Receiver Enable */
> > +            E1000_RCTL_UPE |      /* Unicast Promiscuous (for dummy MAC) */
> > +            E1000_RCTL_MPE |      /* Multicast Promiscuous */
> > +            E1000_RCTL_BAM |      /* Broadcast Accept Mode */
> > +            E1000_RCTL_LBM_MAC |  /* MAC Loopback Mode */
>
> [Severity: Medium]
> Does real 82576 hardware support MAC loopback? It appears this might cause DMA
> loopback to fail on physical hardware even if auto-negotiation completes.

Addressed in patch 2.

> > +            E1000_RCTL_SECRC;     /* Strip CRC (needed for memcmp) */
> > +     igb_write32(igb, E1000_RCTL, rctl);
> > +     igb_write32(igb, E1000_TCTL, E1000_TCTL_EN);
>
> [Severity: Medium]
> Is the Pad Short Packets bit (E1000_TCTL_PSP) missing from the TCTL
> configuration here?
>
> If vfio_pci_driver_memcpy() is called with a size smaller than 60 bytes,
> would the hardware send a runt frame that the receiver drops (because Store
> Bad Packets is disabled), causing igb_memcpy_wait() to time out with
> -ETIMEDOUT?

Yes, this is a bug. Verified this by modifying the memcpy size in
vfio_pci_driver_test. I'll fix this in v9.

> > +
> > +     /* Enable MSI-X with 1 vector for the test */
> > +     vfio_pci_msix_enable(device, MSIX_VECTOR, 1);
> > +
> > +     /*
> > +      * 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.
>
> [Severity: Low]
> This isn't a bug, but is this comment accurate about delivering a single
> interrupt per memcpy batch?
>
> Looking at igb_memcpy_start(), it explicitly disables interrupts before the
> batch:
>
>     igb_irq_disable(igb);
>
> and igb_memcpy_wait() polls the descriptor writeback before clearing the
> cause and re-enabling interrupts:
>
>     igb_irq_clear(igb);
>     igb_irq_enable(igb);
>
> It appears interrupts are prevented entirely during the polling loop.

I'll update the comment in v9.

> > +      *
> > +      * 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.
> > +      */
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1