Re: [NDCTL PATCH v7 4/4] cxl/test: Add test for cxl features device

[Alison Schofield <alison.schofield@intel.com>]

On Wed, May 21, 2025 at 04:22:19PM -0700, Dave Jiang wrote:
> 
> 
> On 5/20/25 12:25 PM, Alison Schofield wrote:
> > On Mon, May 19, 2025 at 01:00:54PM -0700, Dave Jiang wrote:
> >> Add a unit test to verify the features ioctl commands. Test support added
> >> for locating a features device, retrieve and verify the supported features
> >> commands, retrieve specific feature command data, retrieve test feature
> >> data, and write and verify test feature data.
> >>
> >> Acked-by: Dan Williams <dan.j.williams@intel.com>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >>  cxl/fwctl/cxl.h      |   2 +-
> >>  test/cxl-features.sh |  31 +++
> >>  test/fwctl.c         | 439 +++++++++++++++++++++++++++++++++++++++++++
> >>  test/meson.build     |  19 ++
> >>  4 files changed, 490 insertions(+), 1 deletion(-)
> >>  create mode 100755 test/cxl-features.sh
> >>  create mode 100644 test/fwctl.c
> >>
> >> diff --git a/cxl/fwctl/cxl.h b/cxl/fwctl/cxl.h
> >> index 43f522f0cdcd..c560b2a1181d 100644
> >> --- a/cxl/fwctl/cxl.h
> >> +++ b/cxl/fwctl/cxl.h
> >> @@ -9,7 +9,7 @@
> >>  
> >>  #include <linux/types.h>
> >>  #include <linux/stddef.h>
> >> -#include <cxl/features.h>
> >> +#include "features.h"
> >>  
> >>  /**
> >>   * struct fwctl_rpc_cxl - ioctl(FWCTL_RPC) input for CXL
> >> diff --git a/test/cxl-features.sh b/test/cxl-features.sh
> >> new file mode 100755
> >> index 000000000000..3498fa08be53
> >> --- /dev/null
> >> +++ b/test/cxl-features.sh
> >> @@ -0,0 +1,31 @@
> >> +#!/bin/bash -Ex
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (C) 2025 Intel Corporation. All rights reserved.
> >> +
> >> +rc=77
> >> +# 237 is -ENODEV
> >> +ERR_NODEV=237
> >> +
> >> +. $(dirname $0)/common
> >> +FEATURES="$TEST_PATH"/fwctl
> >> +
> >> +trap 'err $LINENO' ERR
> >> +
> >> +modprobe cxl_test
> >> +
> >> +test -x "$FEATURES" || do_skip "no CXL Features Contrl"
> > 
> > Seems like a comment to help understand skip - 
> > 
> > # fwctl test is omitted when ndctl is built with -Dfwctl=disabled
> > # or the kernel is built without CONFIG_CXL_FEATURES enabled.
> > 
> > Now the above is assuming the .sh got in the test list and is 
> > not left out completely with -Dfwctl=disabled. Going to look.
> 
> It's excluded. So comment probably not needed. We skip if there's no fwctl device later, which points to no kernel config enabled.

OK. I've come around to agree on this one. My concern was folks running
the unit test foolishly thinking they had tested everything while this
test was silently skipped. But that's an unlikely because doing the
-Dfwctl=disable is a pretty intentional act. Also folks trying to run
complete suites are aware of the enables/disables because they use
them to turn on the destructive tests. I'll be quiet on this one now.

Thanks!

> 
> DJ
> 
> > 
> > 
> >> +# disable trap
> >> +trap - $(compgen -A signal)
> >> +"$FEATURES"
> >> +rc=$?
> >> +
> >> +echo "error: $rc"
> >> +if [ "$rc" -eq "$ERR_NODEV" ]; then
> >> +	do_skip "no CXL FWCTL char dev"
> >> +elif [ "$rc" -ne 0 ]; then
> >> +	echo "fail: $LINENO" && exit 1
> >> +fi
> >> +
> >> +trap 'err $LINENO' ERR
> >> +
> >> +_cxl_cleanup
> >> diff --git a/test/fwctl.c b/test/fwctl.c
> >> new file mode 100644
> >> index 000000000000..7a780e718872
> >> --- /dev/null
> >> +++ b/test/fwctl.c
> >> @@ -0,0 +1,439 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +// Copyright (C) 2024-2025 Intel Corporation. All rights reserved.
> >> +#include <errno.h>
> >> +#include <fcntl.h>
> >> +#include <stdio.h>
> >> +#include <endian.h>
> >> +#include <stdint.h>
> >> +#include <stdlib.h>
> >> +#include <syslog.h>
> >> +#include <string.h>
> >> +#include <unistd.h>
> >> +#include <sys/ioctl.h>
> >> +#include <cxl/libcxl.h>
> >> +#include <linux/uuid.h>
> >> +#include <uuid/uuid.h>
> >> +#include <util/bitmap.h>
> >> +#include <cxl/fwctl/features.h>
> >> +#include <cxl/fwctl/fwctl.h>
> >> +#include <cxl/fwctl/cxl.h>
> >> +
> >> +static const char provider[] = "cxl_test";
> >> +
> >> +UUID_DEFINE(test_uuid,
> >> +	    0xff, 0xff, 0xff, 0xff,
> >> +	    0xff, 0xff,
> >> +	    0xff, 0xff,
> >> +	    0xff, 0xff,
> >> +	    0xff, 0xff, 0xff, 0xff, 0xff, 0xff
> >> +);
> >> +
> >> +#define CXL_MBOX_OPCODE_GET_SUPPORTED_FEATURES	0x0500
> >> +#define CXL_MBOX_OPCODE_GET_FEATURE		0x0501
> >> +#define CXL_MBOX_OPCODE_SET_FEATURE		0x0502
> >> +
> >> +#define GET_FEAT_SIZE	4
> >> +#define SET_FEAT_SIZE	4
> >> +#define EFFECTS_MASK	(BIT(0) | BIT(9))
> >> +
> >> +#define MAX_TEST_FEATURES	1
> >> +#define DEFAULT_TEST_DATA	0xdeadbeef
> >> +#define DEFAULT_TEST_DATA2	0xabcdabcd
> >> +
> >> +struct test_feature {
> >> +	uuid_t uuid;
> >> +	size_t get_size;
> >> +	size_t set_size;
> >> +};
> >> +
> >> +static int send_command(int fd, struct fwctl_rpc *rpc, struct fwctl_rpc_cxl_out *out)
> >> +{
> >> +	if (ioctl(fd, FWCTL_RPC, rpc) == -1) {
> >> +		fprintf(stderr, "RPC ioctl error: %s\n", strerror(errno));
> >> +		return -errno;
> >> +	}
> >> +
> >> +	if (out->retval) {
> >> +		fprintf(stderr, "operation returned failure: %d\n", out->retval);
> >> +		return -ENXIO;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int get_scope(u16 opcode)
> >> +{
> >> +	switch (opcode) {
> >> +	case CXL_MBOX_OPCODE_GET_SUPPORTED_FEATURES:
> >> +	case CXL_MBOX_OPCODE_GET_FEATURE:
> >> +		return FWCTL_RPC_CONFIGURATION;
> >> +	case CXL_MBOX_OPCODE_SET_FEATURE:
> >> +		return FWCTL_RPC_DEBUG_WRITE_FULL;
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +}
> >> +
> >> +static size_t hw_op_size(u16 opcode)
> >> +{
> >> +	switch (opcode) {
> >> +	case CXL_MBOX_OPCODE_GET_SUPPORTED_FEATURES:
> >> +		return sizeof(struct cxl_mbox_get_sup_feats_in);
> >> +	case CXL_MBOX_OPCODE_GET_FEATURE:
> >> +		return sizeof(struct cxl_mbox_get_feat_in);
> >> +	case CXL_MBOX_OPCODE_SET_FEATURE:
> >> +		return sizeof(struct cxl_mbox_set_feat_in) + sizeof(u32);
> >> +	default:
> >> +		return SIZE_MAX;
> >> +	}
> >> +}
> >> +
> >> +static void free_rpc(struct fwctl_rpc *rpc)
> >> +{
> >> +	void *in, *out;
> >> +
> >> +	in = (void *)rpc->in;
> >> +	out = (void *)rpc->out;
> >> +	free(in);
> >> +	free(out);
> >> +	free(rpc);
> >> +}
> >> +
> >> +static void *zmalloc_aligned(size_t align, size_t size)
> >> +{
> >> +	void *ptr;
> >> +	int rc;
> >> +
> >> +	rc = posix_memalign((void **)&ptr, align, size);
> >> +	if (rc)
> >> +		return NULL;
> >> +	memset(ptr, 0, size);
> >> +
> >> +	return ptr;
> >> +}
> >> +
> >> +static struct fwctl_rpc *get_prepped_command(size_t in_size, size_t out_size,
> >> +					     u16 opcode)
> >> +{
> >> +	struct fwctl_rpc_cxl_out *out;
> >> +	struct fwctl_rpc_cxl *in;
> >> +	struct fwctl_rpc *rpc;
> >> +	size_t op_size;
> >> +	int scope;
> >> +
> >> +	rpc = zmalloc_aligned(16, sizeof(*rpc));
> >> +	if (!rpc)
> >> +		return NULL;
> >> +
> >> +	in = zmalloc_aligned(16, in_size);
> >> +	if (!in)
> >> +		goto free_rpc;
> >> +
> >> +	out = zmalloc_aligned(16, out_size);
> >> +	if (!out)
> >> +		goto free_in;
> >> +
> >> +	in->opcode = opcode;
> >> +
> >> +	op_size = hw_op_size(opcode);
> >> +	if (op_size == SIZE_MAX)
> >> +		goto free_in;
> >> +
> >> +	in->op_size = op_size;
> >> +
> >> +	rpc->size = sizeof(*rpc);
> >> +	scope = get_scope(opcode);
> >> +	if (scope < 0)
> >> +		goto free_all;
> >> +
> >> +	rpc->scope = scope;
> >> +
> >> +	rpc->in_len = in_size;
> >> +	rpc->out_len = out_size;
> >> +	rpc->in = (uint64_t)(uint64_t *)in;
> >> +	rpc->out = (uint64_t)(uint64_t *)out;
> >> +
> >> +	return rpc;
> >> +
> >> +free_all:
> >> +	free(out);
> >> +free_in:
> >> +	free(in);
> >> +free_rpc:
> >> +	free(rpc);
> >> +	return NULL;
> >> +}
> >> +
> >> +static int cxl_fwctl_rpc_get_test_feature(int fd, struct test_feature *feat_ctx,
> >> +					  const uint32_t expected_data)
> >> +{
> >> +	struct cxl_mbox_get_feat_in *feat_in;
> >> +	struct fwctl_rpc_cxl_out *out;
> >> +	size_t out_size, in_size;
> >> +	struct fwctl_rpc_cxl *in;
> >> +	struct fwctl_rpc *rpc;
> >> +	uint32_t val;
> >> +	void *data;
> >> +	int rc;
> >> +
> >> +	in_size = sizeof(*in) + sizeof(*feat_in);
> >> +	out_size = sizeof(*out) + feat_ctx->get_size;
> >> +
> >> +	rpc = get_prepped_command(in_size, out_size,
> >> +				  CXL_MBOX_OPCODE_GET_FEATURE);
> >> +	if (!rpc)
> >> +		return -ENXIO;
> >> +
> >> +	in = (struct fwctl_rpc_cxl *)rpc->in;
> >> +	out = (struct fwctl_rpc_cxl_out *)rpc->out;
> >> +
> >> +	feat_in = &in->get_feat_in;
> >> +	uuid_copy(feat_in->uuid, feat_ctx->uuid);
> >> +	feat_in->count = feat_ctx->get_size;
> >> +
> >> +	rc = send_command(fd, rpc, out);
> >> +	if (rc)
> >> +		goto out;
> >> +
> >> +	data = out->payload;
> >> +	val = le32toh(*(__le32 *)data);
> >> +	if (memcmp(&val, &expected_data, sizeof(val)) != 0) {
> >> +		rc = -ENXIO;
> >> +		goto out;
> >> +	}
> >> +
> >> +out:
> >> +	free_rpc(rpc);
> >> +	return rc;
> >> +}
> >> +
> >> +static int cxl_fwctl_rpc_set_test_feature(int fd, struct test_feature *feat_ctx)
> >> +{
> >> +	struct cxl_mbox_set_feat_in *feat_in;
> >> +	struct fwctl_rpc_cxl_out *out;
> >> +	size_t in_size, out_size;
> >> +	struct fwctl_rpc_cxl *in;
> >> +	struct fwctl_rpc *rpc;
> >> +	uint32_t val;
> >> +	void *data;
> >> +	int rc;
> >> +
> >> +	in_size = sizeof(*in) + sizeof(*feat_in) + sizeof(val);
> >> +	out_size = sizeof(*out) + sizeof(val);
> >> +	rpc = get_prepped_command(in_size, out_size,
> >> +				  CXL_MBOX_OPCODE_SET_FEATURE);
> >> +	if (!rpc)
> >> +		return -ENXIO;
> >> +
> >> +	in = (struct fwctl_rpc_cxl *)rpc->in;
> >> +	out = (struct fwctl_rpc_cxl_out *)rpc->out;
> >> +	feat_in = &in->set_feat_in;
> >> +	uuid_copy(feat_in->uuid, feat_ctx->uuid);
> >> +	data = feat_in->feat_data;
> >> +	val = DEFAULT_TEST_DATA2;
> >> +	*(uint32_t *)data = htole32(val);
> >> +	feat_in->flags = CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER;
> >> +
> >> +	rc = send_command(fd, rpc, out);
> >> +	if (rc)
> >> +		goto out;
> >> +
> >> +	rc = cxl_fwctl_rpc_get_test_feature(fd, feat_ctx, DEFAULT_TEST_DATA2);
> >> +	if (rc) {
> >> +		fprintf(stderr, "Failed ioctl to get feature verify: %d\n", rc);
> >> +		goto out;
> >> +	}
> >> +
> >> +out:
> >> +	free_rpc(rpc);
> >> +	return rc;
> >> +}
> >> +
> >> +static int cxl_fwctl_rpc_get_supported_features(int fd, struct test_feature *feat_ctx)
> >> +{
> >> +	struct cxl_mbox_get_sup_feats_out *feat_out;
> >> +	struct cxl_mbox_get_sup_feats_in *feat_in;
> >> +	struct fwctl_rpc_cxl_out *out;
> >> +	struct cxl_feat_entry *entry;
> >> +	size_t out_size, in_size;
> >> +	struct fwctl_rpc_cxl *in;
> >> +	struct fwctl_rpc *rpc;
> >> +	int feats, rc;
> >> +
> >> +	in_size = sizeof(*in) + sizeof(*feat_in);
> >> +	out_size = sizeof(*out) + sizeof(*feat_out);
> >> +	/* First query, to get number of features w/o per feature data */
> >> +	rpc = get_prepped_command(in_size, out_size,
> >> +				  CXL_MBOX_OPCODE_GET_SUPPORTED_FEATURES);
> >> +	if (!rpc)
> >> +		return -ENXIO;
> >> +
> >> +	/* No need to fill in feat_in first go as we are passing in all 0's */
> >> +
> >> +	out = (struct fwctl_rpc_cxl_out *)rpc->out;
> >> +	rc = send_command(fd, rpc, out);
> >> +	if (rc)
> >> +		goto out;
> >> +
> >> +	feat_out = &out->get_sup_feats_out;
> >> +	feats = le16toh(feat_out->supported_feats);
> >> +	if (feats != MAX_TEST_FEATURES) {
> >> +		fprintf(stderr, "Test device has greater than %d test features.\n",
> >> +			MAX_TEST_FEATURES);
> >> +		rc = -ENXIO;
> >> +		goto out;
> >> +	}
> >> +
> >> +	free_rpc(rpc);
> >> +
> >> +	/* Going second round to retrieve each feature details */
> >> +	in_size = sizeof(*in) + sizeof(*feat_in);
> >> +	out_size = sizeof(*out) + sizeof(*feat_out);
> >> +	out_size += feats * sizeof(*entry);
> >> +	rpc = get_prepped_command(in_size, out_size,
> >> +				  CXL_MBOX_OPCODE_GET_SUPPORTED_FEATURES);
> >> +	if (!rpc)
> >> +		return -ENXIO;
> >> +
> >> +	in = (struct fwctl_rpc_cxl *)rpc->in;
> >> +	out = (struct fwctl_rpc_cxl_out *)rpc->out;
> >> +	feat_in = &in->get_sup_feats_in;
> >> +	feat_in->count = htole32(feats * sizeof(*entry));
> >> +
> >> +	rc = send_command(fd, rpc, out);
> >> +	if (rc)
> >> +		goto out;
> >> +
> >> +	feat_out = &out->get_sup_feats_out;
> >> +	feats = le16toh(feat_out->supported_feats);
> >> +	if (feats != MAX_TEST_FEATURES) {
> >> +		fprintf(stderr, "Test device has greater than %u test features.\n",
> >> +			MAX_TEST_FEATURES);
> >> +		rc = -ENXIO;
> >> +		goto out;
> >> +	}
> >> +
> >> +	if (le16toh(feat_out->num_entries) != MAX_TEST_FEATURES) {
> >> +		fprintf(stderr, "Test device did not return expected entries. %u\n",
> >> +			le16toh(feat_out->num_entries));
> >> +		rc = -ENXIO;
> >> +		goto out;
> >> +	}
> >> +
> >> +	entry = &feat_out->ents[0];
> >> +	if (uuid_compare(test_uuid, entry->uuid) != 0) {
> >> +		fprintf(stderr, "Test device did not export expected test feature.\n");
> >> +		rc = -ENXIO;
> >> +		goto out;
> >> +	}
> >> +
> >> +	if (le16toh(entry->get_feat_size) != GET_FEAT_SIZE ||
> >> +	    le16toh(entry->set_feat_size) != SET_FEAT_SIZE) {
> >> +		fprintf(stderr, "Test device feature in/out size incorrect.\n");
> >> +		rc = -ENXIO;
> >> +		goto out;
> >> +	}
> >> +
> >> +	if (le16toh(entry->effects) != EFFECTS_MASK) {
> >> +		fprintf(stderr, "Test device set effects incorrect\n");
> >> +		rc = -ENXIO;
> >> +		goto out;
> >> +	}
> >> +
> >> +	uuid_copy(feat_ctx->uuid, entry->uuid);
> >> +	feat_ctx->get_size = le16toh(entry->get_feat_size);
> >> +	feat_ctx->set_size = le16toh(entry->set_feat_size);
> >> +
> >> +out:
> >> +	free_rpc(rpc);
> >> +	return rc;
> >> +}
> >> +
> >> +static int test_fwctl_features(struct cxl_memdev *memdev)
> >> +{
> >> +	struct test_feature feat_ctx;
> >> +	unsigned int major, minor;
> >> +	struct cxl_fwctl *fwctl;
> >> +	int fd, rc;
> >> +	char path[256];
> >> +
> >> +	fwctl = cxl_memdev_get_fwctl(memdev);
> >> +	if (!fwctl)
> >> +		return -ENODEV;
> >> +
> >> +	major = cxl_fwctl_get_major(fwctl);
> >> +	minor = cxl_fwctl_get_minor(fwctl);
> >> +
> >> +	if (!major && !minor)
> >> +		return -ENODEV;
> >> +
> >> +	sprintf(path, "/dev/char/%d:%d", major, minor);
> >> +
> >> +	fd = open(path, O_RDONLY, 0644);
> >> +	if (fd < 0) {
> >> +		fprintf(stderr, "Failed to open: %d\n", -errno);
> >> +		return -errno;
> >> +	}
> >> +
> >> +	rc = cxl_fwctl_rpc_get_supported_features(fd, &feat_ctx);
> >> +	if (rc) {
> >> +		fprintf(stderr, "Failed ioctl to get supported features: %d\n", rc);
> >> +		goto out;
> >> +	}
> >> +
> >> +	rc = cxl_fwctl_rpc_get_test_feature(fd, &feat_ctx, DEFAULT_TEST_DATA);
> >> +	if (rc) {
> >> +		fprintf(stderr, "Failed ioctl to get feature: %d\n", rc);
> >> +		goto out;
> >> +	}
> >> +
> >> +	rc = cxl_fwctl_rpc_set_test_feature(fd, &feat_ctx);
> >> +	if (rc) {
> >> +		fprintf(stderr, "Failed ioctl to set feature: %d\n", rc);
> >> +		goto out;
> >> +	}
> >> +
> >> +out:
> >> +	close(fd);
> >> +	return rc;
> >> +}
> >> +
> >> +static int test_fwctl(struct cxl_ctx *ctx, struct cxl_bus *bus)
> >> +{
> >> +	struct cxl_memdev *memdev;
> >> +
> >> +	cxl_memdev_foreach(ctx, memdev) {
> >> +		if (cxl_memdev_get_bus(memdev) != bus)
> >> +			continue;
> >> +		return test_fwctl_features(memdev);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +int main(int argc, char *argv[])
> >> +{
> >> +	struct cxl_ctx *ctx;
> >> +	struct cxl_bus *bus;
> >> +	int rc;
> >> +
> >> +	rc = cxl_new(&ctx);
> >> +	if (rc < 0)
> >> +		return rc;
> >> +
> >> +	cxl_set_log_priority(ctx, LOG_DEBUG);
> >> +
> >> +	bus = cxl_bus_get_by_provider(ctx, provider);
> >> +	if (!bus) {
> >> +		fprintf(stderr, "%s: unable to find bus (%s)\n",
> >> +			argv[0], provider);
> >> +		rc = -EINVAL;
> >> +		goto out;
> >> +	}
> >> +
> >> +	rc = test_fwctl(ctx, bus);
> >> +
> >> +out:
> >> +	cxl_unref(ctx);
> >> +	return rc;
> >> +}
> >> diff --git a/test/meson.build b/test/meson.build
> >> index 2fd7df5211dd..93b1d78671ef 100644
> >> --- a/test/meson.build
> >> +++ b/test/meson.build
> >> @@ -17,6 +17,13 @@ ndctl_deps = libndctl_deps + [
> >>    versiondep,
> >>  ]
> >>  
> >> +libcxl_deps = [
> >> +  cxl_dep,
> >> +  ndctl_dep,
> >> +  uuid,
> >> +  kmod,
> >> +]
> >> +
> >>  libndctl = executable('libndctl', testcore + [ 'libndctl.c'],
> >>    dependencies : libndctl_deps,
> >>    include_directories : root_inc,
> >> @@ -235,6 +242,18 @@ if get_option('keyutils').enabled()
> >>    ]
> >>  endif
> >>  
> >> +uuid_dep = dependency('uuid', required: false)
> >> +if get_option('fwctl').enabled() and uuid_dep.found()
> >> +  fwctl = executable('fwctl', 'fwctl.c',
> >> +    dependencies : libcxl_deps,
> >> +    include_directories : root_inc,
> >> +  )
> >> +  cxl_features = find_program('cxl-features.sh')
> >> +  tests += [
> >> +    [ 'cxl-features.sh',        cxl_features,       'cxl'   ],
> >> +  ]
> >> +endif
> >> +
> >>  foreach t : tests
> >>    test(t[0], t[1],
> >>      is_parallel : false,
> >> -- 
> >> 2.49.0
> >>
> >>
> 
>