Re: [PATCH v4 09/10] vfio: selftests: Add mlx5 driver - data path and memcpy ops

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Race condition in CQ polling where the `recv_counter` doorbell is updated before all CQE fields are read.
- [Medium] Strict aliasing violation when extracting the 64-bit doorbell value via type punning.
--

--- Patch [9]: [PATCH v4 09/10] vfio: selftests: Add mlx5 driver - data path and memcpy ops ---
commit 22f15a19e89d217e93d4e9e51e36db1a9fa3c2c2
Author: Jason Gunthorpe <[email protected]>

vfio: selftests: Add mlx5 driver - data path and memcpy ops

This commit completes the mlx5 driver by adding CQ/QP creation, QP state
transitions, WQE posting, CQ polling, and the memcpy_start/memcpy_wait
callbacks. It enables functional DMA tests by implementing RDMA Write
self-loopback via an RC QP with force-loopback.

> diff --git a/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> index 804801cc564e7..e5e75adb25316 100644
> --- a/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> +++ b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c

[ ... ]

> @@ -1343,6 +1343,354 @@ static void mlx5st_destroy_mkey(struct mlx5st_device *dev)

[ ... ]

> +static void mlx5st_post_rdma_write(struct mlx5st_device *dev, u64 src_addr,
> +				    u32 src_lkey, u64 dst_addr, u32 dst_rkey,
> +				    u32 length, bool signaled)
> +{

[ ... ]

> +	/* Ensure WQE is visible to device before doorbell record */
> +	dma_wmb();
> +
> +	WRITE_ONCE(dev->qp_dbrec.send_counter,
> +		   cpu_to_be32(dev->sq_pi & 0xffff));
> +
> +	/*
> +	 * Ring doorbell: write first 8 bytes of ctrl to UAR BF register,
> +	 * iowrite has an internal dma_wmb() so the doorbell record will be
> +	 * visible.
> +	 */
> +	iowrite64be(be64_to_cpu(*(__be64 *)wqe),
> +		    (u8 __iomem *)dev->uar_base + dev->uar_bf_offset);

[Severity: Medium]
Does casting the struct mlx5st_send_wqe pointer to a __be64 pointer violate
strict aliasing rules?

Because files under tools/ assume standard -fstrict-aliasing, type punning
with incompatible pointers can cause the compiler to reorder the memory read
before the WQE fields are fully initialized, potentially sending uninitialized
data to the device doorbell.

> +	dev->uar_bf_offset ^= MLX5_BF_SIZE;
> +}
> +
> +/*
> + * Poll CQ
> + */
> +static int mlx5st_poll_cq_batch(struct mlx5st_device *dev,
> +				unsigned int max_cqe)
> +{

[ ... ]

> +		opcode = MLX5_GET(cqe64, cqe, opcode);
> +
> +		dev->cq_ci++;
> +		WRITE_ONCE(dev->cq_dbrec.recv_counter,
> +			   cpu_to_be32(dev->cq_ci & 0xffffff));
> +
> +		if (opcode == MLX5_CQE_REQ) {
> +			dev->sq_ci =
> +				(u16)(MLX5_GET(cqe64, cqe, wqe_counter) + 1);

[Severity: High]
Can the hardware overwrite the CQE memory slot while the CPU is still parsing
it?

By updating the recv_counter doorbell record here, it signals to the hardware
that the CQ slot is free to be reused. Immediately after, the CPU reads the
wqe_counter (and syndrome on error paths) from the CQE. If the hardware posts
new completions rapidly, it might DMA the next CQE into the same slot before
these reads complete.

Could the doorbell update be delayed until after all fields have been read
from the CQE, or batched at the end of the poll loop?

> +			polled++;
> +			continue;
> +		}
> +		if (opcode == MLX5_CQE_REQ_ERR ||
> +		    opcode == MLX5_CQE_RESP_ERR) {
> +			dev_dbg(dev->device,
> +				"CQE error: opcode=0x%x syndrome=0x%x vendor=0x%x\n",
> +				opcode,
> +				MLX5_GET(cqe64, cqe, error_syndrome.syndrome),
> +				MLX5_GET(cqe64, cqe,
> +					 error_syndrome.vendor_error_syndrome));
> +			return -1;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9