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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The selftest relies on external PHY auto-negotiation, which will cause the test to fail on real hardware if no physical link (cable) is connected.
- [Medium] MMIO registers are accessed before PCI memory space is enabled, which can crash the test if memory space is disabled.
--

--- Patch [1]: [PATCH v5 1/5] vfio: selftests: igb: Add driver for Intel 82576 device ---
commit b66293558b628d9af7dfd3c68ff9a1e69b1cbb0b
Author: Josh Hilke <jrhilke@google.com>

vfio: selftests: igb: Add driver for Intel 82576 device

Add a VFIO selftest driver for the Intel Gigabit Ethernet controller
(IGB), specifically targeting the 82576 device. IGB is fully
virtualized in QEMU which makes it easy to run VFIO selftests without
needing any specific 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..a59b953030925
> --- /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);
> +	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");

[Severity: Medium]
Will this cause the selftest to fail on real hardware if no physical cable is
connected?

Since this relies on external PHY auto-negotiation, it looks like the
BMSR_ANEGCOMPLETE bit will never set without a physical link partner,
triggering this assertion failure.

[ ... ]

> +static void igb_reset(struct igb *igb)
> +{
> +	igb_write32(igb, E1000_CTRL, igb_read32(igb, E1000_CTRL) | E1000_CTRL_RST);
> +	/*
> +	 * Must wait at least 1 millisecond after setting the reset bit before
> +	 * checking if this device is ready to be used (82576 datasheet section
> +	 * 4.2.1.6.1).
> +	 */
> +	usleep(2000);
> +	VFIO_ASSERT_EQ(igb_read32(igb, E1000_CTRL) & E1000_CTRL_RST, 0);

[Severity: Medium]
Can this assertion fail due to igb_read32() returning 0xFFFFFFFF if PCI
memory space is disabled?

[ ... ]

> +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);

[Severity: Medium]
Is it safe to perform MMIO accesses in igb_reset() before the PCI memory
space is enabled below?

If the device was recently reset or the system does not pre-enable memory
space, these early MMIO accesses might fail.

> +
> +	/* 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);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-igb_v3_b4-v5-0-91503eddb98b@google.com?part=1