Re: [PATCH 09/11] vfio: selftests: Add mlx5 driver - HW init and command interface

Manuel Ebner
2026-05-02T09:35:39Z

Hi Jason,

i've gone through your patch, just some minor and some optional things.

On Thu, 2026-04-30 at 21:08 -0300, Jason Gunthorpe wrote:

> [...]
> diff --git a/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> new file mode 100644
> index 00000000000000..0ab941bad7a66c
> --- /dev/null
> +++ b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> @@ -0,0 +1,1406 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/*
> + * mlx5 VFIO selftest driver
> + *
> + * Programs mlx5 ConnectX VFs and PFs through the bare-metal command
> interface
> + * and RDMA Write self-loopback to perform DMA.  Implements

Write -> write
else it feels like there is a new sentence

> + * (probe/init/remove) and plugs into the VFIO selftest framework.
> + */
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <string.h>
> +#include <time.h>
> +#include <sched.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +#include <linux/log2.h>
> +#include <linux/pci_regs.h>

sort álphabetically:

+#include <sched.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdlib.h>~
+#include <string.h>
+#include <time.h>
+#include <unistd.h>

+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+#include <linux/pci_regs.h>

> +
> +#include <libvfio.h>
> +
> +#include "mlx5_hw.h"

why are these two lines grouped individually?

> +/* Forward declaration — cmd_exec polls events during command wait */
> +static void mlx5st_process_events(struct mlx5st_device *dev);
> +
> +static const char *mlx5st_cmd_name(u16 opcode)
> +{
> + switch (opcode) {
> + case MLX5_CMD_OP_QUERY_HCA_CAP: return "QUERY_HCA_CAP";
> + case MLX5_CMD_OP_INIT_HCA: return "INIT_HCA";
> + case MLX5_CMD_OP_TEARDOWN_HCA: return "TEARDOWN_HCA";
> + case MLX5_CMD_OP_ENABLE_HCA: return "ENABLE_HCA";
> + case MLX5_CMD_OP_DISABLE_HCA: return "DISABLE_HCA";
> + [...]
> + }
> +}

Maybe line them up like:
Depends on your taste.

+ switch (opcode) {
+ case MLX5_CMD_OP_QUERY_HCA_CAP: return "QUERY_HCA_CAP";
+ case MLX5_CMD_OP_INIT_HCA: return "INIT_HCA";
+ case MLX5_CMD_OP_TEARDOWN_HCA: return "TEARDOWN_HCA";
+ case MLX5_CMD_OP_ENABLE_HCA: return "ENABLE_HCA";
+ case MLX5_CMD_OP_DISABLE_HCA: return "DISABLE_HCA";
+ case MLX5_CMD_OP_QUERY_PAGES: return "QUERY_PAGES";
+ [...]

> +
> + /*
> + * Compute signatures: mailbox blocks first, then cmd_queue_entry
> last.
> + * The sig must cover the final state including ownership=0x1, but
> + * we must not set ownership until after the sig is in place —
> + * XOR in the 0x1 without storing it to memory.
> + */

- " last"

+ /*
+ * Compute signatures: mailbox blocks first, then cmd_queue_entry.

Thanks
Manuel