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

[Alex Williamson <alex@shazbot.org>]

On Thu, 30 Jul 2026 23:31:29 +0000
Josh Hilke <jrhilke@google.com> wrote:
> +static void igb_init(struct vfio_pci_device *device)
> +{
> +	struct igb *igb = to_igb_state(device);
> +	u64 iova_tx, iova_rx;
> +	u32 ctrl, rctl;
> +	u16 cmd_reg;
> +	int retries;
> +
> +	VFIO_ASSERT_GE(device->driver.region.size, sizeof(struct igb));
> +
> +	/* Set up rings and calculate IOVAs */
> +	igb->bar0 = device->bars[0].vaddr;
> +
> +	iova_tx = to_iova(device, igb->tx_ring);
> +	iova_rx = to_iova(device, igb->rx_ring);
> +
> +	igb_reset(igb);
> +
> +	/* Signal that the driver is loaded */
> +	ctrl = igb_read32(igb, E1000_CTRL_EXT);
> +	ctrl |= E1000_CTRL_EXT_DRV_LOAD;
> +	ctrl &= ~E1000_CTRL_EXT_LINK_MODE_MASK;
> +	igb_write32(igb, E1000_CTRL_EXT, ctrl);
> +
> +	/* Enable PCI Bus Master. */
> +	cmd_reg = vfio_pci_config_readw(device, PCI_COMMAND);
> +	if ((cmd_reg & (PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY)) !=
> +	    (PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY)) {
> +		cmd_reg |= (PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
> +		vfio_pci_config_writew(device, PCI_COMMAND, cmd_reg);
> +	}
> +
> +	/* Configure PHY internal loopback for testing. */
> +	igb_setup_loopback(igb);
> +
> +	/*
> +	 * Disable DMA re-send on PCIe completion timeout (82576 datasheet
> +	 * section 8.6.1, GCR.Completion_Timeout_Resend, bit 16).  The
> +	 * mix_and_match test intentionally submits descriptors targeting
> +	 * unmapped IOVAs; with the default (set) value, the device keeps
> +	 * retrying the failed read indefinitely, which keeps PCIe AER and
> +	 * IOMMU error handling busy and interferes with reset recovery.
> +	 */
> +	ctrl = igb_read32(igb, E1000_GCR);
> +	ctrl &= ~E1000_GCR_CMPL_TMOUT_RESEND;
> +	igb_write32(igb, E1000_GCR, ctrl);
> +
> +	/* Configure TX and RX descriptor rings */
> +	igb_write32(igb, E1000_TDBAL(0), (u32)iova_tx);
> +	igb_write32(igb, E1000_TDBAH(0), (u32)(iova_tx >> 32));
> +	igb_write32(igb, E1000_TDLEN(0), RING_SIZE * sizeof(struct igb_tx_desc));
> +	igb_write32(igb, E1000_TDH(0), 0);
> +	igb_write32(igb, E1000_TDT(0), 0);
> +	igb_write32(igb, E1000_TXDCTL(0), E1000_TXDCTL_QUEUE_ENABLE);
> +
> +	igb_write32(igb, E1000_RDBAL(0), (u32)iova_rx);
> +	igb_write32(igb, E1000_RDBAH(0), (u32)(iova_rx >> 32));
> +	igb_write32(igb, E1000_RDLEN(0), RING_SIZE * sizeof(struct igb_rx_desc));
> +	igb_write32(igb, E1000_RDH(0), 0);
> +	igb_write32(igb, E1000_RDT(0), 0);
> +
> +	/*
> +	 * Select the advanced one-buffer descriptor format.  Per 82576
> +	 * datasheet section 7.1.5.2: "SRRCTL[n].DESCTYPE must be set to a
> +	 * value other than 000b for the 82576 to write back the special
> +	 * descriptors."  struct igb_rx_desc matches the advanced one-buffer
> +	 * writeback layout (section 7.1.5.2), so polling rx.wb.status_error
> +	 * requires this format.  Section 8.10.2 specifies DESCTYPE[27:25].
> +	 *
> +	 * The direct write also zeroes SRRCTL.BSIZEPACKET, which is
> +	 * intentional: per section 7.1.3.1 a zero BSIZEPACKET falls back to
> +	 * the RCTL.BSIZE buffer size, whose reset default (00b) is 2048
> +	 * bytes -- ample for the loopback frames here.
> +	 */
> +	igb_write32(igb, E1000_SRRCTL(0), E1000_SRRCTL_DESCTYPE_ADV_ONEBUF);
> +
> +	igb_write32(igb, E1000_RXDCTL(0), E1000_RXDCTL_QUEUE_ENABLE);
> +
> +	/* Wait for TX and RX queues to be enabled */
> +	retries = 2000;
> +	while (retries-- > 0) {
> +		if ((igb_read32(igb, E1000_TXDCTL(0)) & E1000_TXDCTL_QUEUE_ENABLE) &&
> +		    (igb_read32(igb, E1000_RXDCTL(0)) & E1000_RXDCTL_QUEUE_ENABLE))
> +			break;
> +		usleep(10);
> +	}
> +	VFIO_ASSERT_GE(retries, 0);

I'm not sure how I missed this in the previous iteration, but this
new-ish assert exposes a latent ordering issue on real hardware.  As per
the below referenced register definitions, the per-queue enable bits are
zero until the global enable bits are set.

diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index 97a2f29aade3..fae523059f86 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -303,16 +303,6 @@ static void igb_hw_init(struct vfio_pci_device *device)
 
        igb_write32(igb, E1000_RXDCTL(0), E1000_RXDCTL_QUEUE_ENABLE);
 
-       /* Wait for TX and RX queues to be enabled */
-       retries = 2000;
-       while (retries-- > 0) {
-               if ((igb_read32(igb, E1000_TXDCTL(0)) & E1000_TXDCTL_QUEUE_ENABLE) &&
-                   (igb_read32(igb, E1000_RXDCTL(0)) & E1000_RXDCTL_QUEUE_ENABLE))
-                       break;
-               usleep(10);
-       }
-       VFIO_ASSERT_GE(retries, 0);
-
        /*
         * Enable Receiver and Transmitter.  RCTL.LBM_MAC is set in addition
         * to PHY loopback as a QEMU-only accommodation: QEMU's emulated igb
@@ -335,6 +325,21 @@ static void igb_hw_init(struct vfio_pci_device *device)
        igb_write32(igb, E1000_RCTL, rctl);
        igb_write32(igb, E1000_TCTL, E1000_TCTL_EN | E1000_TCTL_PSP);
 
+       /*
+        * Wait for TX and RX queues to be enabled.  Per the RXDCTL/TXDCTL
+        * register definitions (8.10.10/8.12.13), the per-queue enable bit
+        * "remains zero" until the global RCTL.RXEN/TCTL.TXEN are set, so
+        * E1000_RCTL_EN and E1000_TCTL_EN must already be written above.
+        */
+       retries = 2000;
+       while (retries-- > 0) {
+               if ((igb_read32(igb, E1000_TXDCTL(0)) & E1000_TXDCTL_QUEUE_ENABLE) &&
+                   (igb_read32(igb, E1000_RXDCTL(0)) & E1000_RXDCTL_QUEUE_ENABLE))
+                       break;
+               usleep(10);
+       }
+       VFIO_ASSERT_GE(retries, 0);
+
        /*
         * Program MSI-X interrupt routing per 82576 datasheet:
         *

Back to 35/35 on real hardware with this.  NB, patch is against fully
applied series, so lands in igb_hw_init() but the source of the
ordering issue is in patch 1 here, where it's igb_init().  Thanks,

Alex

> +
> +	/*
> +	 * Enable Receiver and Transmitter.  RCTL.LBM_MAC is set in addition
> +	 * to PHY loopback as a QEMU-only accommodation: QEMU's emulated igb
> +	 * does not honor PHY register 0 bit 14 (PHY internal loopback) and
> +	 * relies on RCTL.LBM_MAC to wrap TX descriptors back to the RX
> +	 * queue.  Datasheet 8.10.1 (RCTL register) advises "When using the
> +	 * internal PHY, LBM should remain set to 00b", so setting LBM_MAC
> +	 * here deviates from datasheet guidance; empirically the bit has
> +	 * no observable effect on real 82576 hardware because MAC loopback
> +	 * is not implemented (datasheet 3.5.6.2).  Setting both lets the
> +	 * selftest work on both real hardware and QEMU without conditional
> +	 * code paths.
> +	 */
> +	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 - for QEMU emulation only */
> +	       E1000_RCTL_SECRC;     /* Strip CRC (needed for memcmp) */
> +	igb_write32(igb, E1000_RCTL, rctl);
> +	igb_write32(igb, E1000_TCTL, E1000_TCTL_EN | E1000_TCTL_PSP);