Re: [NDCTL PATCH v4 3/3] cxl/test: Add test for cxl features device

[Dave Jiang <dave.jiang@intel.com>]

On 4/9/25 1:40 PM, Alison Schofield wrote:
> On Tue, Feb 18, 2025 at 03:59:56PM -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.
>>
> 
> Let's revisit the naming -
> 
> If the script is cxl-feature.sh then would the C program make sense as
> feature-control.c or ???

I don't have strong opinions on this. I can change it to feature-control.c. 

DJ

> 
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v4:
>> - Adjust for kernel changes of input/out data structures
>> - Setup test script to error out if not -ENODEV
>> - Remove kernel 6.15 check
>> ---
>>  test/cxl-features.sh |  31 ++++
>>  test/fwctl.c         | 383 +++++++++++++++++++++++++++++++++++++++++++
>>  test/meson.build     |  45 +++++
>>  3 files changed, 459 insertions(+)
>>  create mode 100755 test/cxl-features.sh
>>  create mode 100644 test/fwctl.c
>>
>> diff --git a/test/cxl-features.sh b/test/cxl-features.sh
> 
> snip
> 
>> diff --git a/test/fwctl.c b/test/fwctl.c
>> new file mode 100644
>> index 000000000000..ca39e30f6dca
>> --- /dev/null
>> +++ b/test/fwctl.c
>> @@ -0,0 +1,383 @@
>> +// 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 <cxl/features.h>
>> +#include <fwctl/fwctl.h>
>> +#include <fwctl/cxl.h>
>> +#include <linux/uuid.h>
>> +#include <uuid/uuid.h>
>> +#include <util/bitmap.h>
> 
> Not clear bitmap.h is needed?
> 
>> +
>> +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;
>> +}
> 
> Above the send_command() is factored out and reused. How about doing similar with
> the ioctl setup - ie a setup_and_send_command() that setups up the ioctl and calls
> send_command(). That removes redundancy in *get_feature, *set_feature, *get_supported.
> 
> 
>> +
>> +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;
>> +	struct fwctl_rpc rpc = {0};
>> +	struct fwctl_rpc_cxl *in;
>> +	size_t out_size, in_size;
>> +	uint32_t val;
>> +	void *data;
>> +	int rc;
>> +
>> +	in_size = sizeof(*in) + sizeof(*feat_in);
>> +	rc = posix_memalign((void **)&in, 16, in_size);
>> +	if (rc)
>> +		return -ENOMEM;
>> +	memset(in, 0, in_size);
> 
> How about de-duplicating the repeated posix_memalign() + memset() pattern into
> one helper func like alloc_aligned_memory() - including the memset on success.
> 
> 
>> +	feat_in = &in->get_feat_in;
>> +
>> +	uuid_copy(feat_in->uuid, feat_ctx->uuid);
>> +	feat_in->count = feat_ctx->get_size;
>> +
>> +	out_size = sizeof(*out) + feat_ctx->get_size;
>> +	rc = posix_memalign((void **)&out, 16, out_size);
>> +	if (rc)
>> +		goto free_in;
>> +	memset(out, 0, out_size);
>> +
>> +	in->opcode = CXL_MBOX_OPCODE_GET_FEATURE;
>> +	in->op_size = sizeof(*feat_in);
>> +
>> +	rpc.size = sizeof(rpc);
>> +	rpc.scope = FWCTL_RPC_CONFIGURATION;
>> +	rpc.in_len = in_size;
>> +	rpc.out_len = out_size;
>> +	rpc.in = (uint64_t)(uint64_t *)in;
>> +	rpc.out = (uint64_t)(uint64_t *)out;
>> +
>> +	rc = send_command(fd, &rpc, out);
>> +	if (rc)
>> +		goto free_all;
>> +
>> +	data = out->payload;
>> +	val = le32toh(*(__le32 *)data);
>> +	if (memcmp(&val, &expected_data, sizeof(val)) != 0) {
>> +		rc = -ENXIO;
>> +		goto free_all;
>> +	}
>> +
>> +free_all:
>> +	free(out);
>> +free_in:
>> +	free(in);
>> +	return rc;
>> +}
>> +
> snip
> 
>> +static int test_fwctl_features(struct cxl_memdev *memdev)
>> +{
>> +	struct test_feature feat_ctx;
>> +	unsigned int major, minor;
>> +	int fd, rc;
>> +	char path[256];
>> +
>> +	major = cxl_memdev_get_fwctl_major(memdev);
>> +	minor = cxl_memdev_get_fwctl_minor(memdev);
>> +
>> +	if (!major && !minor)
>> +		return -ENODEV;
>> +
>> +	sprintf(path, "/dev/char/%d:%d", major, minor);
>> +
>> +	fd = open(path, O_RDONLY, 0644);
>> +	if (!fd) {
>> +		fprintf(stderr, "Failed to open: %d\n", -errno);
>> +		return -errno;
>> +	}
> 
> Needs to be "if (fd < 0)"  as open() returns -1 on failure.
> 
> snip