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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] vfio/mlx5: HW interface violation in SQ producer index and WQE placement
- [Medium] vfio/mlx5: strict aliasing violation when ringing doorbell via __be64 *
- [Medium] vfio/mlx5: HW DMA race condition due to early CQ doorbell update
--

commit 406822320648621808cbbbbee7e4d206bcd69e8e
Author: Jason Gunthorpe <jgg@ziepe.ca>

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

Complete the mlx5 driver by adding CQ/QP creation, QP state transitions, WQE
posting, CQ polling, and the memcpy_start/memcpy_wait callbacks. The data path
implements RDMA Write self-loopback via an RC QP.

Acked-by: David Matlack <dmatlack@google.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

> diff --git a/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> index 5a87d070dc425..fff3babef5e78 100644
> --- a/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> +++ b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c

[ ... ]

> @@ -1509,24 +1509,24 @@ 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)
>  {
>  	struct mlx5st_send_wqe *wqe;
>  	unsigned int idx;
>  
>  	idx = dev->sq_pi % SQ_WQE_CNT;
>  	wqe = &dev->sq_buf[idx];

[Severity: Medium]
Are we correctly translating between the hardware WQEBB index and our WQE
array index in mlx5st_post_rdma_write()? 

The hardware mandates 16-byte WQE Basic Blocks (WQEBBs), but the driver
calculates the WQE placement assuming a 64-byte stride (SQ_WQE_CNT). 

Could this misalign with what the hardware expects for the send queue?

>  	memset(wqe, 0, sizeof(*wqe));
>  	MLX5_SET(wqe_ctrl_seg, &wqe->ctrl, opcode, MLX5_OPCODE_RDMA_WRITE);
>  	MLX5_SET(wqe_ctrl_seg, &wqe->ctrl, wqe_index, dev->sq_pi);
>  	MLX5_SET(wqe_ctrl_seg, &wqe->ctrl, qp_or_sq, dev->qpn);
>  	MLX5_SET(wqe_ctrl_seg, &wqe->ctrl, ds, MLX5_RDMA_WRITE_DS);

[Severity: Medium]
Here wqe_index is being populated with dev->sq_pi, which seems to be the raw
WQE count, but ds correctly declares the size as 3 WQEBBs. 

Should wqe_index instead track the actual WQEBB index rather than the number
of submitted WQEs?

>  	if (signaled)
>  		MLX5_SET(wqe_ctrl_seg, &wqe->ctrl, ce, MLX5_WQE_CE_CQE_ALWAYS);
>  
>  	MLX5_SET64(wqe_raddr_seg, &wqe->raddr, raddr, dst_addr);
>  	MLX5_SET(wqe_raddr_seg, &wqe->raddr, rkey, dst_rkey);
>  
>  	MLX5_SET(wqe_data_seg, &wqe->data, byte_count, length);
>  	MLX5_SET(wqe_data_seg, &wqe->data, lkey, src_lkey);
>  	MLX5_SET64(wqe_data_seg, &wqe->data, addr, src_addr);
>  
>  	dev->sq_pi++;
>  
>  	/* 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));

[Severity: Medium]
By incrementing sq_pi by 1 per WQE instead of advancing it by the length of
the WQE (3 segments), does this desynchronize the producer index from the
hardware's expected WQEBB offset? 

This looks like it might cause the hardware to read past queue boundaries
or halt.

>  	/*
>  	 * 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 this cast in mlx5st_post_rdma_write() break strict aliasing rules? 

wqe is a pointer to struct mlx5st_send_wqe, and reading it via __be64 * is
an incompatible pointer cast. Since this code resides in tools/ where
-fstrict-aliasing applies, could the compiler reorder this read before the
preceding MLX5_SET writes and send uninitialized data to the doorbell?

>  	dev->uar_bf_offset ^= MLX5_BF_SIZE;
>  }

[ ... ]

> @@ -1565,24 +1565,24 @@ static int mlx5st_poll_cq_batch(struct mlx5st_device *dev,
>  		owner = MLX5_GET_ONCE(cqe64, cqe, owner);
>  		if (owner != ((dev->cq_ci >> LOG_CQ_SIZE) & 1))
>  			break;
>  
>  		dma_rmb();
>  
>  		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: Medium]
Could updating the doorbell record (recv_counter) in mlx5st_poll_cq_batch()
before fully reading the CQE's payload create a hardware race condition?

If the hardware immediately reuses this slot upon seeing the doorbell update,
it might overwrite the buffer before we fetch the remaining fields like
wqe_counter on the next line (or error_syndrome below).

>  			polled++;
>  			continue;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0-v6-8af36cf7b73f+e2-mlx5st_jgg@nvidia.com?part=9