* [PATCH v3 1/9] vfio: selftests: igb: Add driver for IGB QEMU device
2026-07-08 23:18 [PATCH v3 0/9] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
@ 2026-07-08 23:18 ` Josh Hilke
2026-07-08 23:31 ` sashiko-bot
2026-07-08 23:18 ` [PATCH v3 2/9] vfio: selftests: igb: Use PHY internal loopback on 82576 Josh Hilke
` (7 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Josh Hilke @ 2026-07-08 23:18 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Shuah Khan, linux-kernel, kvm, linux-kselftest, Vipin Sharma,
Josh Hilke
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.
IGB does not have a default memcpy operation, and memcpy is required for
all VFIO selftest drivers. However, IGB has a "loopback" mode which is
used in this driver to implement memcpy. When IGB is in loopback mode,
it doesn't send data out to the network. Instead, it sends data from the
Tx queue directly to the Rx queue which points to the memcpy
destination, instead of sending the data out to the network.
This driver passes all of the VFIO selftests in
tools/testing/selftests/vfio/ when running in QEMU. The driver has not
been tested using a real IGB device since the main goal of writing this
driver is to run VFIO selftests in QEMU without requiring any hardware.
This command is used to test the driver. It runs
./tools/testing/selftests/vfio/vfio_pci_driver_test using virtme-ng [2],
which runs a kernel in a virtualized environment using QEMU that has a
copy-on-write snapshot the host filesystem.
vng \
--run arch/x86/boot/bzImage \
--user root \
--disable-microvm \
--memory 32G \
--cpus 8 \
--qemu-opts="-M q35,accel=kvm,kernel-irqchip=split" \
--qemu-opts="-device intel-iommu,intremap=on,caching-mode=on,device-iotlb=on" \
--qemu-opts="-netdev user,id=net0 -device igb,netdev=net0,addr=09.0" \
--append "console=ttyS0 earlyprintk=ttyS0 intel_iommu=on iommu=pt" \
--exec "modprobe vfio-pci && \
./tools/testing/selftests/vfio/scripts/setup.sh 0000:00:09.0 && \
./tools/testing/selftests/vfio/scripts/run.sh ./tools/testing/selftests/vfio/vfio_pci_driver_test"
Code was written entirely by AI (Gemini) by feeding it the Intel IGB
specification, the code for the real IGB driver, and the QEMU
implementation of the IGB device. It took many iterations of prompting
to get the driver to pass all of the tests, and lot's of de-slopping to
remove unnecessary code and make the code readable.
Code comments are also written by Gemini through iterative
prompting.
This patch is based on the kvm/queue branch.
[1] https://www.qemu.org/docs/master/system/devices/igb.html
[2] https://github.com/arighi/virtme-ng
Assisted-by: Gemini:gemini-3-pro-preview
Signed-off-by: Josh Hilke <jrhilke@google.com>
---
.../selftests/vfio/lib/drivers/igb/e1000_82575.h | 1 +
.../selftests/vfio/lib/drivers/igb/e1000_defines.h | 1 +
.../selftests/vfio/lib/drivers/igb/e1000_regs.h | 1 +
tools/testing/selftests/vfio/lib/drivers/igb/igb.c | 362 +++++++++++++++++++++
tools/testing/selftests/vfio/lib/libvfio.mk | 1 +
tools/testing/selftests/vfio/lib/vfio_pci_driver.c | 2 +
6 files changed, 368 insertions(+)
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/e1000_82575.h b/tools/testing/selftests/vfio/lib/drivers/igb/e1000_82575.h
new file mode 120000
index 000000000000..b84affdec559
--- /dev/null
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/e1000_82575.h
@@ -0,0 +1 @@
+../../../../../../../drivers/net/ethernet/intel/igb/e1000_82575.h
\ No newline at end of file
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/e1000_defines.h b/tools/testing/selftests/vfio/lib/drivers/igb/e1000_defines.h
new file mode 120000
index 000000000000..9f97f4330086
--- /dev/null
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/e1000_defines.h
@@ -0,0 +1 @@
+../../../../../../../drivers/net/ethernet/intel/igb/e1000_defines.h
\ No newline at end of file
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/e1000_regs.h b/tools/testing/selftests/vfio/lib/drivers/igb/e1000_regs.h
new file mode 120000
index 000000000000..c733634171bb
--- /dev/null
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/e1000_regs.h
@@ -0,0 +1 @@
+../../../../../../../drivers/net/ethernet/intel/igb/e1000_regs.h
\ No newline at end of file
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 000000000000..339ca88b9c55
--- /dev/null
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <unistd.h>
+#include <errno.h>
+#include <stdint.h>
+#include <linux/io.h>
+#include <linux/pci_regs.h>
+#include <linux/pci_ids.h>
+#include <linux/kernel.h>
+#include <linux/compiler.h>
+#include <linux/mii.h>
+#include <libvfio/vfio_pci_device.h>
+
+#include "e1000_regs.h"
+#include "e1000_defines.h"
+#include "e1000_82575.h"
+
+#define PCI_DEVICE_ID_INTEL_82576 0x10C9
+#define IGB_MAX_CHUNK_SIZE 1024
+#define MSIX_VECTOR 0
+#define RING_SIZE 4096 /* Number of descriptors in ring */
+
+struct igb_tx_desc {
+ union {
+ struct {
+ u64 buffer_addr; /* Address of descriptor's data buffer */
+ u32 cmd_type_len; /* Command/Type/Length */
+ u32 olinfo_status; /* Context/Buffer info */
+ } read;
+
+ struct {
+ u64 rsvd; /* Reserved */
+ u32 nxtseq_seed; /* Next sequence seed */
+ u32 status; /* Descriptor status */
+ } wb;
+ };
+};
+
+struct igb_rx_desc {
+ union {
+ struct {
+ u64 pkt_addr; /* Packet buffer address */
+ u64 hdr_addr; /* Header buffer address */
+ } read;
+ struct {
+ u16 pkt_info; /* RSS type, Packet type */
+ u16 hdr_info; /* Split Head, buf len */
+ u32 rss; /* RSS Hash */
+ u32 status_error; /* ext status/error */
+ u16 length; /* Packet length */
+ u16 vlan; /* VLAN tag */
+ } wb; /* writeback */
+ };
+};
+
+struct igb {
+ void *bar0;
+ u32 tx_tail;
+ u32 rx_tail;
+ struct igb_tx_desc tx_ring[RING_SIZE] __attribute__((aligned(128)));
+ struct igb_rx_desc rx_ring[RING_SIZE] __attribute__((aligned(128)));
+};
+
+static inline struct igb *to_igb_state(struct vfio_pci_device *device)
+{
+ return (struct igb *)device->driver.region.vaddr;
+}
+
+static inline void igb_write32(struct igb *igb, u32 reg, u32 val)
+{
+ writel(val, igb->bar0 + reg);
+}
+
+static inline u32 igb_read32(struct igb *igb, u32 reg)
+{
+ return readl(igb->bar0 + reg);
+}
+
+static int igb_write_phy(struct igb *igb, u32 offset, u16 data)
+{
+ u32 mdic;
+ int i;
+
+ mdic = (((u32)data) |
+ (offset << E1000_MDIC_REG_SHIFT) |
+ (1 << E1000_MDIC_PHY_SHIFT) |
+ E1000_MDIC_OP_WRITE);
+
+ igb_write32(igb, E1000_MDIC, mdic);
+
+ for (i = 0; i < 1000; i++) {
+ usleep(50);
+ mdic = igb_read32(igb, E1000_MDIC);
+ if (mdic & E1000_MDIC_READY)
+ break;
+ }
+
+ if (!(mdic & E1000_MDIC_READY))
+ return -1;
+
+ if (mdic & E1000_MDIC_ERROR)
+ return -1;
+
+ return 0;
+}
+
+static int igb_read_phy(struct igb *igb, u32 offset, u16 *data)
+{
+ u32 mdic;
+ int i;
+
+ mdic = ((offset << E1000_MDIC_REG_SHIFT) |
+ (1 << E1000_MDIC_PHY_SHIFT) |
+ E1000_MDIC_OP_READ);
+
+ igb_write32(igb, E1000_MDIC, mdic);
+
+ for (i = 0; i < 1000; i++) {
+ usleep(50);
+ mdic = igb_read32(igb, E1000_MDIC);
+ if (mdic & E1000_MDIC_READY)
+ break;
+ }
+
+ if (!(mdic & E1000_MDIC_READY))
+ return -1;
+
+ if (mdic & E1000_MDIC_ERROR)
+ return -1;
+
+ *data = (u16)mdic;
+ return 0;
+}
+
+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");
+}
+
+static int igb_probe(struct vfio_pci_device *device)
+{
+ if (!vfio_pci_device_match(device, PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82576))
+ return -EINVAL;
+
+ return 0;
+}
+
+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_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(1000);
+ while (igb_read32(igb, E1000_CTRL) & E1000_CTRL_RST)
+ usleep(10);
+ igb_write32(igb, E1000_IMC, 0xFFFFFFFF);
+
+ /* 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);
+ }
+
+ /* Trigger autonegotiation. This enables IGB to transmit data. */
+ igb_phy_setup_autoneg(igb);
+
+ /* 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);
+ 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);
+ }
+
+ /* Enable Receiver and Transmitter */
+ rctl = E1000_RCTL_EN | /* Receiver Enable */
+ E1000_RCTL_UPE | /* Unicast Promiscuous (for dummy MAC) */
+ E1000_RCTL_LBM_MAC | /* MAC Loopback Mode */
+ E1000_RCTL_SECRC; /* Strip CRC (needed for memcmp) */
+ igb_write32(igb, E1000_RCTL, rctl);
+ igb_write32(igb, E1000_TCTL, E1000_TCTL_EN);
+
+ /* 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, E1000_GPIE, E1000_GPIE_EIAME);
+
+ /* Enable interrupts on vector 0 */
+ igb_write32(igb, E1000_EIMS, 1);
+
+ /* Map vector 0 to interrupt cause 0 and mark it valid */
+ igb_write32(igb, E1000_IVAR0, E1000_IVAR_VALID);
+
+ /* Initialize driver state and capability limits */
+ igb->tx_tail = 0;
+ igb->rx_tail = 0;
+
+ device->driver.max_memcpy_size = IGB_MAX_CHUNK_SIZE;
+ device->driver.max_memcpy_count = RING_SIZE - 1;
+ device->driver.msi = MSIX_VECTOR;
+}
+
+static void igb_remove(struct vfio_pci_device *device)
+{
+ struct igb *igb = to_igb_state(device);
+
+ vfio_pci_msix_disable(device);
+ igb_write32(igb, E1000_RCTL, 0);
+ igb_write32(igb, E1000_TCTL, 0);
+ igb_write32(igb, E1000_CTRL, igb_read32(igb, E1000_CTRL) | E1000_CTRL_RST);
+}
+
+static void igb_irq_disable(struct igb *igb)
+{
+ igb_write32(igb, E1000_EIMC, 1);
+}
+
+static void igb_irq_enable(struct igb *igb)
+{
+ igb_write32(igb, E1000_EIMS, 1);
+}
+
+static void igb_irq_clear(struct igb *igb)
+{
+ igb_read32(igb, E1000_EICR);
+}
+
+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 = cpu_to_le64(dst);
+ tx->read.buffer_addr = cpu_to_le64(src);
+ tx->read.cmd_type_len = cpu_to_le32((u32)size | E1000_TXD_CMD_EOP | E1000_TXD_CMD_IFCS);
+
+ /* Set to 0 to disable offloads and avoid needing a context descriptor */
+ tx->read.olinfo_status = cpu_to_le32(0);
+
+ igb->tx_tail = (igb->tx_tail + 1) % RING_SIZE;
+ igb->rx_tail = (igb->rx_tail + 1) % RING_SIZE;
+ }
+
+ igb_write32(igb, E1000_RDT(0), igb->rx_tail);
+ igb_write32(igb, E1000_TDT(0), igb->tx_tail);
+}
+
+static int igb_memcpy_wait(struct vfio_pci_device *device)
+{
+ struct igb *igb = to_igb_state(device);
+ struct igb_rx_desc *rx;
+ u32 status = 0;
+ 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) {
+ status = le32_to_cpu(READ_ONCE(rx->wb.status_error));
+ if (status & 1)
+ break;
+ usleep(10);
+ }
+
+ igb_irq_clear(igb);
+
+ igb_irq_enable(igb);
+
+ return (status & 1) ? 0 : -ETIMEDOUT;
+}
+
+static void igb_send_msi(struct vfio_pci_device *device)
+{
+ struct igb *igb = to_igb_state(device);
+
+ igb_write32(igb, E1000_EICS, 1);
+}
+
+const struct vfio_pci_driver_ops igb_ops = {
+ .name = "igb",
+ .probe = igb_probe,
+ .init = igb_init,
+ .remove = igb_remove,
+ .memcpy_start = igb_memcpy_start,
+ .memcpy_wait = igb_memcpy_wait,
+ .send_msi = igb_send_msi,
+};
diff --git a/tools/testing/selftests/vfio/lib/libvfio.mk b/tools/testing/selftests/vfio/lib/libvfio.mk
index 9f47bceed16f..1f13cca04348 100644
--- a/tools/testing/selftests/vfio/lib/libvfio.mk
+++ b/tools/testing/selftests/vfio/lib/libvfio.mk
@@ -12,6 +12,7 @@ LIBVFIO_C += vfio_pci_driver.c
ifeq ($(ARCH:x86_64=x86),x86)
LIBVFIO_C += drivers/ioat/ioat.c
LIBVFIO_C += drivers/dsa/dsa.c
+LIBVFIO_C += drivers/igb/igb.c
endif
LIBVFIO_OUTPUT := $(OUTPUT)/libvfio
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_driver.c b/tools/testing/selftests/vfio/lib/vfio_pci_driver.c
index 6827f4a6febe..a5d0547132c4 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_driver.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_driver.c
@@ -5,12 +5,14 @@
#ifdef __x86_64__
extern struct vfio_pci_driver_ops dsa_ops;
extern struct vfio_pci_driver_ops ioat_ops;
+extern struct vfio_pci_driver_ops igb_ops;
#endif
static struct vfio_pci_driver_ops *driver_ops[] = {
#ifdef __x86_64__
&dsa_ops,
&ioat_ops,
+ &igb_ops,
#endif
};
--
2.55.0.795.g602f6c329a-goog
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 1/9] vfio: selftests: igb: Add driver for IGB QEMU device
2026-07-08 23:18 ` [PATCH v3 1/9] vfio: selftests: igb: Add driver for IGB QEMU device Josh Hilke
@ 2026-07-08 23:31 ` sashiko-bot
2026-07-09 19:23 ` Josh Hilke
0 siblings, 1 reply; 23+ messages in thread
From: sashiko-bot @ 2026-07-08 23:31 UTC (permalink / raw)
To: Josh Hilke; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Advanced Receive Descriptor Misconfiguration (SRRCTL)
- [High] Unsupported MAC Loopback and Link State Deadlock
- [High] Interrupt Delivery Failure due to Incomplete MSI-X Configuration
- [High] Fault Recovery Failure due to PCIe Completion Timeout Resend
- [High] Inadequate Wait Timeout for Large Memcpy Batches
--
--- Patch [1]: [PATCH v3 1/9] vfio: selftests: igb: Add driver for IGB QEMU device ---
commit a7e0dd62086e602fc2886d15801884902daf6cba
Author: Josh Hilke <jrhilke@google.com>
vfio: selftests: igb: Add driver for IGB QEMU device
This patch adds a VFIO selftest driver for the Intel Gigabit Ethernet
controller (IGB) to allow running VFIO selftests in QEMU without requiring
specific hardware. It utilizes the IGB loopback mode to implement the
required memcpy operations.
> 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..339ca88b9c555
> --- /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: High]
Does this configuration work on unplugged physical hardware?
Writing to the PHY without enabling BMCR_LOOPBACK and relying on
autonegotiation will block indefinitely and time out after 1 second if no
network cable is connected.
> + 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);
[ ... ]
> +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);
[Severity: High]
Could an unmapped IOVA access permanently lock up the physical device here?
VFIO selftests intentionally submit descriptors targeting unmapped IOVAs to
test IOMMU fault handling. By default, 82576 hardware sets the
E1000_GCR_CMPL_TMOUT_RESEND bit, which instructs the DMA engine to
indefinitely retry upon PCIe Completion Timeout or Unsupported Request from
the IOMMU, flooding the system with PCIe AER errors.
Should the initialization sequence clear this bit in the E1000_GCR register
before starting DMA operations so the device can gracefully recover?
> + 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).
> + */
[ ... ]
> + /* 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);
[Severity: High]
Will the hardware use the correct descriptor format without configuring
E1000_SRRCTL?
The driver's struct igb_rx_desc expects the Advanced One-Buffer format, but
without writing E1000_SRRCTL_DESCTYPE_ADV_ONEBUF to E1000_SRRCTL during
igb_init(), the 82576 hardware defaults to the legacy format.
This causes the hardware to write the status byte to offset 4 instead of
offset 8, making igb_memcpy_wait() poll an unpopulated offset and causing
the test to fail with an indefinite timeout on physical hardware.
> + 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);
> + 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);
> + }
> +
> + /* Enable Receiver and Transmitter */
> + rctl = E1000_RCTL_EN | /* Receiver Enable */
> + E1000_RCTL_UPE | /* Unicast Promiscuous (for dummy MAC) */
> + E1000_RCTL_LBM_MAC | /* MAC Loopback Mode */
> + E1000_RCTL_SECRC; /* Strip CRC (needed for memcmp) */
> + igb_write32(igb, E1000_RCTL, rctl);
[Severity: High]
Is it expected that internal MAC loopback is not actually implemented on
real 82576 hardware?
The datasheet (Section 3.5.6.2) indicates E1000_RCTL_LBM_MAC has no physical
effect on the 82576. Additionally, the MAC link is never forced up via
E1000_CTRL_SLU here. This will stall the descriptor engine when running on
real hardware without a live network connection.
> + igb_write32(igb, E1000_TCTL, E1000_TCTL_EN);
> +
> + /* 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, E1000_GPIE, E1000_GPIE_EIAME);
> +
> + /* Enable interrupts on vector 0 */
> + igb_write32(igb, E1000_EIMS, 1);
> +
> + /* Map vector 0 to interrupt cause 0 and mark it valid */
> + igb_write32(igb, E1000_IVAR0, E1000_IVAR_VALID);
[Severity: High]
Will the hardware successfully deliver MSI-X interrupts with this configuration?
The RX queue is mapped to MSI-X vector 0 using E1000_IVAR0, but the
E1000_GPIE_MSIX_MODE bit isn't set in E1000_GPIE. This leaves the 82576
operating in legacy interrupt mode, causing it to ignore IVAR routing.
Also, while E1000_GPIE_EIAME (Auto-Mask Enable) is set, the E1000_EIAM
and E1000_EIAC registers appear to be left unprogrammed.
> +
> + /* Initialize driver state and capability limits */
> + igb->tx_tail = 0;
[ ... ]
> +static int igb_memcpy_wait(struct vfio_pci_device *device)
> +{
> + struct igb *igb = to_igb_state(device);
> + struct igb_rx_desc *rx;
> + u32 status = 0;
> + 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) {
> + status = le32_to_cpu(READ_ONCE(rx->wb.status_error));
> + if (status & 1)
> + break;
> + usleep(10);
> + }
[Severity: High]
Is a 1 millisecond total wait timeout sufficient for full-size batch copies?
This lockless polling loop executes 100 iterations of 10 microseconds each,
yielding a strict 1ms timeout limit. However, the driver initializes
max_memcpy_count to 4095 and max_memcpy_size to 1024 bytes. Transmitting
~4 MB of data across a 1 Gbps loopback interface physically requires at least
~33.5 milliseconds, which will cause large memcpy batches to consistently
time out before completion.
> +
> + igb_irq_clear(igb);
> +
> + igb_irq_enable(igb);
> +
> + return (status & 1) ? 0 : -ETIMEDOUT;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-igb_v3_b4-v3-0-1272650ca614@google.com?part=1
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 1/9] vfio: selftests: igb: Add driver for IGB QEMU device
2026-07-08 23:31 ` sashiko-bot
@ 2026-07-09 19:23 ` Josh Hilke
0 siblings, 0 replies; 23+ messages in thread
From: Josh Hilke @ 2026-07-09 19:23 UTC (permalink / raw)
To: sashiko-reviews; +Cc: kvm
On Wed, Jul 8, 2026 at 4:31 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Advanced Receive Descriptor Misconfiguration (SRRCTL)
> - [High] Unsupported MAC Loopback and Link State Deadlock
> - [High] Interrupt Delivery Failure due to Incomplete MSI-X Configuration
> - [High] Fault Recovery Failure due to PCIe Completion Timeout Resend
> - [High] Inadequate Wait Timeout for Large Memcpy Batches
It's worth noting most of these issues are specific to the physical
hardware. This first patch intends only to do the bare minimum to get
the driver tests to pass on QEMU IGB. Alex's patches add the necessary
support for the physical device. I think keeping those patches
separate makes it easy to see the differences between the QEMU device
and the physical device. I'll add comments for each issue below
stating which patch addresses it.
I'll also update the commit message to clarify that this patch does
not support the physical hardware.
>
> --- Patch [1]: [PATCH v3 1/9] vfio: selftests: igb: Add driver for IGB QEMU device ---
> commit a7e0dd62086e602fc2886d15801884902daf6cba
> Author: Josh Hilke <jrhilke@google.com>
>
> vfio: selftests: igb: Add driver for IGB QEMU device
>
> This patch adds a VFIO selftest driver for the Intel Gigabit Ethernet
> controller (IGB) to allow running VFIO selftests in QEMU without requiring
> specific hardware. It utilizes the IGB loopback mode to implement the
> required memcpy operations.
>
> > 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..339ca88b9c555
> > --- /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: High]
> Does this configuration work on unplugged physical hardware?
>
> Writing to the PHY without enabling BMCR_LOOPBACK and relying on
> autonegotiation will block indefinitely and time out after 1 second if no
> network cable is connected.
Fixed in patch 2 which sets up loopback for physical hardware.
>
> > + 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);
>
> [ ... ]
>
> > +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);
>
> [Severity: High]
> Could an unmapped IOVA access permanently lock up the physical device here?
>
> VFIO selftests intentionally submit descriptors targeting unmapped IOVAs to
> test IOMMU fault handling. By default, 82576 hardware sets the
> E1000_GCR_CMPL_TMOUT_RESEND bit, which instructs the DMA engine to
> indefinitely retry upon PCIe Completion Timeout or Unsupported Request from
> the IOMMU, flooding the system with PCIe AER errors.
>
> Should the initialization sequence clear this bit in the E1000_GCR register
> before starting DMA operations so the device can gracefully recover?
Fixed in patch 6, which clears the GCR retry bit, and patch 9 which
implements FLR recovery.
>
> > + 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).
> > + */
>
> [ ... ]
>
> > + /* 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);
>
> [Severity: High]
> Will the hardware use the correct descriptor format without configuring
> E1000_SRRCTL?
>
> The driver's struct igb_rx_desc expects the Advanced One-Buffer format, but
> without writing E1000_SRRCTL_DESCTYPE_ADV_ONEBUF to E1000_SRRCTL during
> igb_init(), the 82576 hardware defaults to the legacy format.
>
> This causes the hardware to write the status byte to offset 4 instead of
> offset 8, making igb_memcpy_wait() poll an unpopulated offset and causing
> the test to fail with an indefinite timeout on physical hardware.
Fixed in patch 3 which implements advanced TX and RX descriptors.
> > + 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);
> > + 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);
> > + }
> > +
> > + /* Enable Receiver and Transmitter */
> > + rctl = E1000_RCTL_EN | /* Receiver Enable */
> > + E1000_RCTL_UPE | /* Unicast Promiscuous (for dummy MAC) */
> > + E1000_RCTL_LBM_MAC | /* MAC Loopback Mode */
> > + E1000_RCTL_SECRC; /* Strip CRC (needed for memcmp) */
> > + igb_write32(igb, E1000_RCTL, rctl);
>
> [Severity: High]
> Is it expected that internal MAC loopback is not actually implemented on
> real 82576 hardware?
>
> The datasheet (Section 3.5.6.2) indicates E1000_RCTL_LBM_MAC has no physical
> effect on the 82576. Additionally, the MAC link is never forced up via
> E1000_CTRL_SLU here. This will stall the descriptor engine when running on
> real hardware without a live network connection.
Fixed in patch 2 which uses PHY loopback instead of MAC.
>
> > + igb_write32(igb, E1000_TCTL, E1000_TCTL_EN);
> > +
> > + /* 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, E1000_GPIE, E1000_GPIE_EIAME);
> > +
> > + /* Enable interrupts on vector 0 */
> > + igb_write32(igb, E1000_EIMS, 1);
> > +
> > + /* Map vector 0 to interrupt cause 0 and mark it valid */
> > + igb_write32(igb, E1000_IVAR0, E1000_IVAR_VALID);
>
> [Severity: High]
> Will the hardware successfully deliver MSI-X interrupts with this configuration?
>
> The RX queue is mapped to MSI-X vector 0 using E1000_IVAR0, but the
> E1000_GPIE_MSIX_MODE bit isn't set in E1000_GPIE. This leaves the 82576
> operating in legacy interrupt mode, causing it to ignore IVAR routing.
>
> Also, while E1000_GPIE_EIAME (Auto-Mask Enable) is set, the E1000_EIAM
> and E1000_EIAC registers appear to be left unprogrammed.
E1000_GPIE_MSIX_MODE does indeed need to be set in order for the
driver tests to pass in QEMU (the mix_and_match test fails without
it). Patch 4 fixes the MSI-X configuration for the physical hardware.
> > +
> > + /* Initialize driver state and capability limits */
> > + igb->tx_tail = 0;
>
> [ ... ]
>
> > +static int igb_memcpy_wait(struct vfio_pci_device *device)
> > +{
> > + struct igb *igb = to_igb_state(device);
> > + struct igb_rx_desc *rx;
> > + u32 status = 0;
> > + 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) {
> > + status = le32_to_cpu(READ_ONCE(rx->wb.status_error));
> > + if (status & 1)
> > + break;
> > + usleep(10);
> > + }
>
> [Severity: High]
> Is a 1 millisecond total wait timeout sufficient for full-size batch copies?
>
> This lockless polling loop executes 100 iterations of 10 microseconds each,
> yielding a strict 1ms timeout limit. However, the driver initializes
> max_memcpy_count to 4095 and max_memcpy_size to 1024 bytes. Transmitting
> ~4 MB of data across a 1 Gbps loopback interface physically requires at least
> ~33.5 milliseconds, which will cause large memcpy batches to consistently
> time out before completion.
Fixed in patch 5 which extends the timeout for the physical hardware's
line rate.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 2/9] vfio: selftests: igb: Use PHY internal loopback on 82576
2026-07-08 23:18 [PATCH v3 0/9] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
2026-07-08 23:18 ` [PATCH v3 1/9] vfio: selftests: igb: Add driver for IGB QEMU device Josh Hilke
@ 2026-07-08 23:18 ` Josh Hilke
2026-07-08 23:28 ` sashiko-bot
2026-07-08 23:18 ` [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors Josh Hilke
` (6 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Josh Hilke @ 2026-07-08 23:18 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Shuah Khan, linux-kernel, kvm, linux-kselftest, Vipin Sharma,
Josh Hilke, Alex Williamson
From: Alex Williamson <alex.williamson@nvidia.com>
The submitted driver waits for PHY autonegotiation and then enables MAC
loopback via RCTL.LBM_MAC. QEMU's emulated igb tolerates this, but the
82576 datasheet rejects the MAC loopback path on real hardware:
Section 3.5.6.1 (Loopback Support / General): "Use PHY Loopback
instead of MAC Loopback on the 82576."
Section 3.5.6.2 (MAC Loopback): "MAC Loopback is not used on this
device."
Section 3.5.6.3.1 (Setting the 82576 to PHY loopback Mode): set PHY
control register bits 8 (duplex), 14 (loopback), clear bit 12
(autoneg enable), set the speed via bits 6 and 13. For 1Gb/s the
register value is 0x4140.
Section 8.10.1 (RCTL register): "When using the internal PHY, LBM
should remain set to 00b and the PHY instead configured for
loopback through the MDIO interface."
Replace igb_phy_setup_autoneg() with igb_setup_loopback() which:
- writes PHY register 0 with LOOPBACK | SPEED_1000 | FULL_DUPLEX
- forces the MAC into 1Gb/s full duplex via CTRL.FRCSPD, CTRL.FRCDPX,
CTRL.SPD_1000, CTRL.FD, CTRL.SLU; without forcing the MAC link
state, the descriptor engine does not run on real hardware
PHY internal loopback (section 3.5.6.3) wraps data at the end of the
PHY datapath before the MDI, so the physical link state and cable
speed are irrelevant. This matches the kernel ethtool selftest path
in igb_integrated_phy_loopback()
(drivers/net/ethernet/intel/igb/igb_ethtool.c).
QEMU's igb emulation drives STATUS.LU exclusively from its autoneg-done
timer or a network-backend link-state change; the guest cannot set
STATUS.LU through CTRL.SLU. Its receive path checks STATUS.LU
(e1000x_hw_rx_enabled in hw/net/e1000x_common.c) and drops every
loopback frame until LU is set. Issue a one-shot autoneg-restart PHY
write at the top of igb_setup_loopback() to kick the timer; the
subsequent PHY write clears autoneg-enable, so on real hardware
autoneg never starts and the write is a no-op.
QEMU's igb also 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 advises that LBM remain 00b when
using the internal PHY, but empirically setting LBM_MAC has no
observable effect on real 82576 (MAC loopback is not implemented per
3.5.6.2), so set it alongside PHY loopback as the actual loopback
mechanism under QEMU. With these two QEMU-only accommodations the
selftest works in both environments without environment-specific code
paths.
The submitted driver also wrote bit 14 (link disable) to PHY register
16 (port control). Datasheet 3.5.6.3.1 describes this as required for
10/100Mb/s but explicitly "not a must for 1G", and the kernel ethtool
selftest omits it. It is dropped here. Flagging the removal in case a
future regression bisects to this commit.
Remove igb_read_phy() and the PHY status macros it served, which become
unused: IGB_PHY_STATUS_REG_OFFSET, IGB_PHY_STATUS_AN_COMP. Keep
IGB_PHY_CTRL_AN_ENABLE and IGB_PHY_CTRL_AN_RESTART for the QEMU-only
autoneg kick; add the PHY ctrl loopback and CTRL force-link macros.
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
tools/testing/selftests/vfio/lib/drivers/igb/igb.c | 122 +++++++++++++--------
1 file changed, 74 insertions(+), 48 deletions(-)
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index 339ca88b9c55..58c6aceea63a 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -103,57 +103,70 @@ static int igb_write_phy(struct igb *igb, u32 offset, u16 data)
return 0;
}
-static int igb_read_phy(struct igb *igb, u32 offset, u16 *data)
+/*
+ * Configure the device for PHY internal loopback per 82576 datasheet
+ * section 3.5.6.3.1. Force the PHY to 1Gb/s full duplex with loopback
+ * enabled, then force the MAC link state to match. Internal loopback
+ * wraps data at the end of the PHY datapath (section 3.5.6.3), so the
+ * physical link state is irrelevant.
+ *
+ * Section 3.5.6.1 directs to "Use PHY Loopback instead of MAC Loopback
+ * on the 82576", and section 3.5.6.2 states "MAC Loopback is not used
+ * on this device." RCTL.LBM_MAC is still set elsewhere as a QEMU-only
+ * accommodation; see the RCTL programming in the caller for the
+ * rationale.
+ */
+static void igb_setup_loopback(struct igb *igb)
{
- u32 mdic;
- int i;
-
- mdic = ((offset << E1000_MDIC_REG_SHIFT) |
- (1 << E1000_MDIC_PHY_SHIFT) |
- E1000_MDIC_OP_READ);
-
- igb_write32(igb, E1000_MDIC, mdic);
-
- for (i = 0; i < 1000; i++) {
- usleep(50);
- mdic = igb_read32(igb, E1000_MDIC);
- if (mdic & E1000_MDIC_READY)
- break;
- }
-
- if (!(mdic & E1000_MDIC_READY))
- return -1;
-
- if (mdic & E1000_MDIC_ERROR)
- return -1;
-
- *data = (u16)mdic;
- return 0;
-}
-
-static void igb_phy_setup_autoneg(struct igb *igb)
-{
- int timeout_ms = 1000;
- bool success = false;
- u16 phy_status;
+ u32 ctrl;
int ret;
- int i;
- /* Trigger auto-negotiation */
- ret = igb_write_phy(igb, MII_BMCR,
+ /*
+ * Kick the autoneg machinery solely to bring STATUS.LU up under
+ * QEMU's igb emulation: QEMU only updates STATUS.LU via its
+ * autoneg-done timer, and without LU set its receive path
+ * (e1000x_hw_rx_enabled) drops every loopback frame. On real
+ * hardware autoneg cannot complete before the next PHY write
+ * below clears the autoneg-enable bit, so this is effectively a
+ * no-op there.
+ */
+ (void)igb_write_phy(igb, MII_BMCR,
BMCR_ANENABLE | BMCR_ANRESTART);
+
+ /* PHY control: loopback + 1Gb/s full duplex, autoneg disabled. */
+ ret = igb_write_phy(igb, MII_BMCR,
+ BMCR_LOOPBACK |
+ BMCR_SPEED1000 |
+ BMCR_FULLDPLX);
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);
- }
+ /*
+ * Brief delay before forcing the MAC, mirroring the kernel ethtool
+ * selftest in igb_integrated_phy_loopback(). Not specified by the
+ * datasheet, but empirically required by the kernel driver.
+ */
+ usleep(50000);
+
+ /*
+ * Force the MAC to 1Gb/s full duplex with link up. Without forcing
+ * the link state the descriptor engine does not run, since the chip
+ * normally waits for a real negotiated link.
+ */
+ ctrl = igb_read32(igb, E1000_CTRL);
+ ctrl &= ~E1000_CTRL_SPD_SEL;
+ ctrl |= E1000_CTRL_FRCSPD |
+ E1000_CTRL_FRCDPX |
+ E1000_CTRL_SPD_1000 |
+ E1000_CTRL_FD |
+ E1000_CTRL_SLU;
+ igb_write32(igb, E1000_CTRL, ctrl);
- VFIO_ASSERT_TRUE(success, "Auto-negotiation did not complete in time");
+ /*
+ * Settling delay matching the kernel ethtool selftest's msleep(500)
+ * at the tail of igb_integrated_phy_loopback(). Not specified by
+ * the datasheet; empirical, and inherited from the kernel driver.
+ */
+ usleep(500000);
}
static int igb_probe(struct vfio_pci_device *device)
@@ -180,6 +193,7 @@ static void igb_init(struct vfio_pci_device *device)
iova_tx = to_iova(device, igb->tx_ring);
iova_rx = to_iova(device, igb->rx_ring);
+ /* Reset device and disable all interrupts */
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
@@ -205,8 +219,8 @@ static void igb_init(struct vfio_pci_device *device)
vfio_pci_config_writew(device, PCI_COMMAND, cmd_reg);
}
- /* Trigger autonegotiation. This enables IGB to transmit data. */
- igb_phy_setup_autoneg(igb);
+ /* Configure PHY internal loopback for testing. */
+ igb_setup_loopback(igb);
/* Configure TX and RX descriptor rings */
igb_write32(igb, E1000_TDBAL(0), (u32)iova_tx);
@@ -232,10 +246,22 @@ static void igb_init(struct vfio_pci_device *device)
usleep(10);
}
- /* Enable Receiver and Transmitter */
+ /*
+ * 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_LBM_MAC | /* MAC Loopback 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);
--
2.55.0.795.g602f6c329a-goog
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 2/9] vfio: selftests: igb: Use PHY internal loopback on 82576
2026-07-08 23:18 ` [PATCH v3 2/9] vfio: selftests: igb: Use PHY internal loopback on 82576 Josh Hilke
@ 2026-07-08 23:28 ` sashiko-bot
2026-07-09 19:27 ` Josh Hilke
0 siblings, 1 reply; 23+ messages in thread
From: sashiko-bot @ 2026-07-08 23:28 UTC (permalink / raw)
To: Josh Hilke; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message incorrectly references removing and keeping macros and code that do not exist in the baseline or the patch diff.
--
--- Patch [2]: [PATCH v3 2/9] vfio: selftests: igb: Use PHY internal loopback on 82576 ---
commit 2d54b65042a5cbe6702c330df52d94e7c2ff3b57
Author: Alex Williamson <alex.williamson@nvidia.com>
vfio: selftests: igb: Use PHY internal loopback on 82576
This commit replaces igb_phy_setup_autoneg() with igb_setup_loopback() to use
PHY internal loopback on the 82576 device instead of MAC loopback, aligning
with the datasheet requirements for real hardware. It forces the MAC into 1Gb/s
full duplex with link up to ensure the descriptor engine runs. The patch also
maintains compatibility with QEMU's emulated igb behavior without needing
environment-specific code paths.
> The submitted driver also wrote bit 14 (link disable) to PHY register
> 16 (port control). Datasheet 3.5.6.3.1 describes this as required for
> 10/100Mb/s but explicitly "not a must for 1G", and the kernel ethtool
> selftest omits it. It is dropped here. Flagging the removal in case a
> future regression bisects to this commit.
>
> Remove igb_read_phy() and the PHY status macros it served, which become
> unused: IGB_PHY_STATUS_REG_OFFSET, IGB_PHY_STATUS_AN_COMP. Keep
> IGB_PHY_CTRL_AN_ENABLE and IGB_PHY_CTRL_AN_RESTART for the QEMU-only
> autoneg kick; add the PHY ctrl loopback and CTRL force-link macros.
[Severity: Medium]
This isn't a bug in the code, but the commit message references removing and
keeping macros and code that do not exist in the baseline or the patch diff.
Looking at the baseline code being modified, igb_phy_setup_autoneg() did not
write to PHY register 16. It also used standard macros like MII_BMCR and
BMSR_ANEGCOMPLETE rather than IGB_PHY_STATUS_REG_OFFSET,
IGB_PHY_STATUS_AN_COMP, or IGB_PHY_CTRL_AN_ENABLE.
Could this part of the commit message be a leftover from an earlier version
of the patch series?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-igb_v3_b4-v3-0-1272650ca614@google.com?part=2
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/9] vfio: selftests: igb: Use PHY internal loopback on 82576
2026-07-08 23:28 ` sashiko-bot
@ 2026-07-09 19:27 ` Josh Hilke
0 siblings, 0 replies; 23+ messages in thread
From: Josh Hilke @ 2026-07-09 19:27 UTC (permalink / raw)
To: sashiko-reviews; +Cc: kvm
On Wed, Jul 8, 2026 at 4:28 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The commit message incorrectly references removing and keeping macros and code that do not exist in the baseline or the patch diff.
> --
>
> --- Patch [2]: [PATCH v3 2/9] vfio: selftests: igb: Use PHY internal loopback on 82576 ---
> commit 2d54b65042a5cbe6702c330df52d94e7c2ff3b57
> Author: Alex Williamson <alex.williamson@nvidia.com>
>
> vfio: selftests: igb: Use PHY internal loopback on 82576
>
> This commit replaces igb_phy_setup_autoneg() with igb_setup_loopback() to use
> PHY internal loopback on the 82576 device instead of MAC loopback, aligning
> with the datasheet requirements for real hardware. It forces the MAC into 1Gb/s
> full duplex with link up to ensure the descriptor engine runs. The patch also
> maintains compatibility with QEMU's emulated igb behavior without needing
> environment-specific code paths.
>
> > The submitted driver also wrote bit 14 (link disable) to PHY register
> > 16 (port control). Datasheet 3.5.6.3.1 describes this as required for
> > 10/100Mb/s but explicitly "not a must for 1G", and the kernel ethtool
> > selftest omits it. It is dropped here. Flagging the removal in case a
> > future regression bisects to this commit.
> >
> > Remove igb_read_phy() and the PHY status macros it served, which become
> > unused: IGB_PHY_STATUS_REG_OFFSET, IGB_PHY_STATUS_AN_COMP. Keep
> > IGB_PHY_CTRL_AN_ENABLE and IGB_PHY_CTRL_AN_RESTART for the QEMU-only
> > autoneg kick; add the PHY ctrl loopback and CTRL force-link macros.
>
> [Severity: Medium]
> This isn't a bug in the code, but the commit message references removing and
> keeping macros and code that do not exist in the baseline or the patch diff.
>
> Looking at the baseline code being modified, igb_phy_setup_autoneg() did not
> write to PHY register 16. It also used standard macros like MII_BMCR and
> BMSR_ANEGCOMPLETE rather than IGB_PHY_STATUS_REG_OFFSET,
> IGB_PHY_STATUS_AN_COMP, or IGB_PHY_CTRL_AN_ENABLE.
>
> Could this part of the commit message be a leftover from an earlier version
> of the patch series?
Sashiko is correct here. In v3, I updated the first patch in the
series to import the real IGB headers, so the macros are now
different. I'll update the commit message accordingly for v4.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors
2026-07-08 23:18 [PATCH v3 0/9] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
2026-07-08 23:18 ` [PATCH v3 1/9] vfio: selftests: igb: Add driver for IGB QEMU device Josh Hilke
2026-07-08 23:18 ` [PATCH v3 2/9] vfio: selftests: igb: Use PHY internal loopback on 82576 Josh Hilke
@ 2026-07-08 23:18 ` Josh Hilke
2026-07-08 23:29 ` sashiko-bot
2026-07-08 23:18 ` [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing Josh Hilke
` (5 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Josh Hilke @ 2026-07-08 23:18 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Shuah Khan, linux-kernel, kvm, linux-kselftest, Vipin Sharma,
Josh Hilke, Alex Williamson
From: Alex Williamson <alex.williamson@nvidia.com>
The submitted driver builds a partial legacy TX descriptor (just
DTALEN | CMD_EOP) and never programs SRRCTL.DESCTYPE. QEMU's emulated
igb tolerates this by treating descriptors as advanced regardless of
DESCTYPE, but real 82576 hardware does not.
For receive, 82576 datasheet section 7.1.5.2 states: "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, so the test polls rx.wb.status_error,
which is only written in that layout. Section 8.10.2 places DESCTYPE
in SRRCTL bits 27:25; program it with 001b (advanced one-buffer).
For transmit, datasheet section 7.2.2.3 describes the advanced data
descriptor with DEXT (DCMD bit 5) marking the descriptor as advanced,
DTYP=0011b selecting the data descriptor, IFCS (DCMD bit 1) asking the
MAC to append the Ethernet FCS (without it the frame is dropped as
malformed), EOP (DCMD bit 0) marking end of packet, and PAYLEN in
olinfo_status[31:14] carrying the total payload size. Build this
descriptor in igb_memcpy_start().
Remove the legacy CMD macros (IGB_TXD_CMD_EOP, IGB_TXD_CMD_IFCS,
IGB_TXD_CMD_RS, IGB_TXD_CMD_SHIFT, IGB_TXD_CMD_LEGACY_FORMAT) that
become unused, and add the SRRCTL register offset, DESCTYPE encoding,
and advanced TX descriptor field macros.
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
tools/testing/selftests/vfio/lib/drivers/igb/igb.c | 37 +++++++++++++++++++---
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index 58c6aceea63a..f4f6d2ee262d 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -235,6 +235,17 @@ static void igb_init(struct vfio_pci_device *device)
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].
+ */
+ 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 */
@@ -330,11 +341,29 @@ static void igb_memcpy_start(struct vfio_pci_device *device, iova_t src,
memset(rx, 0, sizeof(struct igb_rx_desc));
rx->read.pkt_addr = cpu_to_le64(dst);
- tx->read.buffer_addr = cpu_to_le64(src);
- tx->read.cmd_type_len = cpu_to_le32((u32)size | E1000_TXD_CMD_EOP | E1000_TXD_CMD_IFCS);
+ rx->read.hdr_addr = cpu_to_le64(0);
- /* Set to 0 to disable offloads and avoid needing a context descriptor */
- tx->read.olinfo_status = cpu_to_le32(0);
+ tx->read.buffer_addr = cpu_to_le64(src);
+ /*
+ * Build an advanced data descriptor per 82576 datasheet
+ * section 7.2.2.3. DEXT marks the descriptor as advanced
+ * (required by hardware); DTYP=data selects the data
+ * descriptor; IFCS asks the MAC to append the Ethernet
+ * FCS (without it the frame is dropped as malformed);
+ * EOP marks end of packet. DTALEN is the buffer length
+ * in bits 15:0 of cmd_type_len.
+ */
+ tx->read.cmd_type_len = cpu_to_le32((uint32_t)size |
+ E1000_ADVTXD_DTYP_DATA |
+ E1000_ADVTXD_DCMD_DEXT |
+ E1000_ADVTXD_DCMD_IFCS |
+ E1000_ADVTXD_DCMD_EOP);
+ /*
+ * PAYLEN (section 7.2.2.3.11) is the total payload size
+ * in olinfo_status[31:14].
+ */
+ tx->read.olinfo_status =
+ cpu_to_le32((uint32_t)size << E1000_ADVTXD_PAYLEN_SHIFT);
igb->tx_tail = (igb->tx_tail + 1) % RING_SIZE;
igb->rx_tail = (igb->rx_tail + 1) % RING_SIZE;
--
2.55.0.795.g602f6c329a-goog
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors
2026-07-08 23:18 ` [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors Josh Hilke
@ 2026-07-08 23:29 ` sashiko-bot
2026-07-09 20:43 ` Josh Hilke
2026-07-09 21:33 ` Alex Williamson
0 siblings, 2 replies; 23+ messages in thread
From: sashiko-bot @ 2026-07-08 23:29 UTC (permalink / raw)
To: Josh Hilke; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Programming `SRRCTL` with `E1000_SRRCTL_DESCTYPE_ADV_ONEBUF` overwrites the register, clearing the `BSIZEPACKET` (Receive Buffer Size) field to 0, which violates the 82576 datasheet.
- [Medium] The commit message incorrectly claims to add new header macros and remove legacy CMD macros, but the patch contains no such header modifications.
--
--- Patch [3]: [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors ---
commit 44f58d3e0349aa95894e8741ae9ff09ee319dcae
Author: Alex Williamson <alex.williamson@nvidia.com>
vfio: selftests: igb: Use advanced TX and RX descriptors
The submitted driver builds a partial legacy TX descriptor (just
DTALEN | CMD_EOP) and never programs SRRCTL.DESCTYPE. QEMU's emulated
igb tolerates this by treating descriptors as advanced regardless of
DESCTYPE, but real 82576 hardware does not.
[ ... ]
> Remove the legacy CMD macros (IGB_TXD_CMD_EOP, IGB_TXD_CMD_IFCS,
> IGB_TXD_CMD_RS, IGB_TXD_CMD_SHIFT, IGB_TXD_CMD_LEGACY_FORMAT) that
> become unused, and add the SRRCTL register offset, DESCTYPE encoding,
> and advanced TX descriptor field macros.
[Severity: Medium]
Does the patch actually include these macro changes?
The commit message claims to remove legacy CMD macros and add new ones, but
there are no header modifications in the diff.
> diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> index 58c6aceea63a9..f4f6d2ee262da 100644
> --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> @@ -235,6 +235,17 @@ static void igb_init(struct vfio_pci_device *device)
> 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].
> + */
> + igb_write32(igb, E1000_SRRCTL(0), E1000_SRRCTL_DESCTYPE_ADV_ONEBUF);
[Severity: Medium]
Will this overwrite the SRRCTL register and clear the BSIZEPACKET field to 0?
By clearing BSIZEPACKET (bits 6:0 of SRRCTL), real 82576 hardware may see a
0-byte receive buffer and truncate or drop received packets, causing the test
to fail on hardware.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-igb_v3_b4-v3-0-1272650ca614@google.com?part=3
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors
2026-07-08 23:29 ` sashiko-bot
@ 2026-07-09 20:43 ` Josh Hilke
2026-07-09 21:33 ` Alex Williamson
1 sibling, 0 replies; 23+ messages in thread
From: Josh Hilke @ 2026-07-09 20:43 UTC (permalink / raw)
To: sashiko-reviews; +Cc: kvm
On Wed, Jul 8, 2026 at 4:29 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Programming `SRRCTL` with `E1000_SRRCTL_DESCTYPE_ADV_ONEBUF` overwrites the register, clearing the `BSIZEPACKET` (Receive Buffer Size) field to 0, which violates the 82576 datasheet.
> - [Medium] The commit message incorrectly claims to add new header macros and remove legacy CMD macros, but the patch contains no such header modifications.
> --
>
> --- Patch [3]: [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors ---
> commit 44f58d3e0349aa95894e8741ae9ff09ee319dcae
> Author: Alex Williamson <alex.williamson@nvidia.com>
>
> vfio: selftests: igb: Use advanced TX and RX descriptors
>
> The submitted driver builds a partial legacy TX descriptor (just
> DTALEN | CMD_EOP) and never programs SRRCTL.DESCTYPE. QEMU's emulated
> igb tolerates this by treating descriptors as advanced regardless of
> DESCTYPE, but real 82576 hardware does not.
>
> [ ... ]
>
> > Remove the legacy CMD macros (IGB_TXD_CMD_EOP, IGB_TXD_CMD_IFCS,
> > IGB_TXD_CMD_RS, IGB_TXD_CMD_SHIFT, IGB_TXD_CMD_LEGACY_FORMAT) that
> > become unused, and add the SRRCTL register offset, DESCTYPE encoding,
> > and advanced TX descriptor field macros.
>
> [Severity: Medium]
> Does the patch actually include these macro changes?
>
> The commit message claims to remove legacy CMD macros and add new ones, but
> there are no header modifications in the diff.
I'll update the commit message. I removed those macros from the series
when I added the real IGB headers to the first patch of this version.
>
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > index 58c6aceea63a9..f4f6d2ee262da 100644
> > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > @@ -235,6 +235,17 @@ static void igb_init(struct vfio_pci_device *device)
> > 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].
> > + */
> > + igb_write32(igb, E1000_SRRCTL(0), E1000_SRRCTL_DESCTYPE_ADV_ONEBUF);
>
> [Severity: Medium]
> Will this overwrite the SRRCTL register and clear the BSIZEPACKET field to 0?
>
> By clearing BSIZEPACKET (bits 6:0 of SRRCTL), real 82576 hardware may see a
> 0-byte receive buffer and truncate or drop received packets, causing the test
> to fail on hardware.
Alex already addressed this here:
https://lore.kernel.org/kvm/20260529142439.5bd03e5f@nvidia.com/
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260708-igb_v3_b4-v3-0-1272650ca614@google.com?part=3
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors
2026-07-08 23:29 ` sashiko-bot
2026-07-09 20:43 ` Josh Hilke
@ 2026-07-09 21:33 ` Alex Williamson
1 sibling, 0 replies; 23+ messages in thread
From: Alex Williamson @ 2026-07-09 21:33 UTC (permalink / raw)
To: sashiko-bot; +Cc: sashiko-reviews, Josh Hilke, kvm, alex
On Wed, 08 Jul 2026 23:29:25 +0000
sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Programming `SRRCTL` with `E1000_SRRCTL_DESCTYPE_ADV_ONEBUF` overwrites the register, clearing the `BSIZEPACKET` (Receive Buffer Size) field to 0, which violates the 82576 datasheet.
> - [Medium] The commit message incorrectly claims to add new header macros and remove legacy CMD macros, but the patch contains no such header modifications.
> --
>
> --- Patch [3]: [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors ---
> commit 44f58d3e0349aa95894e8741ae9ff09ee319dcae
> Author: Alex Williamson <alex.williamson@nvidia.com>
>
> vfio: selftests: igb: Use advanced TX and RX descriptors
>
> The submitted driver builds a partial legacy TX descriptor (just
> DTALEN | CMD_EOP) and never programs SRRCTL.DESCTYPE. QEMU's emulated
> igb tolerates this by treating descriptors as advanced regardless of
> DESCTYPE, but real 82576 hardware does not.
>
> [ ... ]
>
> > Remove the legacy CMD macros (IGB_TXD_CMD_EOP, IGB_TXD_CMD_IFCS,
> > IGB_TXD_CMD_RS, IGB_TXD_CMD_SHIFT, IGB_TXD_CMD_LEGACY_FORMAT) that
> > become unused, and add the SRRCTL register offset, DESCTYPE encoding,
> > and advanced TX descriptor field macros.
>
> [Severity: Medium]
> Does the patch actually include these macro changes?
>
> The commit message claims to remove legacy CMD macros and add new ones, but
> there are no header modifications in the diff.
Entire paragraph is stale after including E1000 headers, drop.
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > index 58c6aceea63a9..f4f6d2ee262da 100644
> > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > @@ -235,6 +235,17 @@ static void igb_init(struct vfio_pci_device *device)
> > 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].
> > + */
> > + igb_write32(igb, E1000_SRRCTL(0), E1000_SRRCTL_DESCTYPE_ADV_ONEBUF);
>
> [Severity: Medium]
> Will this overwrite the SRRCTL register and clear the BSIZEPACKET field to 0?
>
> By clearing BSIZEPACKET (bits 6:0 of SRRCTL), real 82576 hardware may see a
> 0-byte receive buffer and truncate or drop received packets, causing the test
> to fail on hardware.
Same false positive as flagged and rebutted[1] last time. If we
extrapolate that a human reviewer might have the same confusion, it
might be worth expanding this comment to include:
+ *
+ * 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.
Thanks,
Alex
[1]https://lore.kernel.org/all/20260529142439.5bd03e5f@nvidia.com/
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing
2026-07-08 23:18 [PATCH v3 0/9] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
` (2 preceding siblings ...)
2026-07-08 23:18 ` [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors Josh Hilke
@ 2026-07-08 23:18 ` Josh Hilke
2026-07-08 23:33 ` sashiko-bot
2026-07-08 23:18 ` [PATCH v3 5/9] vfio: selftests: igb: Extend memcpy completion timeout for line-rate hardware Josh Hilke
` (4 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Josh Hilke @ 2026-07-08 23:18 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Shuah Khan, linux-kernel, kvm, linux-kselftest, Vipin Sharma,
Josh Hilke, Alex Williamson
From: Alex Williamson <alex.williamson@nvidia.com>
The submitted driver writes only GPIE.EIAME (with a register value of
0x10, which is actually GPIE.Multiple_MSIX, bit 4) and clears EICR by
reading it. On QEMU this works because the emulated loopback path is
synchronous and EICR is implemented as read-to-clear unconditionally.
Real 82576 hardware needs the full MSI-X programming sequence.
Per 82576 datasheet section 7.3.2.11 Table 7-47, MSI-X mode requires:
GPIE.Multiple_MSIX (bit 4): route causes through IVAR.
GPIE.EIAME (bit 30): apply EIAM on MSI-X assertion. Without EIAME,
section 7.3.2.11 specifies EIAM only takes effect on EICR
read/write, which is not the path used here.
Configure auto-clear and auto-mask for vector 0:
EIAC (section 8.8.5): auto-clear of EICR cause bit on MSI-X assertion.
EIAM (section 8.8.6): with EIAME set, auto-mask of EIMS on MSI-X
assertion. This guarantees one interrupt per memcpy batch and
prevents repeat delivery if the cause re-asserts before EIMS is
restored.
Replace the read-to-clear of EICR with write-to-clear. Section 8.8.5
states "If any bits are set in EIAC, the EICR register should not be
read", and section 7.3.4.3 cautions against read-to-clear in MSI-X
mode in general. Write-to-clear (section 7.3.4.2) is unconditional.
Replace the magic '1' values written to EIMS/EIMC with IGB_EICR_VEC0,
add the GPIE/EIAC/EIAM macros, and drop the wrong-valued IGB_GPIE_EIAME
macro (the new definition lives next to IGB_GPIE_MULTIPLE_MSIX).
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
tools/testing/selftests/vfio/lib/drivers/igb/igb.c | 40 ++++++++++++++++++----
1 file changed, 34 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index f4f6d2ee262d..637d05e872de 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -280,11 +280,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, E1000_GPIE, E1000_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, E1000_GPIE, E1000_GPIE_MSIX_MODE | E1000_GPIE_EIAME);
+ igb_write32(igb, E1000_EIAC, E1000_EICR_RX_QUEUE0);
+ igb_write32(igb, E1000_EIAM, E1000_EICR_RX_QUEUE0);
/* Enable interrupts on vector 0 */
- igb_write32(igb, E1000_EIMS, 1);
+ igb_write32(igb, E1000_EIMS, E1000_EICR_RX_QUEUE0);
/* Map vector 0 to interrupt cause 0 and mark it valid */
igb_write32(igb, E1000_IVAR0, E1000_IVAR_VALID);
@@ -310,17 +331,24 @@ static void igb_remove(struct vfio_pci_device *device)
static void igb_irq_disable(struct igb *igb)
{
- igb_write32(igb, E1000_EIMC, 1);
+ igb_write32(igb, E1000_EIMC, E1000_EICR_RX_QUEUE0);
}
static void igb_irq_enable(struct igb *igb)
{
- igb_write32(igb, E1000_EIMS, 1);
+ igb_write32(igb, E1000_EIMS, E1000_EICR_RX_QUEUE0);
}
static void igb_irq_clear(struct igb *igb)
{
- igb_read32(igb, E1000_EICR);
+ /*
+ * Use write-to-clear (datasheet 7.3.4.2). In MSI-X mode with EIAC
+ * programmed, section 8.8.5 explicitly states "If any bits are set
+ * in EIAC, the EICR register should not be read", which rules out
+ * the read-to-clear path in 7.3.4.3. Bits not in EIAC are still
+ * cleared by writing 1.
+ */
+ igb_write32(igb, E1000_EICR, 0xFFFFFFFF);
}
static void igb_memcpy_start(struct vfio_pci_device *device, iova_t src,
--
2.55.0.795.g602f6c329a-goog
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing
2026-07-08 23:18 ` [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing Josh Hilke
@ 2026-07-08 23:33 ` sashiko-bot
2026-07-09 20:44 ` Josh Hilke
2026-07-09 22:59 ` Alex Williamson
0 siblings, 2 replies; 23+ messages in thread
From: sashiko-bot @ 2026-07-08 23:33 UTC (permalink / raw)
To: Josh Hilke; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Commit message and implementation mismatch regarding macro definitions.
- [Medium] Hardware Initialization Order: Unmasking interrupts before routing is configured.
--
--- Patch [4]: [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing ---
commit f99b27fd1b6bc3801803a7e5be3ec664facafb7a
Author: Alex Williamson <alex.williamson@nvidia.com>
Subject: vfio: selftests: igb: Program MSI-X interrupt routing
This commit updates the IGB selftest driver to use the full MSI-X programming
sequence required by the 82576 hardware instead of relying on synchronous
QEMU workarounds. It configures auto-clear and auto-mask for vector 0 and
changes the EICR clear mechanism from read-to-clear to write-to-clear.
> Replace the magic '1' values written to EIMS/EIMC with IGB_EICR_VEC0,
> add the GPIE/EIAC/EIAM macros, and drop the wrong-valued IGB_GPIE_EIAME
> macro (the new definition lives next to IGB_GPIE_MULTIPLE_MSIX).
[Severity: Medium]
This isn't a bug in the code, but the commit message mentions introducing
custom macros like IGB_EICR_VEC0 and dropping IGB_GPIE_EIAME.
However, the implementation uses standard kernel definitions such as
E1000_EICR_RX_QUEUE0 and E1000_GPIE_MSIX_MODE.
Could the commit message be updated to match the actual code?
> diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
[ ... ]
> @@ -280,11 +280,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, E1000_GPIE, E1000_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, E1000_GPIE, E1000_GPIE_MSIX_MODE | E1000_GPIE_EIAME);
> + igb_write32(igb, E1000_EIAC, E1000_EICR_RX_QUEUE0);
> + igb_write32(igb, E1000_EIAM, E1000_EICR_RX_QUEUE0);
>
> /* Enable interrupts on vector 0 */
> - igb_write32(igb, E1000_EIMS, 1);
> + igb_write32(igb, E1000_EIMS, E1000_EICR_RX_QUEUE0);
>
> /* Map vector 0 to interrupt cause 0 and mark it valid */
> igb_write32(igb, E1000_IVAR0, E1000_IVAR_VALID);
[Severity: Medium]
Does unmasking the interrupt via E1000_EIMS before programming the routing
in E1000_IVAR0 risk unpredictable behavior?
If an interrupt condition is met in the small window between these writes, it
seems an interrupt could be triggered before the IVAR mapping is valid.
Should the mapping in IVAR0 be established before unmasking the interrupt?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-igb_v3_b4-v3-0-1272650ca614@google.com?part=4
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing
2026-07-08 23:33 ` sashiko-bot
@ 2026-07-09 20:44 ` Josh Hilke
2026-07-09 22:59 ` Alex Williamson
1 sibling, 0 replies; 23+ messages in thread
From: Josh Hilke @ 2026-07-09 20:44 UTC (permalink / raw)
To: sashiko-reviews; +Cc: kvm
On Wed, Jul 8, 2026 at 4:33 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Commit message and implementation mismatch regarding macro definitions.
> - [Medium] Hardware Initialization Order: Unmasking interrupts before routing is configured.
> --
>
> --- Patch [4]: [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing ---
> commit f99b27fd1b6bc3801803a7e5be3ec664facafb7a
> Author: Alex Williamson <alex.williamson@nvidia.com>
> Subject: vfio: selftests: igb: Program MSI-X interrupt routing
>
> This commit updates the IGB selftest driver to use the full MSI-X programming
> sequence required by the 82576 hardware instead of relying on synchronous
> QEMU workarounds. It configures auto-clear and auto-mask for vector 0 and
> changes the EICR clear mechanism from read-to-clear to write-to-clear.
>
> > Replace the magic '1' values written to EIMS/EIMC with IGB_EICR_VEC0,
> > add the GPIE/EIAC/EIAM macros, and drop the wrong-valued IGB_GPIE_EIAME
> > macro (the new definition lives next to IGB_GPIE_MULTIPLE_MSIX).
>
> [Severity: Medium]
> This isn't a bug in the code, but the commit message mentions introducing
> custom macros like IGB_EICR_VEC0 and dropping IGB_GPIE_EIAME.
>
> However, the implementation uses standard kernel definitions such as
> E1000_EICR_RX_QUEUE0 and E1000_GPIE_MSIX_MODE.
>
> Could the commit message be updated to match the actual code?
I'll update it in v4.
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
>
> [ ... ]
>
> > @@ -280,11 +280,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, E1000_GPIE, E1000_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, E1000_GPIE, E1000_GPIE_MSIX_MODE | E1000_GPIE_EIAME);
> > + igb_write32(igb, E1000_EIAC, E1000_EICR_RX_QUEUE0);
> > + igb_write32(igb, E1000_EIAM, E1000_EICR_RX_QUEUE0);
> >
> > /* Enable interrupts on vector 0 */
> > - igb_write32(igb, E1000_EIMS, 1);
> > + igb_write32(igb, E1000_EIMS, E1000_EICR_RX_QUEUE0);
> >
> > /* Map vector 0 to interrupt cause 0 and mark it valid */
> > igb_write32(igb, E1000_IVAR0, E1000_IVAR_VALID);
>
> [Severity: Medium]
> Does unmasking the interrupt via E1000_EIMS before programming the routing
> in E1000_IVAR0 risk unpredictable behavior?
>
> If an interrupt condition is met in the small window between these writes, it
> seems an interrupt could be triggered before the IVAR mapping is valid.
>
> Should the mapping in IVAR0 be established before unmasking the interrupt?
Yes, but the behavior is introduced in patch 1, so I'll fix it there.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing
2026-07-08 23:33 ` sashiko-bot
2026-07-09 20:44 ` Josh Hilke
@ 2026-07-09 22:59 ` Alex Williamson
1 sibling, 0 replies; 23+ messages in thread
From: Alex Williamson @ 2026-07-09 22:59 UTC (permalink / raw)
To: sashiko-bot; +Cc: sashiko-reviews, Josh Hilke, kvm, alex
On Wed, 08 Jul 2026 23:33:40 +0000
sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Commit message and implementation mismatch regarding macro definitions.
> - [Medium] Hardware Initialization Order: Unmasking interrupts before routing is configured.
> --
>
> --- Patch [4]: [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing ---
> commit f99b27fd1b6bc3801803a7e5be3ec664facafb7a
> Author: Alex Williamson <alex.williamson@nvidia.com>
> Subject: vfio: selftests: igb: Program MSI-X interrupt routing
>
> This commit updates the IGB selftest driver to use the full MSI-X programming
> sequence required by the 82576 hardware instead of relying on synchronous
> QEMU workarounds. It configures auto-clear and auto-mask for vector 0 and
> changes the EICR clear mechanism from read-to-clear to write-to-clear.
>
> > Replace the magic '1' values written to EIMS/EIMC with IGB_EICR_VEC0,
> > add the GPIE/EIAC/EIAM macros, and drop the wrong-valued IGB_GPIE_EIAME
> > macro (the new definition lives next to IGB_GPIE_MULTIPLE_MSIX).
>
> [Severity: Medium]
> This isn't a bug in the code, but the commit message mentions introducing
> custom macros like IGB_EICR_VEC0 and dropping IGB_GPIE_EIAME.
>
> However, the implementation uses standard kernel definitions such as
> E1000_EICR_RX_QUEUE0 and E1000_GPIE_MSIX_MODE.
>
> Could the commit message be updated to match the actual code?
Again the paragraph can mostly go away, the only thing left seems to be
replacing 1 with E1000_EICR_RX_QUEUE0.
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
>
> [ ... ]
>
> > @@ -280,11 +280,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, E1000_GPIE, E1000_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, E1000_GPIE, E1000_GPIE_MSIX_MODE | E1000_GPIE_EIAME);
> > + igb_write32(igb, E1000_EIAC, E1000_EICR_RX_QUEUE0);
> > + igb_write32(igb, E1000_EIAM, E1000_EICR_RX_QUEUE0);
> >
> > /* Enable interrupts on vector 0 */
> > - igb_write32(igb, E1000_EIMS, 1);
> > + igb_write32(igb, E1000_EIMS, E1000_EICR_RX_QUEUE0);
> >
> > /* Map vector 0 to interrupt cause 0 and mark it valid */
> > igb_write32(igb, E1000_IVAR0, E1000_IVAR_VALID);
>
> [Severity: Medium]
> Does unmasking the interrupt via E1000_EIMS before programming the routing
> in E1000_IVAR0 risk unpredictable behavior?
>
> If an interrupt condition is met in the small window between these writes, it
> seems an interrupt could be triggered before the IVAR mapping is valid.
>
> Should the mapping in IVAR0 be established before unmasking the interrupt?
Yes, this can change to:
- /* Enable interrupts on vector 0 */
- igb_write32(igb, E1000_EIMS, E1000_EICR_RX_QUEUE0);
-
/* Map vector 0 to interrupt cause 0 and mark it valid */
igb_write32(igb, E1000_IVAR0, E1000_IVAR_VALID);
+
+ /* Enable interrupts on vector 0 once routing is established */
+ igb_write32(igb, E1000_EIMS, E1000_EICR_RX_QUEUE0);
Thanks,
Alex
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 5/9] vfio: selftests: igb: Extend memcpy completion timeout for line-rate hardware
2026-07-08 23:18 [PATCH v3 0/9] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
` (3 preceding siblings ...)
2026-07-08 23:18 ` [PATCH v3 4/9] vfio: selftests: igb: Program MSI-X interrupt routing Josh Hilke
@ 2026-07-08 23:18 ` Josh Hilke
2026-07-08 23:18 ` [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries Josh Hilke
` (3 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Josh Hilke @ 2026-07-08 23:18 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Shuah Khan, linux-kernel, kvm, linux-kselftest, Vipin Sharma,
Josh Hilke, Alex Williamson
From: Alex Williamson <alex.williamson@nvidia.com>
The submitted driver waits at most 1 ms (100 * 10 us) for the last RX
descriptor to be written back. QEMU's emulated loopback is synchronous:
by the time igb_memcpy_wait() runs, the receive descriptors are already
written back. Real 82576 hardware processes the descriptor ring at
line rate.
max_memcpy_size is (RING_SIZE - 1) * IGB_MAX_CHUNK_SIZE, approximately
4 MB, split into 4095 1 KB frames. At 1 Gb/s line rate (~125 MB/s),
4 MB takes ~32 ms on the wire, plus per-frame preamble, SFD,
inter-frame gap and FCS overhead (~3%) and descriptor fetch/writeback
latency. The 1 ms cap times out long before any valid transfer can
complete.
Wait up to ~200 ms (200 iterations * 1 ms) for descriptor writeback
before returning -ETIMEDOUT. ~6x the line-rate floor leaves comfortable
headroom for host scheduling jitter while keeping the intentional
invalid-DMA tests (mix_and_match) so they recover quickly on real faults.
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
tools/testing/selftests/vfio/lib/drivers/igb/igb.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index 637d05e872de..1848be6cd46c 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -412,12 +412,23 @@ static int igb_memcpy_wait(struct vfio_pci_device *device)
prev_tail = (igb->rx_tail + RING_SIZE - 1) % RING_SIZE;
rx = &igb->rx_ring[prev_tail];
- retries = 100;
+ /*
+ * Real 82576 hardware processes the descriptor ring at line rate.
+ * max_memcpy_size = (RING_SIZE - 1) * IGB_MAX_CHUNK_SIZE ~= 4 MB,
+ * split into 4095 1 KB frames. At 1 Gb/s (~125 MB/s) the worst
+ * valid memcpy takes ~32 ms on the wire, plus per-frame preamble,
+ * SFD, IFG and FCS overhead (~3%) and descriptor fetch/writeback
+ * latency. Wait up to ~200 ms before declaring the device hung;
+ * ~6x the line-rate floor leaves comfortable headroom for host
+ * scheduling jitter while keeping the intentional invalid-DMA
+ * tests bounded.
+ */
+ retries = 200;
while (retries-- > 0) {
status = le32_to_cpu(READ_ONCE(rx->wb.status_error));
if (status & 1)
break;
- usleep(10);
+ usleep(1000);
}
igb_irq_clear(igb);
--
2.55.0.795.g602f6c329a-goog
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries
2026-07-08 23:18 [PATCH v3 0/9] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
` (4 preceding siblings ...)
2026-07-08 23:18 ` [PATCH v3 5/9] vfio: selftests: igb: Extend memcpy completion timeout for line-rate hardware Josh Hilke
@ 2026-07-08 23:18 ` Josh Hilke
2026-07-08 23:31 ` sashiko-bot
2026-07-08 23:18 ` [PATCH v3 7/9] vfio: selftests: Add helpers to re-enable interrupts Josh Hilke
` (2 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Josh Hilke @ 2026-07-08 23:18 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Shuah Khan, linux-kernel, kvm, linux-kselftest, Vipin Sharma,
Josh Hilke, Alex Williamson
From: Alex Williamson <alex.williamson@nvidia.com>
The mix_and_match test intentionally submits TX descriptors with an
unmapped source IOVA so that the DMA read fails. By default the 82576
re-sends the request after a PCIe completion timeout (datasheet section
8.6.1, GCR.Completion_Timeout_Resend, bit 16, initial value 1b). On
real hardware this turns a single fault into a stream of retried reads,
keeping PCIe AER and IOMMU error handling busy and interfering with
reset recovery.
Clear GCR.Completion_Timeout_Resend during device initialization so a
failed read fails once and stays failed.
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
tools/testing/selftests/vfio/lib/drivers/igb/igb.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index 1848be6cd46c..a7a30660b23e 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -219,6 +219,18 @@ static void igb_init(struct vfio_pci_device *device)
vfio_pci_config_writew(device, PCI_COMMAND, cmd_reg);
}
+ /*
+ * 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 PHY internal loopback for testing. */
igb_setup_loopback(igb);
--
2.55.0.795.g602f6c329a-goog
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries
2026-07-08 23:18 ` [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries Josh Hilke
@ 2026-07-08 23:31 ` sashiko-bot
2026-07-09 22:48 ` Josh Hilke
2026-07-09 22:59 ` Alex Williamson
0 siblings, 2 replies; 23+ messages in thread
From: sashiko-bot @ 2026-07-08 23:31 UTC (permalink / raw)
To: Josh Hilke; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing SW/FW synchronization semaphore when accessing PHY registers via MDIO.
- [Medium] Missing DMA read memory barrier (dma_rmb) after descriptor completion polling.
--
--- Patch [6]: [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries ---
commit b47012b45a463be841715f383fb27408df2c020e
Author: Alex Williamson <alex.williamson@nvidia.com>
vfio: selftests: igb: Disable PCIe completion timeout retries
The mix_and_match test intentionally submits TX descriptors with an
unmapped source IOVA so that the DMA read fails. By default the 82576
re-sends the request after a PCIe completion timeout (datasheet section
8.6.1, GCR.Completion_Timeout_Resend, bit 16, initial value 1b). On
real hardware this turns a single fault into a stream of retried reads,
keeping PCIe AER and IOMMU error handling busy and interfering with
reset recovery.
Clear GCR.Completion_Timeout_Resend during device initialization so a
failed read fails once and stays failed.
> diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> index 1848be6cd46c9..a7a30660b23ed 100644
> --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> @@ -219,6 +219,18 @@ static void igb_init(struct vfio_pci_device *device)
> vfio_pci_config_writew(device, PCI_COMMAND, cmd_reg);
> }
>
> + /*
> + * 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 PHY internal loopback for testing. */
> igb_setup_loopback(igb);
[Severity: Medium]
Does writing directly to the MDIC register on real 82576 hardware risk
colliding with the firmware or BMC management engine?
When running tests on physical devices, accessing PHY registers without
acquiring the E1000_SW_FW_SYNC semaphore can cause corrupted MDIO
transactions or hardware hangs. For example, in igb_write_phy():
mdic = (((u32)data) |
(offset << E1000_MDIC_REG_SHIFT) |
(1 << E1000_MDIC_PHY_SHIFT) |
E1000_MDIC_OP_WRITE);
igb_write32(igb, E1000_MDIC, mdic);
[Severity: Medium]
Can the CPU speculatively read the destination DMA buffer before the hardware
descriptor writeback is architecturally complete?
In igb_memcpy_wait(), the code polls the status bit via READ_ONCE() but
returns without issuing a dma_rmb():
status = le32_to_cpu(READ_ONCE(rx->wb.status_error));
if (status & 1)
break;
usleep(1000);
}
igb_irq_clear(igb);
On weakly-ordered architectures like ARM64, the test runner's subsequent
memcmp() reads on the DMA destination buffer could be speculatively
reordered before the loop's status poll finishes, resulting in stale
data reads and sporadic test failures.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-igb_v3_b4-v3-0-1272650ca614@google.com?part=6
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries
2026-07-08 23:31 ` sashiko-bot
@ 2026-07-09 22:48 ` Josh Hilke
2026-07-09 22:59 ` Alex Williamson
1 sibling, 0 replies; 23+ messages in thread
From: Josh Hilke @ 2026-07-09 22:48 UTC (permalink / raw)
To: sashiko-reviews; +Cc: kvm
On Wed, Jul 8, 2026 at 4:31 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Missing SW/FW synchronization semaphore when accessing PHY registers via MDIO.
> - [Medium] Missing DMA read memory barrier (dma_rmb) after descriptor completion polling.
> --
>
> --- Patch [6]: [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries ---
> commit b47012b45a463be841715f383fb27408df2c020e
> Author: Alex Williamson <alex.williamson@nvidia.com>
>
> vfio: selftests: igb: Disable PCIe completion timeout retries
>
> The mix_and_match test intentionally submits TX descriptors with an
> unmapped source IOVA so that the DMA read fails. By default the 82576
> re-sends the request after a PCIe completion timeout (datasheet section
> 8.6.1, GCR.Completion_Timeout_Resend, bit 16, initial value 1b). On
> real hardware this turns a single fault into a stream of retried reads,
> keeping PCIe AER and IOMMU error handling busy and interfering with
> reset recovery.
>
> Clear GCR.Completion_Timeout_Resend during device initialization so a
> failed read fails once and stays failed.
>
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > index 1848be6cd46c9..a7a30660b23ed 100644
> > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > @@ -219,6 +219,18 @@ static void igb_init(struct vfio_pci_device *device)
> > vfio_pci_config_writew(device, PCI_COMMAND, cmd_reg);
> > }
> >
> > + /*
> > + * 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 PHY internal loopback for testing. */
> > igb_setup_loopback(igb);
>
> [Severity: Medium]
> Does writing directly to the MDIC register on real 82576 hardware risk
> colliding with the firmware or BMC management engine?
>
> When running tests on physical devices, accessing PHY registers without
> acquiring the E1000_SW_FW_SYNC semaphore can cause corrupted MDIO
> transactions or hardware hangs. For example, in igb_write_phy():
>
> mdic = (((u32)data) |
> (offset << E1000_MDIC_REG_SHIFT) |
> (1 << E1000_MDIC_PHY_SHIFT) |
> E1000_MDIC_OP_WRITE);
>
> igb_write32(igb, E1000_MDIC, mdic);
The risk of this happening is low for physical hardware, and for QEMU,
it isn't even possible because writes to MDIC are synchronous.
There's a couple of ways to address this:
1) Do nothing and add a comment stating the risk.
2) Add a simple retry loop that checks MDIC ready and error bits.
Technically data corruption is still possible because the MDIO
protocol doesn't have a CRC, parity check, or checksum.
3) Implement the full semaphore acquisition which requires about 60
more lines of code.
> [Severity: Medium]
> Can the CPU speculatively read the destination DMA buffer before the hardware
> descriptor writeback is architecturally complete?
>
> In igb_memcpy_wait(), the code polls the status bit via READ_ONCE() but
> returns without issuing a dma_rmb():
>
> status = le32_to_cpu(READ_ONCE(rx->wb.status_error));
> if (status & 1)
> break;
> usleep(1000);
> }
>
> igb_irq_clear(igb);
>
> On weakly-ordered architectures like ARM64, the test runner's subsequent
> memcmp() reads on the DMA destination buffer could be speculatively
> reordered before the loop's status poll finishes, resulting in stale
> data reads and sporadic test failures.
Will fix in v4.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries
2026-07-08 23:31 ` sashiko-bot
2026-07-09 22:48 ` Josh Hilke
@ 2026-07-09 22:59 ` Alex Williamson
1 sibling, 0 replies; 23+ messages in thread
From: Alex Williamson @ 2026-07-09 22:59 UTC (permalink / raw)
To: sashiko-bot; +Cc: sashiko-reviews, Josh Hilke, kvm, alex
On Wed, 08 Jul 2026 23:31:23 +0000
sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Missing SW/FW synchronization semaphore when accessing PHY registers via MDIO.
> - [Medium] Missing DMA read memory barrier (dma_rmb) after descriptor completion polling.
> --
>
> --- Patch [6]: [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries ---
> commit b47012b45a463be841715f383fb27408df2c020e
> Author: Alex Williamson <alex.williamson@nvidia.com>
>
> vfio: selftests: igb: Disable PCIe completion timeout retries
>
> The mix_and_match test intentionally submits TX descriptors with an
> unmapped source IOVA so that the DMA read fails. By default the 82576
> re-sends the request after a PCIe completion timeout (datasheet section
> 8.6.1, GCR.Completion_Timeout_Resend, bit 16, initial value 1b). On
> real hardware this turns a single fault into a stream of retried reads,
> keeping PCIe AER and IOMMU error handling busy and interfering with
> reset recovery.
>
> Clear GCR.Completion_Timeout_Resend during device initialization so a
> failed read fails once and stays failed.
>
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > index 1848be6cd46c9..a7a30660b23ed 100644
> > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > @@ -219,6 +219,18 @@ static void igb_init(struct vfio_pci_device *device)
> > vfio_pci_config_writew(device, PCI_COMMAND, cmd_reg);
> > }
> >
> > + /*
> > + * 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 PHY internal loopback for testing. */
> > igb_setup_loopback(igb);
>
> [Severity: Medium]
> Does writing directly to the MDIC register on real 82576 hardware risk
> colliding with the firmware or BMC management engine?
>
> When running tests on physical devices, accessing PHY registers without
> acquiring the E1000_SW_FW_SYNC semaphore can cause corrupted MDIO
> transactions or hardware hangs. For example, in igb_write_phy():
>
> mdic = (((u32)data) |
> (offset << E1000_MDIC_REG_SHIFT) |
> (1 << E1000_MDIC_PHY_SHIFT) |
> E1000_MDIC_OP_WRITE);
>
> igb_write32(igb, E1000_MDIC, mdic);
Technically yes, but it's non-trivial and we assume an isolated
environment. I test on a plugin dual-port NIC, potentially an
integrated NIC with manageability on the platform might encounter
something like this, but it's a narrow window that's probably
acceptable for a selftest driver. We could add the following above
igb_write_phy() to note the deferral:
+/*
+ * Write a PHY register over MDIO.
+ *
+ * A production driver would hold the SW/FW semaphore (SWSM.SWESMBI + the
+ * SW_FW_SYNC PHY bit) across the MDIO transaction to serialize against the
+ * device's management firmware. The selftest owns the assigned function
+ * exclusively on a dedicated test device with no active manageability
+ * contending for the PHY, so the sync is omitted; it should be added here
+ * if this ever needs to run on a manageability-enabled NIC.
+ */
> [Severity: Medium]
> Can the CPU speculatively read the destination DMA buffer before the hardware
> descriptor writeback is architecturally complete?
>
> In igb_memcpy_wait(), the code polls the status bit via READ_ONCE() but
> returns without issuing a dma_rmb():
>
> status = le32_to_cpu(READ_ONCE(rx->wb.status_error));
> if (status & 1)
> break;
> usleep(1000);
> }
>
> igb_irq_clear(igb);
>
> On weakly-ordered architectures like ARM64, the test runner's subsequent
> memcmp() reads on the DMA destination buffer could be speculatively
> reordered before the loop's status poll finishes, resulting in stale
> data reads and sporadic test failures.
igb_memcpy_wait() is implemented in patch 1, and could indeed make use
of an rmb(). This function changes through the series, created in 1,
touched in 5 and 9. Before we get here we could include
<asm/barrier.h> and call rmb() just before return 0;
Re-tested all the suggested code changes, reorder in igb_hw_init() and
rmb() in igb_memcpy_wait() against physical 82576 NIC, 35/35 pass.
Thanks,
Alex
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 7/9] vfio: selftests: Add helpers to re-enable interrupts
2026-07-08 23:18 [PATCH v3 0/9] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
` (5 preceding siblings ...)
2026-07-08 23:18 ` [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries Josh Hilke
@ 2026-07-08 23:18 ` Josh Hilke
2026-07-08 23:18 ` [PATCH v3 8/9] vfio: selftests: igb: Factor hardware programming into igb_hw_init() Josh Hilke
2026-07-08 23:18 ` [PATCH v3 9/9] vfio: selftests: igb: Recover after DMA-read faults Josh Hilke
8 siblings, 0 replies; 23+ messages in thread
From: Josh Hilke @ 2026-07-08 23:18 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Shuah Khan, linux-kernel, kvm, linux-kselftest, Vipin Sharma,
Josh Hilke, Alex Williamson
From: Alex Williamson <alex.williamson@nvidia.com>
Selftest drivers that recover from a fault by issuing VFIO_DEVICE_RESET
need to re-arm device interrupts afterwards. VFIO_DEVICE_RESET tears
down the kernel-side IRQ trigger so a subsequent VFIO_DEVICE_SET_IRQS
is required, but the user-side eventfds (and any fd cached in a test
fixture) are still valid and must be preserved.
vfio_pci_irq_enable() refuses to be called for vectors that already
have an eventfd (VFIO_ASSERT_LT), and vfio_pci_irq_disable() closes
all eventfds before resetting the trigger, so neither is suitable.
Add vfio_pci_irq_reenable(device, index, vector, count) which asserts
that the requested range has existing eventfds and re-issues
VFIO_DEVICE_SET_IRQS using them. Signature mirrors vfio_pci_irq_enable().
Add vfio_pci_msi{,x}_reenable() wrappers around vfio_pci_irq_reenable()
for additional ease of use and readability.
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
---
.../vfio/lib/include/libvfio/vfio_pci_device.h | 14 ++++++++++++++
tools/testing/selftests/vfio/lib/vfio_pci_device.c | 22 ++++++++++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
index 2858885a89bb..2e67afc0d580 100644
--- a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
+++ b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
@@ -68,6 +68,8 @@ void vfio_pci_config_access(struct vfio_pci_device *device, bool write,
void vfio_pci_irq_enable(struct vfio_pci_device *device, u32 index,
u32 vector, int count);
void vfio_pci_irq_disable(struct vfio_pci_device *device, u32 index);
+void vfio_pci_irq_reenable(struct vfio_pci_device *device, u32 index,
+ u32 vector, int count);
void vfio_pci_irq_trigger(struct vfio_pci_device *device, u32 index, u32 vector);
static inline void fcntl_set_nonblock(int fd)
@@ -92,6 +94,12 @@ static inline void vfio_pci_msi_disable(struct vfio_pci_device *device)
vfio_pci_irq_disable(device, VFIO_PCI_MSI_IRQ_INDEX);
}
+static inline void vfio_pci_msi_reenable(struct vfio_pci_device *device,
+ u32 vector, int count)
+{
+ vfio_pci_irq_reenable(device, VFIO_PCI_MSI_IRQ_INDEX, vector, count);
+}
+
static inline void vfio_pci_msix_enable(struct vfio_pci_device *device,
u32 vector, int count)
{
@@ -103,6 +111,12 @@ static inline void vfio_pci_msix_disable(struct vfio_pci_device *device)
vfio_pci_irq_disable(device, VFIO_PCI_MSIX_IRQ_INDEX);
}
+static inline void vfio_pci_msix_reenable(struct vfio_pci_device *device,
+ u32 vector, int count)
+{
+ vfio_pci_irq_reenable(device, VFIO_PCI_MSIX_IRQ_INDEX, vector, count);
+}
+
static inline int __to_iova(struct vfio_pci_device *device, void *vaddr, iova_t *iova)
{
return __iommu_hva2iova(device->iommu, vaddr, iova);
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index fc75e04ef010..7b8394d0ac50 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -106,6 +106,28 @@ void vfio_pci_irq_disable(struct vfio_pci_device *device, u32 index)
vfio_pci_irq_set(device, index, 0, 0, NULL);
}
+/*
+ * Re-issue VFIO_DEVICE_SET_IRQS for an already-enabled vector range using
+ * the existing eventfds. Intended for drivers that need to re-arm device
+ * interrupts after a VFIO_DEVICE_RESET, which tears down the kernel-side
+ * IRQ trigger but leaves user-side eventfds intact. Recreating the
+ * eventfds would invalidate any test-fixture cache of the fd, so this
+ * helper deliberately preserves them.
+ */
+void vfio_pci_irq_reenable(struct vfio_pci_device *device, u32 index,
+ u32 vector, int count)
+{
+ int i;
+
+ check_supported_irq_index(index);
+
+ for (i = vector; i < vector + count; i++)
+ VFIO_ASSERT_GE(device->msi_eventfds[i], 0,
+ "vector %d eventfd not allocated\n", i);
+
+ vfio_pci_irq_set(device, index, vector, count, device->msi_eventfds + vector);
+}
+
static void vfio_pci_irq_get(struct vfio_pci_device *device, u32 index,
struct vfio_irq_info *irq_info)
{
--
2.55.0.795.g602f6c329a-goog
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 8/9] vfio: selftests: igb: Factor hardware programming into igb_hw_init()
2026-07-08 23:18 [PATCH v3 0/9] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
` (6 preceding siblings ...)
2026-07-08 23:18 ` [PATCH v3 7/9] vfio: selftests: Add helpers to re-enable interrupts Josh Hilke
@ 2026-07-08 23:18 ` Josh Hilke
2026-07-08 23:18 ` [PATCH v3 9/9] vfio: selftests: igb: Recover after DMA-read faults Josh Hilke
8 siblings, 0 replies; 23+ messages in thread
From: Josh Hilke @ 2026-07-08 23:18 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Shuah Khan, linux-kernel, kvm, linux-kselftest, Vipin Sharma,
Josh Hilke, Alex Williamson
From: Alex Williamson <alex.williamson@nvidia.com>
Split the device register programming out of igb_init() into a new
igb_hw_init() helper so that the same sequence can be re-run after a
VFIO_DEVICE_RESET to restore the registers that CTRL.RST clears. No
functional change for the initial path.
igb_init() now performs the one-shot setup: region size assertion, BAR
mapping, CTRL.RST + IMC mask-all to put the device into a known state,
and vfio_pci_msix_enable() to set up the kernel-side IRQ trigger.
igb_hw_init() does the rest: ring pointer setup and IOVA calc,
CTRL_EXT, PCI bus master, GCR, PHY loopback, descriptor rings, RCTL,
TCTL, GPIE/EIAC/EIAM/EIMS/IVAR, and driver-state initialization.
vfio_pci_msix_enable() moves from after RCTL/TCTL to before all
device-side programming. Its only side effects are the VFIO kernel
IRQ trigger setup and the PCI MSI-X capability bits in config space;
neither has any ordering dependency on the 82576 device register
writes performed in igb_hw_init(). Performing it once in igb_init()
keeps igb_hw_init() reusable from the reset recovery path (which uses
vfio_pci_irq_reenable() to re-arm the existing trigger).
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
tools/testing/selftests/vfio/lib/drivers/igb/igb.c | 61 ++++++++++++++--------
1 file changed, 40 insertions(+), 21 deletions(-)
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index a7a30660b23e..46207f018358 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -177,7 +177,13 @@ static int igb_probe(struct vfio_pci_device *device)
return 0;
}
-static void igb_init(struct vfio_pci_device *device)
+/*
+ * Program the device into a usable state. Split out of igb_init() so it
+ * can be reused after a device reset to re-program the registers that
+ * CTRL.RST clears. Expects bar0 to be mapped and MSI-X already enabled
+ * via VFIO.
+ */
+static void igb_hw_init(struct vfio_pci_device *device)
{
struct igb *igb = to_igb_state(device);
u64 iova_tx, iova_rx;
@@ -185,26 +191,9 @@ static void igb_init(struct vfio_pci_device *device)
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);
- /* Reset device and disable all interrupts */
- 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(1000);
- while (igb_read32(igb, E1000_CTRL) & E1000_CTRL_RST)
- usleep(10);
- igb_write32(igb, E1000_IMC, 0xFFFFFFFF);
-
/* Signal that the driver is loaded */
ctrl = igb_read32(igb, E1000_CTRL_EXT);
ctrl |= E1000_CTRL_EXT_DRV_LOAD;
@@ -289,9 +278,6 @@ static void igb_init(struct vfio_pci_device *device)
igb_write32(igb, E1000_RCTL, rctl);
igb_write32(igb, E1000_TCTL, E1000_TCTL_EN);
- /* 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:
*
@@ -331,6 +317,39 @@ static void igb_init(struct vfio_pci_device *device)
device->driver.msi = MSIX_VECTOR;
}
+static void igb_init(struct vfio_pci_device *device)
+{
+ struct igb *igb = to_igb_state(device);
+
+ VFIO_ASSERT_GE(device->driver.region.size, sizeof(struct igb));
+
+ igb->bar0 = device->bars[0].vaddr;
+
+ /* Reset device and disable all interrupts. */
+ 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(1000);
+ while (igb_read32(igb, E1000_CTRL) & E1000_CTRL_RST)
+ usleep(10);
+ igb_write32(igb, E1000_IMC, 0xFFFFFFFF);
+
+ /*
+ * Enable MSI-X via VFIO before device-side register programming.
+ * vfio_pci_msix_enable() only touches the VFIO IRQ machinery and the
+ * PCI MSI-X capability via config space; it has no ordering
+ * dependency on the device-side writes performed by igb_hw_init().
+ * Placing it here keeps igb_hw_init() reusable from the reset
+ * recovery path (which calls vfio_pci_irq_reenable() instead).
+ */
+ vfio_pci_msix_enable(device, MSIX_VECTOR, 1);
+
+ igb_hw_init(device);
+}
+
static void igb_remove(struct vfio_pci_device *device)
{
struct igb *igb = to_igb_state(device);
--
2.55.0.795.g602f6c329a-goog
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 9/9] vfio: selftests: igb: Recover after DMA-read faults
2026-07-08 23:18 [PATCH v3 0/9] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
` (7 preceding siblings ...)
2026-07-08 23:18 ` [PATCH v3 8/9] vfio: selftests: igb: Factor hardware programming into igb_hw_init() Josh Hilke
@ 2026-07-08 23:18 ` Josh Hilke
8 siblings, 0 replies; 23+ messages in thread
From: Josh Hilke @ 2026-07-08 23:18 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Shuah Khan, linux-kernel, kvm, linux-kselftest, Vipin Sharma,
Josh Hilke, Alex Williamson
From: Alex Williamson <alex.williamson@nvidia.com>
The mix_and_match test intentionally submits a TX descriptor with an
unmapped source IOVA so that the DMA read fails. On real 82576
hardware the resulting fault leaves the descriptor engine unable to
service subsequent valid descriptors, so the next memcpy in the same
test iteration times out.
The 82576 datasheet (section 4.2.1.6.1) describes CTRL.RST as the
software mechanism to recover from a hung device. Empirically
CTRL.RST alone is not sufficient in this state: the visible queue
registers are reinitialized, but the next valid memcpy still posts
descriptors without any TDH/TDT progress in the same process. A
fresh device open after the failure works, which points to a reset
scope broader than CTRL.RST being required. The 82576 advertises
PCIe FLR; VFIO_DEVICE_RESET drives FLR and supplies that scope while
preserving the selftest process and its DMA mappings.
Add igb_error_reset_and_reinit() implementing the recovery sequence:
issue VFIO_DEVICE_RESET, re-arm the kernel-side MSI-X trigger against
the still-valid eventfd via vfio_pci_irq_reenable() (this does not
touch the eventfd, which test fixtures may have cached), and
re-program the device via igb_hw_init(). FLR clears EICR and leaves
EIMS=0, so no explicit interrupt mask or cause writes are needed.
igb_hw_init() resets tx_tail/rx_tail to 0 and igb_memcpy_start() zeros
each descriptor before submission, so no ring memset is needed either.
Call this from igb_memcpy_wait() on completion timeout, preceded by a
10 ms delay so that PCIe/IOMMU/AER error handling triggered by the
just-observed DMA fault can release the device lock VFIO_DEVICE_RESET
contends for. The delay is heuristic and tied to the fault path, so
it lives at the call site rather than inside the reset helper. The
failed memcpy still returns -ETIMEDOUT; reset recovery only ensures
the next operation starts from a usable device state.
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Alex Williamson <alex.williamson@nvidia.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
tools/testing/selftests/vfio/lib/drivers/igb/igb.c | 45 +++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index 46207f018358..196905041338 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -432,6 +432,28 @@ static void igb_memcpy_start(struct vfio_pci_device *device, iova_t src,
igb_write32(igb, E1000_TDT(0), igb->tx_tail);
}
+/*
+ * Reset the device via VFIO_DEVICE_RESET (PCIe FLR on the 82576) and
+ * re-program it. VFIO_DEVICE_RESET tears down the kernel-side MSI-X
+ * trigger but leaves user-side eventfds intact, so re-arm the trigger
+ * via vfio_pci_irq_reenable() before reprogramming so any caller-cached
+ * eventfd remains valid.
+ *
+ * FLR clears device-side state to power-on reset values (datasheet
+ * 4.2.1.5.1: a PF FLR is "equivalent to a D0->D3->D0 transition"), so
+ * EIMS and EICR come back as 0 from their register-defined initial
+ * values, and igb_hw_init() resets tx_tail/rx_tail to 0. The next
+ * igb_memcpy_start() will memset each descriptor it touches before
+ * submission, so no explicit IMC/EICR writes or ring memsets are
+ * needed here.
+ */
+static void igb_error_reset_and_reinit(struct vfio_pci_device *device)
+{
+ vfio_pci_device_reset(device);
+ vfio_pci_msix_reenable(device, MSIX_VECTOR, 1);
+ igb_hw_init(device);
+}
+
static int igb_memcpy_wait(struct vfio_pci_device *device)
{
struct igb *igb = to_igb_state(device);
@@ -466,7 +488,28 @@ static int igb_memcpy_wait(struct vfio_pci_device *device)
igb_irq_enable(igb);
- return (status & 1) ? 0 : -ETIMEDOUT;
+ if (status & 1)
+ return 0;
+
+ /*
+ * The descriptor never completed. On real 82576 hardware this
+ * typically follows a DMA-read fault from one of the intentional
+ * unmapped-IOVA tests; the fault leaves the descriptor engine
+ * unable to service subsequent valid descriptors. CTRL.RST alone
+ * reinitializes the queue registers but leaves the engine wedged
+ * for the current process, so a broader VFIO_DEVICE_RESET (FLR)
+ * is required.
+ *
+ * Delay before requesting reset so PCIe/IOMMU/AER error handling
+ * triggered by the just-observed DMA fault can release the device
+ * lock VFIO_DEVICE_RESET contends for. The 10 ms value is
+ * heuristic. The current memcpy still fails with -ETIMEDOUT;
+ * recovery only ensures the next memcpy starts from a usable state.
+ */
+ usleep(10000);
+ igb_error_reset_and_reinit(device);
+
+ return -ETIMEDOUT;
}
static void igb_send_msi(struct vfio_pci_device *device)
--
2.55.0.795.g602f6c329a-goog
^ permalink raw reply related [flat|nested] 23+ messages in thread