Re: [ndctl PATCH v3 4/7] cxl: Add inject-error command

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

On 10/22/2025 12:06 PM, Dave Jiang wrote:
> 
> 
> On 10/21/25 11:31 AM, Ben Cheatham wrote:
>> Add the 'cxl-inject-error' command. This command will provide CXL
>> protocol error injection for CXL VH root ports and CXL RCH downstream
>> ports, as well as poison injection for CXL memory devices.
>>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
>> ---
>>  cxl/builtin.h      |   1 +
>>  cxl/cxl.c          |   1 +
>>  cxl/inject-error.c | 195 +++++++++++++++++++++++++++++++++++++++++++++
>>  cxl/meson.build    |   1 +
>>  4 files changed, 198 insertions(+)
>>  create mode 100644 cxl/inject-error.c
>>
>> diff --git a/cxl/builtin.h b/cxl/builtin.h
>> index c483f30..e82fcb5 100644
>> --- a/cxl/builtin.h
>> +++ b/cxl/builtin.h
>> @@ -25,6 +25,7 @@ int cmd_create_region(int argc, const char **argv, struct cxl_ctx *ctx);
>>  int cmd_enable_region(int argc, const char **argv, struct cxl_ctx *ctx);
>>  int cmd_disable_region(int argc, const char **argv, struct cxl_ctx *ctx);
>>  int cmd_destroy_region(int argc, const char **argv, struct cxl_ctx *ctx);
>> +int cmd_inject_error(int argc, const char **argv, struct cxl_ctx *ctx);
>>  #ifdef ENABLE_LIBTRACEFS
>>  int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx);
>>  #else
>> diff --git a/cxl/cxl.c b/cxl/cxl.c
>> index 1643667..a98bd6b 100644
>> --- a/cxl/cxl.c
>> +++ b/cxl/cxl.c
>> @@ -80,6 +80,7 @@ static struct cmd_struct commands[] = {
>>  	{ "disable-region", .c_fn = cmd_disable_region },
>>  	{ "destroy-region", .c_fn = cmd_destroy_region },
>>  	{ "monitor", .c_fn = cmd_monitor },
>> +	{ "inject-error", .c_fn = cmd_inject_error },
>>  };
>>  
>>  int main(int argc, const char **argv)
>> diff --git a/cxl/inject-error.c b/cxl/inject-error.c
>> new file mode 100644
>> index 0000000..c48ea69
>> --- /dev/null
>> +++ b/cxl/inject-error.c
>> @@ -0,0 +1,195 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (C) 2025 AMD. All rights reserved. */
>> +#include <util/parse-options.h>
>> +#include <cxl/libcxl.h>
>> +#include <cxl/filter.h>
>> +#include <util/log.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +#include <stdio.h>
>> +#include <errno.h>
>> +#include <limits.h>
>> +
>> +#define EINJ_TYPES_BUF_SIZE 512
>> +
>> +static bool debug;
>> +
>> +static struct inject_params {
>> +	const char *type;
>> +	const char *address;
>> +} inj_param;
>> +
>> +static const struct option inject_options[] = {
>> +	OPT_STRING('t', "type", &inj_param.type, "Error type",
>> +		   "Error type to inject into <device>"),
>> +	OPT_STRING('a', "address", &inj_param.address, "Address for poison injection",
>> +		   "Device physical address for poison injection in hex or decimal"),
>> +#ifdef ENABLE_DEBUG
>> +	OPT_BOOLEAN(0, "debug", &debug, "turn on debug output"),
>> +#endif
>> +	OPT_END(),
>> +};
>> +
>> +static struct log_ctx iel;
>> +
>> +static struct cxl_protocol_error *find_cxl_proto_err(struct cxl_ctx *ctx,
>> +						     const char *type)
>> +{
>> +	struct cxl_protocol_error *perror;
>> +
>> +	cxl_protocol_error_foreach(ctx, perror) {
>> +		if (strcmp(type, cxl_protocol_error_get_str(perror)) == 0)
>> +			return perror;
>> +	}
>> +
>> +	log_err(&iel, "Invalid CXL protocol error type: %s\n", type);
>> +	return NULL;
>> +}
>> +
>> +static struct cxl_dport *find_cxl_dport(struct cxl_ctx *ctx, const char *devname)
>> +{
>> +	struct cxl_port *port, *top;
>> +	struct cxl_dport *dport;
>> +	struct cxl_bus *bus;
>> +
>> +	cxl_bus_foreach(ctx, bus) {
>> +		top = cxl_bus_get_port(bus);
>> +
>> +		cxl_port_foreach_all(top, port)
>> +			cxl_dport_foreach(port, dport)
>> +				if (!strcmp(devname,
>> +					    cxl_dport_get_devname(dport)))
>> +					return dport;
> 
> Would it be worthwhile to create a util_cxl_dport_filter()?
> 

Yeah probably. I'll make one for the next revision.

>> +	}
>> +
>> +	log_err(&iel, "Downstream port \"%s\" not found\n", devname);
>> +	return NULL;
>> +}
>> +
>> +static struct cxl_memdev *find_cxl_memdev(struct cxl_ctx *ctx,
>> +					  const char *filter)
>> +{
>> +	struct cxl_memdev *memdev;
>> +
>> +	cxl_memdev_foreach(ctx, memdev) {
>> +		if (util_cxl_memdev_filter(memdev, filter, NULL))
>> +			return memdev;
>> +	}
>> +
>> +	log_err(&iel, "Memdev \"%s\" not found\n", filter);
>> +	return NULL;
>> +}
>> +
>> +static int inject_proto_err(struct cxl_ctx *ctx, const char *devname,
>> +			    struct cxl_protocol_error *perror)
>> +{
>> +	struct cxl_dport *dport;
>> +	int rc;
>> +
>> +	if (!devname) {
>> +		log_err(&iel, "No downstream port specified for injection\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dport = find_cxl_dport(ctx, devname);
>> +	if (!dport)
>> +		return -ENODEV;
>> +
>> +	rc = cxl_dport_protocol_error_inject(dport,
>> +					     cxl_protocol_error_get_num(perror));
>> +	if (rc)
>> +		return rc;
>> +
>> +	printf("injected %s protocol error.\n",
>> +	       cxl_protocol_error_get_str(perror));
> 
> log_info() maybe?

I think I had it as log_info() before, but I don't think it was making it's way to
the console. I think I wanted the console output because I personally don't like running
silent commands. Not a great reason, so I'm fine with changing it if that's the preferred
way.

> 
>> +	return 0;
>> +}
>> +
>> +static int poison_action(struct cxl_ctx *ctx, const char *filter,
>> +			 const char *addr)
>> +{
>> +	struct cxl_memdev *memdev;
>> +	size_t a;
> 
> Maybe rename 'addr' to 'addr_str' and rename 'a' to 'addr'
> 

Sure.

>> +	int rc;
>> +
>> +	memdev = find_cxl_memdev(ctx, filter);
>> +	if (!memdev)
>> +		return -ENODEV;
>> +
>> +	if (!cxl_memdev_has_poison_injection(memdev)) {
>> +		log_err(&iel, "%s does not support error injection\n",
>> +			cxl_memdev_get_devname(memdev));
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!addr) {
>> +		log_err(&iel, "no address provided\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	a = strtoull(addr, NULL, 0);
>> +	if (a == ULLONG_MAX && errno == ERANGE) {
>> +		log_err(&iel, "invalid address %s", addr);
>> +		return -EINVAL;
>> +	}
>> +
>> +	rc = cxl_memdev_inject_poison(memdev, a);
>> +
> 
> unnecessary blank line> +	if (rc)

Will remove!

>> +		log_err(&iel, "failed to inject poison at %s:%s: %s\n",
>> +			cxl_memdev_get_devname(memdev), addr, strerror(-rc));
>> +	else
>> +		printf("poison injected at %s:%s\n",
>> +		       cxl_memdev_get_devname(memdev), addr);
> 
> log_info() maybe?

Same thing as above.

Thanks,
Ben

> 
> DJ
>