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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing hardware reset after DMA timeout causes subsequent tests to fail
- [Medium] Hardware padding causes out-of-bounds DMA write when memcpy size is less than 60 bytes
--

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

vfio: selftests: igb: Add driver for Intel 82576 device
    
Add a VFIO selftest driver for the Intel Gigabit Ethernet controller
(IGB), specifically targeting the 82576 device. IGB is fully virtualized
in QEMU which makes it easy to run VFIO selftests without needing any
specific hardware.

> diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> new file mode 100644
> index 0000000000000..2f7e5cb26271c
> --- /dev/null
> +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c

[ ... ]

> +static void igb_init(struct vfio_pci_device *device)
> +{
[ ... ]
> +	rctl = E1000_RCTL_EN |       /* Receiver Enable */
> +	       E1000_RCTL_UPE |      /* Unicast Promiscuous (for dummy MAC) */
> +	       E1000_RCTL_MPE |      /* Multicast Promiscuous */
> +	       E1000_RCTL_BAM |      /* Broadcast Accept 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 | E1000_TCTL_PSP);

[Severity: Medium]
Does enabling E1000_TCTL_PSP cause out-of-bounds DMA writes when the memcpy
size is less than 60 bytes?

By setting E1000_TCTL_PSP, the hardware automatically pads short packets to the
Ethernet minimum frame size of 64 bytes.

When igb_memcpy_start() submits a transmission with a small size, it gets
padded by the device. On loopback receive, the MAC strips the 4-byte FCS and
writes the remaining 60 bytes into the destination buffer via DMA.

If the destination buffer was allocated exactly to the requested size, this
will write up to 60 - size bytes past the end of the buffer. 

Could we ensure the destination buffer is always large enough to handle
the padding, or disable the hardware padding?

[ ... ]

> +static int igb_memcpy_wait(struct vfio_pci_device *device)
> +{
[ ... ]
> +	if (status & 1)
> +		/*
> +		 * Ensure the test code doesn't speculatively read the DMA
> +		 * destination buffer before we have verified that the
> +		 * descriptor writeback is complete.
> +		 */
> +		rmb();
> +
> +	igb_irq_clear(igb);
> +
> +	igb_irq_enable(igb);
> +
> +	return (status & 1) ? 0 : -ETIMEDOUT;

[Severity: Medium]
Will this leave the device permanently wedged if the DMA times out?

During intentional invalid-DMA tests, unmapped IOVAs cause IOMMU faults which
halt the DMA engine. This function will then time out and return -ETIMEDOUT.

Because this error path doesn't reset the device or reinitialize the
descriptor rings, the driver state and hardware remain out of sync. This
causes all subsequent tests in the suite to fail.

Should a hardware reset and reinitialization be performed before returning
on a timeout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-igb_v3_b4-v9-0-9e4d8682437e@google.com?part=1