Re: [PATCH v2 01/10] selftests/vfio: igb: Add driver for IGB QEMU device

[David Matlack <dmatlack@google.com>]

On 2026-05-27 12:32 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
> - [High] Silent failures during device initialization leave the driver in an invalid state.
> - [High] PCI_COMMAND_MEMORY enablement is skipped if PCI_COMMAND_MASTER is already set.
> - [High] Device reset is asynchronous but not awaited, and MSI-X is disabled before the device is quiesced.
> - [High] `igb_memcpy_start` fails to increment `src` and `dst` pointers for multiple chunks.
> - [Medium] Missing SRRCTL configuration causes incorrect Rx descriptor write-back offset on real hardware.
> - [Medium] Missing Transmit FCS (Frame Check Sequence) insertion causes payload truncation.
> - [Medium] Lockless polling of DMA memory without READ_ONCE() or a volatile cast.
> - [Medium] Incorrect MDI Error bit offset in MDIC register definition.
> - [Medium] Missing read/write memory barriers around DMA descriptor and doorbell accesses.
> - [Medium] Missing endianness conversion macros when populating hardware descriptors.
> - [Medium] Unsafe hardware reset logic uses a sleep instead of polling the reset completion bit.
> - [Medium] Incorrect bit definition for IGB_GPIE_EIAME silently disables auto-masking functionality.
> --
> 
> commit f2aa2cf5b25f70be67a7ce0a012a1bdb1dba535c
> Author: Josh Hilke <jrhilke@google.com>
> 
> selftests/vfio: igb: Add driver for IGB QEMU device
>     
> Add a VFIO selftest driver for the Intel Gigabit Ethernet controller
> (IGB). IGB is fully virtualized in QEMU [1] which makes it easy to run
> VFIO selftests without needing any specific hardware.
> 
> [1] https://www.qemu.org/docs/master/system/devices/igb.html
> [2] https://github.com/arighi/virtme-ng
> 
> > 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..064ca7c05da02
> > --- /dev/null
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> [ ... ]
> > +static void igb_init(struct vfio_pci_device *device)
> > +{
> [ ... ]
> > +	/* Reset device and disable all interrupts */
> > +	igb_write32(igb, IGB_CTRL, igb_read32(igb, IGB_CTRL) | IGB_CTRL_RST);
> > +	usleep(20000);
> 
> [Severity: Medium]
> Could this unsafe hardware reset logic cause issues by sleeping instead of
> polling the reset completion bit? Accessing device registers while a reset is
> propagating internally could trigger PCIe bus hangs.

Please fix this. At minimum the sleep needs to be documented if that is
relying on duration mandated by the hardware spec.

> 
> > +	igb_write32(igb, IGB_IMC, 0xFFFFFFFF);
> > +
> > +	/* Signal that the driver is loaded */
> > +	ctrl = igb_read32(igb, IGB_CTRL_EXT);
> > +	ctrl |= IGB_CTRL_EXT_DRV_LOAD;
> > +	ctrl &= ~IGB_CTRL_EXT_LINK_MODE_MASK;
> > +	igb_write32(igb, IGB_CTRL_EXT, ctrl);
> > +
> > +	/* Enable PCI Bus Master. */
> > +	cmd_reg = vfio_pci_config_readw(device, PCI_COMMAND);
> > +	if (!(cmd_reg & PCI_COMMAND_MASTER)) {
> > +		cmd_reg |= (PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
> > +		vfio_pci_config_writew(device, PCI_COMMAND, cmd_reg);
> > +	}
> 
> [Severity: High]
> Does this logic inadvertently skip enabling PCI_COMMAND_MEMORY if
> PCI_COMMAND_MASTER is already set? If the Bus Master bit is already enabled
> by firmware or VFIO but Memory Space is disabled, the block is skipped, which
> would leave Memory Space disabled and cause subsequent MMIO accesses to
> trigger bus faults.

Please fix this.

> 
> > +
> > +	/* Trigger autonegotiation. This enables IGB to transmit data. */
> > +	if (igb_phy_setup_autoneg(igb))
> > +		return;
> 
> [Severity: High]
> Does this early return result in a silent failure during device initialization?
> If it fails, crucial setup steps are skipped, leaving variables like
> max_memcpy_size uninitialized. The test framework will proceed to use the
> partially configured device, leading to undefined behavior or crashes.
> Could it explicitly assert failure via VFIO_ASSERT() instead?

Please fix this. IMO igb_phy_setup_autoneg() should use VFIO_ASSERT()
instead of returning an int.
> 
> > +
> > +	/* Configure TX and RX descriptor rings */
> > +	igb_write32(igb, IGB_TDBAL0, (u32)iova_tx);
> > +	igb_write32(igb, IGB_TDBAH0, (u32)(iova_tx >> 32));
> > +	igb_write32(igb, IGB_TDLEN0, RING_SIZE * sizeof(struct igb_tx_desc));
> > +	igb_write32(igb, IGB_TDH0, 0);
> > +	igb_write32(igb, IGB_TDT0, 0);
> > +	igb_write32(igb, IGB_TXDCTL0, IGB_TXDCTL0_Q_EN);
> > +
> > +	igb_write32(igb, IGB_RDBAL0, (u32)iova_rx);
> 
> [Severity: Medium]
> Is the SRRCTL configuration missing here? The driver expects the hardware to
> use Advanced Receive Descriptors and polls for the DD bit at byte offset 8,
> but fails to configure the SRRCTL register. Real hardware defaults to Legacy
> Receive Descriptors (DD bit at offset 4), causing polling timeouts.
> 
> I noticed this is fixed in a later commit in the series: "selftests/vfio: igb:
> Use advanced TX and RX descriptors".
> 
> [ ... ]
> > +static void igb_remove(struct vfio_pci_device *device)
> > +{
> > +	struct igb *igb = to_igb_state(device);
> > +
> > +	vfio_pci_msix_disable(device);
> > +	igb_write32(igb, IGB_RCTL, 0);
> > +	igb_write32(igb, IGB_TCTL, 0);
> > +	igb_write32(igb, IGB_CTRL, igb_read32(igb, IGB_CTRL) | IGB_CTRL_RST);
> > +}
> 
> [Severity: High]
> Is it safe to disable MSI-X before the device is quiesced, and trigger an
> asynchronous device reset without awaiting its completion?
> 
> Calling vfio_pci_msix_disable() before masking interrupts and triggering a
> hardware reset risks rogue interrupts during teardown. Returning immediately
> after setting IGB_CTRL_RST might create a race condition where the VFIO
> framework destroys IOMMU mappings while the device is still resetting,
> potentially leading to a DMA use-after-free.

I don't think this is a bug. The device should not be sending interrupts
after they are disabled in the device itself.

> 
> [ ... ]
> > +static void igb_memcpy_start(struct vfio_pci_device *device, iova_t src,
> > +			     iova_t dst, u64 size, u64 count)
> > +{
> > +	struct igb *igb = to_igb_state(device);
> > +	struct igb_rx_desc *rx;
> > +	struct igb_tx_desc *tx;
> > +	u32 i;
> > +
> > +	igb_irq_disable(igb);
> > +
> > +	for (i = 0; i < count; i++) {
> > +		tx = &igb->tx_ring[igb->tx_tail];
> > +		rx = &igb->rx_ring[igb->rx_tail];
> > +
> > +		memset(tx, 0, sizeof(struct igb_tx_desc));
> > +		memset(rx, 0, sizeof(struct igb_rx_desc));
> > +
> > +		rx->read.pkt_addr = dst;
> > +		tx->read.buffer_addr = src;
> > +		tx->read.cmd_type_len = (u32)size;
> > +		tx->read.cmd_type_len |= (u32)(IGB_TXD_CMD_EOP) << IGB_TXD_CMD_SHIFT;
> 
> [Severity: High]
> Does this loop fail to increment the src and dst pointers for multiple chunks?
> When count > 1, the device repeatedly copies the first chunk of data to the
> exact same destination address, leaving the remainder of the buffer uncopied
> and causing data verification failures.

This is not a bug, that's how memcpy_start() is defined to behave.

> [Severity: Medium]
> Are endianness conversion macros missing here? Populating 64-bit addresses
> and 32-bit fields in hardware descriptors using native CPU byte order instead
> of cpu_to_le32 and cpu_to_le64 will break on big-endian host architectures.

Please fix this.

> [Severity: Medium]
> Should the Transmit FCS (Frame Check Sequence) insertion flag be present?
> The driver sets IGB_RCTL_SECRC to strip Ethernet CRC on RX but omits the
> IGB_TXD_CMD_IFCS flag on TX. On real hardware, no CRC is appended, causing
> the RX MAC to strip the last 4 bytes of the actual payload.
> 
> I noticed this is fixed in a later commit in the series: "selftests/vfio: igb:
> Use advanced TX and RX descriptors".
> 
> > +
> > +		/* Set to 0 to disable offloads and avoid needing a context descriptor */
> > +		tx->read.olinfo_status = 0;
> > +
> > +		igb->tx_tail = (igb->tx_tail + 1) % RING_SIZE;
> > +		igb->rx_tail = (igb->rx_tail + 1) % RING_SIZE;
> > +	}
> > +
> > +	igb_write32(igb, IGB_RDT0, igb->rx_tail);
> > +	igb_write32(igb, IGB_TDT0, igb->tx_tail);
> 
> [Severity: Medium]
> Are write memory barriers needed here around the DMA descriptor and doorbell
> accesses? Without a write memory barrier between populating RAM descriptors
> and ringing the TX doorbell, the hardware might fetch stale descriptors.

Can you look into this one? I would have assumed writel() would provide
the proper barriers.

> 
> > +}
> > +
> > +static int igb_memcpy_wait(struct vfio_pci_device *device)
> > +{
> > +	struct igb *igb = to_igb_state(device);
> > +	struct igb_rx_desc *rx;
> > +	u32 prev_tail;
> > +	int retries;
> > +
> > +	prev_tail = (igb->rx_tail + RING_SIZE - 1) % RING_SIZE;
> > +	rx = &igb->rx_ring[prev_tail];
> > +
> > +	retries = 100;
> > +	while (retries-- > 0) {
> > +		if (rx->wb.status_error & 1)
> 
> [Severity: Medium]
> Could this lockless polling of DMA memory without READ_ONCE() or a volatile
> cast cause problems? The loop polls rx->wb.status_error, which allows the
> compiler to theoretically cache the value in a register. While the external
> usleep() call typically acts as an implicit compiler barrier, relying on it
> is brittle and constitutes a data race.

Please fix this one.

> 
> > +			break;
> > +		usleep(10);
> > +	}
> > +
> > +	igb_irq_clear(igb);
> > +
> > +	igb_irq_enable(igb);
> > +
> > +	if (rx->wb.status_error & 1)
> 
> [Severity: Medium]
> Is a read memory barrier missing here after polling the DMA completion status?
> Without it, there could be speculative CPU reads of the destination buffer.

Maybe just avoid this issue entirely and read rx->wb.status_error into a
variable so you can check it again after the loop.

> 
> > +		return 0;
> > +
> > +	return -ETIMEDOUT;

nit: return error ? -ETIMEDOUT : 0;

> > +}
> [ ... ]
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/registers.h b/tools/testing/selftests/vfio/lib/drivers/igb/registers.h
> > new file mode 100644
> > index 0000000000000..e2756be2e9c0c
> > --- /dev/null
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/registers.h
> [ ... ]
> > +#define IGB_MDIC_PHY_SHIFT 21 /* PHY Address Shift */
> > +#define IGB_MDIC_REG_SHIFT 16 /* Register Address Shift */
> > +#define IGB_MDIC_READY (1 << 28) /* MDI Data Ready */
> > +#define IGB_MDIC_ERROR (1 << 29) /* MDI Error */
> 
> [Severity: Medium]
> Is this MDI Error bit offset correct in the MDIC register definition?
> The macro is defined as (1 << 29), but according to the datasheet, bit 29 is
> the Interrupt Enable bit, while the actual MDI Error bit is bit 30.
> 
> I noticed this is fixed in a later commit in the series: "selftests/vfio: igb:
> Use offical IGB headers in selftest driver".

Please fix this.

> 
> [ ... ]
> > +#define IGB_CR_1000T_FD_CAPS 0x0200 /* Advertise 1000 Mbps Full Duplex */
> > +
> > +#define IGB_GPIE_EIAME 0x10 /* Extended Interrupt Auto Mask Enable */
> 
> [Severity: Medium]
> Does this incorrect bit definition for IGB_GPIE_EIAME silently disable
> auto-masking functionality? It is defined as 0x10, which corresponds to
> Multiple_MSIX. The actual Extended Interrupt Auto Mask Enable bit is bit 30.
> Writing 0x10 fails to enable auto-masking.
> 
> I noticed this is fixed in a later commit in the series: "selftests/vfio: igb:
> Program MSI-X interrupt routing".

Lot of bugs that get fixed later in the series. I think you want to
preserve Alex's author tag by keeping them separate, but let's not
introduce any temporary bugs.

Since we haven't merged anything yet, let's fix all the bugs in this
patch directly, and use the follow up patches for any improvements.

> 
> > +#define IGB_IVAR_VALID 0x80 /* Valid bit for IVAR register */
> > +
> > +#define IGB_TXD_CMD_EOP 0x01 /* End of Packet */
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260526235417.2058313-1-jrhilke@google.com?part=1
>