From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7B473425CFA for ; Fri, 10 Jul 2026 22:19:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783721981; cv=none; b=Wsoxz27Cpu7pFz7iohS5hmBmqSS5fEkpVlp902jJBemRD6HZj8HwLbIyZAO3kuxZnwE6iEQcJm+ifk3VdKVYQ2VeVkNOcYIuwvLpiQ2AStSEJLYfGqNSVIefG4b782N83cF7Wtes1cYiruP1okmkKmsOYMSl5iXjyrnRnSwNLNc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783721981; c=relaxed/simple; bh=tlF5NoPIP1L20THWJguzFwb4o9AretVG8X1wheF/Yik=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=AVdtZF8zc20essXrKwULLwNiH0oqx+ZwkUuC+IFlVqNstMBQzJ2+pp3sza4WgxQgTMwM4GkAehRjulP8a5JUKVPR3dxgiQjN6Qy9lb1aaNza0AKCdxiERFXZJ+9M/ApHzxc6oe/Z2Yc5pbFs2N8JNnvjMqfwoYzyYWvniyMyG1g= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=EXmBCj3F; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="EXmBCj3F" Received: by smtp.kernel.org (Postfix) with ESMTPSA id CFE741F000E9; Fri, 10 Jul 2026 22:19:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783721980; bh=9RD2vHCH1f6eviXKp8thAO4SMtxFtwCGuAu4UkDylCg=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=EXmBCj3Fojlcbaxmpm1+J+IMaBVtnQ6WS7r6yIlb/Ex5tqjk9CfNizCN22HeLNjZE 6r4FZGaWgwQv+5oonQKeCYHQy5NQrzDL6UVj0CbiDPvj7Fz6an07cNxv5ssqNOEGEc f2KHU+OuvjAaiVT48GG+OACkcDpoi9dWMr6kpgpoiKgO9l+1S7Xz/kK2OLuw1rt0KJ qmgcF2WGmFxdTZvFFsWGmYXWWS0zu0o/hKhPDJBbXqhNvO6Z6OSDUPNCv0vtnIeNVG 77i4gZLQ88SecaYDxRT7rJ4E9AJvv4kNVNggDi03em1r8iKqKMEhhyxpz+1w3WVZcI techtlHP/Rb8w== From: sashiko-bot@kernel.org Subject: Re: [PATCH v4 1/9] vfio: selftests: igb: Add driver for IGB QEMU device Reply-To: sashiko-reviews@lists.linux.dev To: "Josh Hilke" Cc: kvm@vger.kernel.org In-Reply-To: <20260710-igb_v3_b4-v4-1-56e7e2576cc1@google.com> References: <20260710-igb_v3_b4-v4-0-56e7e2576cc1@google.com> <20260710-igb_v3_b4-v4-1-56e7e2576cc1@google.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Fri, 10 Jul 2026 22:19:39 +0000 Message-Id: <20260710221939.CFE741F000E9@smtp.kernel.org> Precedence: bulk X-Mailing-List: kvm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 6 potential issue(= s) to consider: - [Medium] Reading EICR does not clear the interrupt cause in MSI-X mode, l= eading to spurious interrupts. - [Medium] Failure to disable DMA completion timeout resend causes infinite= retries on intentional unmapped IOVA accesses. - [Medium] Missing Multicast and Broadcast Promiscuous Modes will cause MAC= loopback to drop arbitrary test packets. - [Medium] Unbounded hardware polling loop for device reset completion. - [Medium] Disabling MSI-X before quiescing the device in `igb_remove` crea= tes a race condition causing PCIe faults. - [Low] Driver ignores timeouts when polling for TX/RX queue enable. -- --- Patch [1]: [PATCH v4 1/9] vfio: selftests: igb: Add driver for IGB QEMU= device --- commit 163a68c618c35adde837d2be447ecb7a92e5fdf0 Author: Josh Hilke vfio: selftests: igb: Add driver for IGB QEMU device Add a VFIO selftest driver for the Intel Gigabit Ethernet controller (IGB) to test VFIO features under QEMU. Uses IGB's loopback mode to implement the required memcpy operations. > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/t= esting/selftests/vfio/lib/drivers/igb/igb.c > new file mode 100644 > index 0000000000000..923b2341abadd > --- /dev/null > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c [ ... ] > +static void igb_init(struct vfio_pci_device *device) > +{ > + struct igb *igb =3D 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 =3D device->bars[0].vaddr; > + > + iova_tx =3D to_iova(device, igb->tx_ring); > + iova_rx =3D to_iova(device, igb->rx_ring); > + > + igb_write32(igb, E1000_CTRL, igb_read32(igb, E1000_CTRL) | E1000_CTRL_R= ST); > + /* > + * 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); [Severity: Medium] If the hardware or emulator enters a faulty state and fails to complete the reset, will this unbounded loop hang the test process indefinitely? Should this have a timeout? > + igb_write32(igb, E1000_IMC, 0xFFFFFFFF); > + > + /* Signal that the driver is loaded */ > + ctrl =3D igb_read32(igb, E1000_CTRL_EXT); > + ctrl |=3D E1000_CTRL_EXT_DRV_LOAD; > + ctrl &=3D ~E1000_CTRL_EXT_LINK_MODE_MASK; > + igb_write32(igb, E1000_CTRL_EXT, ctrl); > + > + /* Enable PCI Bus Master. */ > + cmd_reg =3D vfio_pci_config_readw(device, PCI_COMMAND); > + if ((cmd_reg & (PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY)) !=3D > + (PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY)) { > + cmd_reg |=3D (PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY); > + vfio_pci_config_writew(device, PCI_COMMAND, cmd_reg); > + } [Severity: Medium] Since the VFIO selftest framework contains negative tests that intentionally submit DMA descriptors targeting unmapped IOVAs, shouldn't we disable the PCIe completion timeout resend behavior in the GCR register? Without disabling the E1000_GCR_CMPL_TMOUT_RESEND bit, could the 82576 controller continuously retry failed PCIe reads indefinitely, flooding the host with PCIe AER errors and IOMMU faults? [ ... ] > + /* Wait for TX and RX queues to be enabled */ > + retries =3D 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); > + } [Severity: Low] Does this code silently proceed if the retries hit zero? If the hardware queues fail to enable within the allotted time, it seems we might continue with disabled queues, leading to confusing downstream failures. Should there be an assertion or error handling here? > + > + /* Enable Receiver and Transmitter */ > + rctl =3D 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: Medium] Since the driver uses the memcpy test framework which sends arbitrary memory data as Ethernet frames, shouldn't Multicast Promiscuous (E1000_RCTL_MPE) and Broadcast Accept Mode (E1000_RCTL_BAM) be enabled here? Without these flags, any test data where the first byte's least significant bit is 1 will be treated as a multicast/broadcast frame and dropped by the hardware's MAC filter, causing a timeout. [ ... ] > +static void igb_remove(struct vfio_pci_device *device) > +{ > + struct igb *igb =3D 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_R= ST); > +} [Severity: Medium] Does disabling MSI-X before quiescing the device create a race condition? If the device attempts to DMA an MSI-X interrupt during this window (e.g., from a delayed loopback packet), it will perform an unmapped memory write on the PCIe bus, which might trigger an AER or IOMMU fault. Could we disable the receiver and transmitter before calling vfio_pci_msix_disable()? [ ... ] > +static void igb_irq_clear(struct igb *igb) > +{ > + igb_read32(igb, E1000_EICR); > +} [Severity: Medium] Will reading the EICR register clear the interrupt cause in MSI-X mode on the 82576 controller without the EIAC (Extended Interrupt Auto Clear) register being properly configured? It appears the hardware might leave the interrupt cause active, which could trigger immediate spurious interrupts once re-enabled. Should EIAC be programmed during initialization? --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260710-igb_v3_b4-= v4-0-56e7e2576cc1@google.com?part=3D1