Re: [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries

[sashiko-bot@kernel.org]

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