[ndctl 0/3] Support for inject poison

[ndctl 0/3] Support for inject poison

[Junhyeok Im]

This series adds new ability to inject poison, including library update
and new command interface(inject-poison).

Junhyeok Im (3):
  libcxl: add memdev inject poison support
  cxl: add inject-poison command to cxl tool
  Documentation: add man page documentation for inject-poison

 Documentation/cxl/cxl-inject-poison.txt | 42 ++++++++++++++++++++
 Documentation/cxl/lib/libcxl.txt        |  4 ++
 Documentation/cxl/meson.build           |  1 +
 cxl/builtin.h                           |  1 +
 cxl/cxl.c                               |  1 +
 cxl/lib/libcxl.c                        | 26 ++++++++++++
 cxl/lib/libcxl.sym                      |  5 +++
 cxl/libcxl.h                            |  1 +
 cxl/memdev.c                            | 53 +++++++++++++++++++++++--
 9 files changed, 131 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/cxl/cxl-inject-poison.txt


base-commit: a88bdcfb4202c73aadfee6f83c5502eb5121cbd9
-- 
2.34.1

[ndctl 1/3] libcxl: add memdev inject poison support

[Junhyeok Im]

Add ability to inject poison based on the kernel inject poison sysfs interface.

This interface trigger inject poison function by writing DPA to the memory device
sysfs attribute ('inject_poison').

Link to corresponding kernel patchset:
  https://lore.kernel.org/linux-cxl/cover.1674101475.git.alison.schofield@intel.com/

Signed-off-by: Junhyeok Im <junhyeok.im@samsung.com>
---
 cxl/lib/libcxl.c   | 26 ++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  5 +++++
 cxl/libcxl.h       |  1 +
 3 files changed, 32 insertions(+)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 4859bd5..d2b8e19 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1407,6 +1407,32 @@ CXL_EXPORT int cxl_memdev_enable(struct cxl_memdev *memdev)
 	return 0;
 }
 
+CXL_EXPORT int cxl_memdev_inject_poison(struct cxl_memdev *memdev,
+					const char *address)
+{
+	struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
+	char *path = memdev->dev_buf;
+	int len = memdev->buf_len, rc;
+
+	if (snprintf(path, len, "%s/inject_poison",
+		     memdev->dev_path) >= len) {
+		err(ctx, "%s: buffer too small\n",
+		    cxl_memdev_get_devname(memdev));
+		return -ENXIO;
+	}
+	rc = sysfs_write_attr(ctx, path, address);
+	if (rc < 0) {
+		fprintf(stderr, "%s: Failed write sysfs attr inject_poison\n",
+			cxl_memdev_get_devname(memdev));
+		return rc;
+	}
+
+	dbg(ctx, "%s: poison injected at %s\n", cxl_memdev_get_devname(memdev),
+	    address);
+
+	return 0;
+}
+
 static struct cxl_endpoint *cxl_port_find_endpoint(struct cxl_port *parent_port,
 						   struct cxl_memdev *memdev)
 {
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 6bc0810..c6ba248 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -242,3 +242,8 @@ global:
 	cxl_target_get_firmware_node;
 	cxl_dport_get_firmware_node;
 } LIBCXL_3;
+
+LIBCXL_5 {
+global:
+        cxl_memdev_inject_poison;
+} LIBCXL_4;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index d699af8..7a2cc5f 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -68,6 +68,7 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
 		size_t offset);
 int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
 		size_t offset);
+int cxl_memdev_inject_poison(struct cxl_memdev *memdev, const char *address);
 
 #define cxl_memdev_foreach(ctx, memdev) \
         for (memdev = cxl_memdev_get_first(ctx); \
-- 
2.34.1

Re: [ndctl 1/3] libcxl: add memdev inject poison support

[Alison Schofield]

Commit message needs to include PATCH for all of these.
ie [ndctl PATCH 1/3]

On Mon, Feb 20, 2023 at 01:57:07PM +0900, Junhyeok Im wrote:
> Add ability to inject poison based on the kernel inject poison sysfs interface.
> 
> This interface trigger inject poison function by writing DPA to the memory device
> sysfs attribute ('inject_poison').

I'm not so sure about no validation on that address, but I'll comment on
that in the next patch.

Alison

> 
> Link to corresponding kernel patchset:
>   https://lore.kernel.org/linux-cxl/cover.1674101475.git.alison.schofield@intel.com/
> 
> Signed-off-by: Junhyeok Im <junhyeok.im@samsung.com>
> ---
>  cxl/lib/libcxl.c   | 26 ++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  5 +++++
>  cxl/libcxl.h       |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 4859bd5..d2b8e19 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1407,6 +1407,32 @@ CXL_EXPORT int cxl_memdev_enable(struct cxl_memdev *memdev)
>  	return 0;
>  }
>  
> +CXL_EXPORT int cxl_memdev_inject_poison(struct cxl_memdev *memdev,
> +					const char *address)
> +{
> +	struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
> +	char *path = memdev->dev_buf;
> +	int len = memdev->buf_len, rc;
> +
> +	if (snprintf(path, len, "%s/inject_poison",
> +		     memdev->dev_path) >= len) {
> +		err(ctx, "%s: buffer too small\n",
> +		    cxl_memdev_get_devname(memdev));
> +		return -ENXIO;
> +	}

git clang-format suggests this reformatting:
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 08ee9e61c4ca..8d8a01330891 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1459,8 +1459,7 @@ CXL_EXPORT int cxl_memdev_inject_poison(struct cxl_memdev *memdev,
        char *path = memdev->dev_buf;
        int len = memdev->buf_len, rc;

-       if (snprintf(path, len, "%s/inject_poison",
-                    memdev->dev_path) >= len) {
+       if (snprintf(path, len, "%s/inject_poison", memdev->dev_path) >= len) {
                err(ctx, "%s: buffer too small\n",
                    cxl_memdev_get_devname(memdev));
                return -ENXIO;


> +	rc = sysfs_write_attr(ctx, path, address);
> +	if (rc < 0) {
> +		fprintf(stderr, "%s: Failed write sysfs attr inject_poison\n",
> +			cxl_memdev_get_devname(memdev));
> +		return rc;
> +	}
> +
> +	dbg(ctx, "%s: poison injected at %s\n", cxl_memdev_get_devname(memdev),
> +	    address);
> +
> +	return 0;
> +}
> +
>  static struct cxl_endpoint *cxl_port_find_endpoint(struct cxl_port *parent_port,
>  						   struct cxl_memdev *memdev)
>  {
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 6bc0810..c6ba248 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -242,3 +242,8 @@ global:
>  	cxl_target_get_firmware_node;
>  	cxl_dport_get_firmware_node;
>  } LIBCXL_3;
> +
> +LIBCXL_5 {
> +global:
> +        cxl_memdev_inject_poison;
> +} LIBCXL_4;
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index d699af8..7a2cc5f 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -68,6 +68,7 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
>  		size_t offset);
>  int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
>  		size_t offset);
> +int cxl_memdev_inject_poison(struct cxl_memdev *memdev, const char *address);
>  
>  #define cxl_memdev_foreach(ctx, memdev) \
>          for (memdev = cxl_memdev_get_first(ctx); \
> -- 
> 2.34.1
> 

[ndctl 2/3] cxl: add inject-poison command to cxl tool

[Junhyeok Im]

Add new command to cli tool, to inject poison into dpa(-a) on the
memory device.

DPA written in sysfs attribute(inject_poison) is converted by
kstrtou64 with 0 base by 'inject_poison_store' of CXL driver, so if
it begins with 0x the number will be parsed as a hexadecimal
(case insensitive), if it otherwise begins with 0, it will be parsed
as an octal number. Otherwise it will be parsed as a decimal.

Since the validity verification of the dpa would be done in
'cxl_validate_poison_dpa' of CXL driver, no additional logic
is added.

Also since it is expected no use case of injecting poison into the
same address for multiple devices, this command targets only one
memdev, like write-labels command.

 usage: cxl inject-poison <memdev> -a <dpa> [<options>]

    -v, --verbose         turn on debug
    -S, --serial          use serial numbers to id memdevs
    -a, --address <dpa>   DPA to inject poison

Link to corresponding kernel patch:
  https://patchwork.kernel.org/project/cxl/patch/97a0b128d0d0df56cea1a1a4ead65a40b9cf008e.1674101475.git.alison.schofield@intel.com/

Signed-off-by: Junhyeok Im <junhyeok.im@samsung.com>
---
 cxl/builtin.h |  1 +
 cxl/cxl.c     |  1 +
 cxl/memdev.c  | 53 ++++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/cxl/builtin.h b/cxl/builtin.h
index 34c5cfb..ddc4da9 100644
--- a/cxl/builtin.h
+++ b/cxl/builtin.h
@@ -23,4 +23,5 @@ 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_monitor(int argc, const char **argv, struct cxl_ctx *ctx);
+int cmd_inject_poison(int argc, const char **argv, struct cxl_ctx *ctx);
 #endif /* _CXL_BUILTIN_H_ */
diff --git a/cxl/cxl.c b/cxl/cxl.c
index 3be7026..aa8d090 100644
--- a/cxl/cxl.c
+++ b/cxl/cxl.c
@@ -77,6 +77,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-poison", .c_fn = cmd_inject_poison },
 };
 
 int main(int argc, const char **argv)
diff --git a/cxl/memdev.c b/cxl/memdev.c
index 0b3ad02..7a10f79 100644
--- a/cxl/memdev.c
+++ b/cxl/memdev.c
@@ -34,6 +34,7 @@ static struct parameters {
 	const char *type;
 	const char *size;
 	const char *decoder_filter;
+	const char *poison_address;
 } param;
 
 static struct log_ctx ml;
@@ -85,6 +86,10 @@ OPT_STRING('t', "type", &param.type, "type",                   \
 OPT_BOOLEAN('f', "force", &param.force,                        \
 	    "Attempt 'expected to fail' operations")
 
+#define INJECT_POISON_OPTIONS()				\
+OPT_STRING('a', "address", &param.poison_address, "dpa",	\
+	   "DPA to inject poison")
+
 static const struct option read_options[] = {
 	BASE_OPTIONS(),
 	LABEL_OPTIONS(),
@@ -135,6 +140,12 @@ static const struct option free_dpa_options[] = {
 	OPT_END(),
 };
 
+static const struct option inject_poison_options[] = {
+	BASE_OPTIONS(),
+	INJECT_POISON_OPTIONS(),
+	OPT_END(),
+};
+
 enum reserve_dpa_mode {
 	DPA_ALLOC,
 	DPA_FREE,
@@ -351,6 +362,24 @@ static int action_free_dpa(struct cxl_memdev *memdev,
 	return __reserve_dpa(memdev, DPA_FREE, actx);
 }
 
+static int action_inject_poison(struct cxl_memdev *memdev,
+				struct action_context *actx)
+{
+	int rc;
+
+	if (!param.poison_address) {
+		log_err(&ml, "%s: set dpa to inject poison.\n",
+			cxl_memdev_get_devname(memdev));
+		return -EINVAL;
+	}
+	rc = cxl_memdev_inject_poison(memdev, param.poison_address);
+	if (rc < 0) {
+		log_err(&ml, "%s: inject poison failed: %s\n",
+			cxl_memdev_get_devname(memdev), strerror(-rc));
+	}
+	return rc;
+}
+
 static int action_disable(struct cxl_memdev *memdev, struct action_context *actx)
 {
 	if (!cxl_memdev_is_enabled(memdev))
@@ -755,7 +784,8 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
 				continue;
 			found = true;
 
-			if (action == action_write) {
+			if ((action == action_write) ||
+			    (action == action_inject_poison)) {
 				single = memdev;
 				rc = 0;
 			} else
@@ -771,9 +801,15 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
 	}
 	rc = err;
 
-	if (action == action_write) {
+	if ((action == action_write) || (action == action_inject_poison)) {
 		if (count > 1) {
-			error("write-labels only supports writing a single memdev\n");
+			if (action == action_write) {
+				error("write-labels only supports writing "
+				      "a single memdev\n");
+			} else {
+				error("inject-poison only supports injection "
+				      "of poison into a single memdev\n");
+			}
 			usage_with_options(u, options);
 			return -EINVAL;
 		} else if (single) {
@@ -893,3 +929,14 @@ int cmd_free_dpa(int argc, const char **argv, struct cxl_ctx *ctx)
 
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
+
+int cmd_inject_poison(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	int count = memdev_action(
+		argc, argv, ctx, action_inject_poison, inject_poison_options,
+		"cxl inject-poison <memdev> -a <dpa> [<options>]");
+	log_info(&ml, "inject-poison %d mem%s\n", count >= 0 ? count : 0,
+		 count > 1 ? "s" : "");
+
+	return count >= 0 ? 0 : EXIT_FAILURE;
+}
-- 
2.34.1

Re: [ndctl 2/3] cxl: add inject-poison command to cxl tool

[Alison Schofield]

On Mon, Feb 20, 2023 at 01:57:08PM +0900, Junhyeok Im wrote:
> Add new command to cli tool, to inject poison into dpa(-a) on the
> memory device.
> 
> DPA written in sysfs attribute(inject_poison) is converted by
> kstrtou64 with 0 base by 'inject_poison_store' of CXL driver, so if
> it begins with 0x the number will be parsed as a hexadecimal
> (case insensitive), if it otherwise begins with 0, it will be parsed
> as an octal number. Otherwise it will be parsed as a decimal.
> 
> Since the validity verification of the dpa would be done in
> 'cxl_validate_poison_dpa' of CXL driver, no additional logic
> is added.

I'm not sure that zero validity checks on that DPA is best here.
That validity checks in the driver always returns -EINVAL, and
the driver emits a dev_dbg message offering more details. So, for
a user that is not running a debug kernel, figuring out which
validity check they failed is not so trivial.

The driver fails the validity check for these reasons:

device has no dpa resource
dpa in resource
dpa is not 64-byte aligned
dpa mapped in region

I lean towards the cxl command checking all of these, but I'm not
clear of the precedence here, so let's see what others say.

> 
> Also since it is expected no use case of injecting poison into the
> same address for multiple devices, this command targets only one
> memdev, like write-labels command.
> 
>  usage: cxl inject-poison <memdev> -a <dpa> [<options>]

maybe -m memdev (to be like others)
> 
>     -v, --verbose         turn on debug
>     -S, --serial          use serial numbers to id memdevs
>     -a, --address <dpa>   DPA to inject poison

Let's open up the naming discussion again.  This isn't solely about
your patch here. With the cxl list --media-errors patch, Jonathan and I
had a discussion about whether that should be --poison or --media-errors
and stuck with --media-errors to align with the CXL spec and be like ndctl.

Our prior thoughts:
https://lore.kernel.org/nvdimm/20221121105711.0000770c@Huawei.com/

Note that this is all pending work, nothing has been merged. I started
using 'poison' for the sysfs attributes: trigger_poison_list, inject_poison,
clear_posion.

But, in the CXL tool, I went with 'media-errors', cxl list --media-errors.

Following that pattern, s/inject-poison/inject-media-error  

I'd like to revisit that, because now it seems like less of a match as
we grow to include inject_poison and clear_poison.

> 
> Link to corresponding kernel patch:
>   https://patchwork.kernel.org/project/cxl/patch/97a0b128d0d0df56cea1a1a4ead65a40b9cf008e.1674101475.git.alison.schofield@intel.com/
> 
> Signed-off-by: Junhyeok Im <junhyeok.im@samsung.com>
> ---
>  cxl/builtin.h |  1 +
>  cxl/cxl.c     |  1 +
>  cxl/memdev.c  | 53 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/cxl/builtin.h b/cxl/builtin.h
> index 34c5cfb..ddc4da9 100644
> --- a/cxl/builtin.h
> +++ b/cxl/builtin.h
> @@ -23,4 +23,5 @@ 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_monitor(int argc, const char **argv, struct cxl_ctx *ctx);
> +int cmd_inject_poison(int argc, const char **argv, struct cxl_ctx *ctx);
>  #endif /* _CXL_BUILTIN_H_ */
> diff --git a/cxl/cxl.c b/cxl/cxl.c
> index 3be7026..aa8d090 100644
> --- a/cxl/cxl.c
> +++ b/cxl/cxl.c
> @@ -77,6 +77,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-poison", .c_fn = cmd_inject_poison },
>  };
>  
>  int main(int argc, const char **argv)
> diff --git a/cxl/memdev.c b/cxl/memdev.c
> index 0b3ad02..7a10f79 100644
> --- a/cxl/memdev.c
> +++ b/cxl/memdev.c
> @@ -34,6 +34,7 @@ static struct parameters {
>  	const char *type;
>  	const char *size;
>  	const char *decoder_filter;
> +	const char *poison_address;
>  } param;
>  
>  static struct log_ctx ml;
> @@ -85,6 +86,10 @@ OPT_STRING('t', "type", &param.type, "type",                   \
>  OPT_BOOLEAN('f', "force", &param.force,                        \
>  	    "Attempt 'expected to fail' operations")
>  
> +#define INJECT_POISON_OPTIONS()				\
> +OPT_STRING('a', "address", &param.poison_address, "dpa",	\
> +	   "DPA to inject poison")
> +

git clang-format doesn't like the above.

-#define INJECT_POISON_OPTIONS()                                \
-OPT_STRING('a', "address", &param.poison_address, "dpa",       \
-          "DPA to inject poison")
+#define INJECT_POISON_OPTIONS()                                  \
+       OPT_STRING('a', "address", &param.poison_address, "dpa", \
+                  "DPA to inject poison")



>  static const struct option read_options[] = {
>  	BASE_OPTIONS(),
>  	LABEL_OPTIONS(),
> @@ -135,6 +140,12 @@ static const struct option free_dpa_options[] = {
>  	OPT_END(),
>  };
>  
> +static const struct option inject_poison_options[] = {
> +	BASE_OPTIONS(),
> +	INJECT_POISON_OPTIONS(),
> +	OPT_END(),
> +};
> +
>  enum reserve_dpa_mode {
>  	DPA_ALLOC,
>  	DPA_FREE,
> @@ -351,6 +362,24 @@ static int action_free_dpa(struct cxl_memdev *memdev,
>  	return __reserve_dpa(memdev, DPA_FREE, actx);
>  }
>  
> +static int action_inject_poison(struct cxl_memdev *memdev,
> +				struct action_context *actx)
> +{
> +	int rc;
> +
> +	if (!param.poison_address) {
> +		log_err(&ml, "%s: set dpa to inject poison.\n",
> +			cxl_memdev_get_devname(memdev));
> +		return -EINVAL;
> +	}
> +	rc = cxl_memdev_inject_poison(memdev, param.poison_address);
> +	if (rc < 0) {
> +		log_err(&ml, "%s: inject poison failed: %s\n",
> +			cxl_memdev_get_devname(memdev), strerror(-rc));
> +	}
> +	return rc;
> +}
> +
>  static int action_disable(struct cxl_memdev *memdev, struct action_context *actx)
>  {
>  	if (!cxl_memdev_is_enabled(memdev))
> @@ -755,7 +784,8 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  				continue;
>  			found = true;
>  
> -			if (action == action_write) {
> +			if ((action == action_write) ||
> +			    (action == action_inject_poison)) {
>  				single = memdev;
>  				rc = 0;
>  			} else
> @@ -771,9 +801,15 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  	}
>  	rc = err;
>  
> -	if (action == action_write) {
> +	if ((action == action_write) || (action == action_inject_poison)) {
>  		if (count > 1) {
> -			error("write-labels only supports writing a single memdev\n");
> +			if (action == action_write) {
> +				error("write-labels only supports writing "
> +				      "a single memdev\n");
> +			} else {
> +				error("inject-poison only supports injection "
> +				      "of poison into a single memdev\n");
> +			}
>  			usage_with_options(u, options);
>  			return -EINVAL;
>  		} else if (single) {
> @@ -893,3 +929,14 @@ int cmd_free_dpa(int argc, const char **argv, struct cxl_ctx *ctx)
>  
>  	return count >= 0 ? 0 : EXIT_FAILURE;
>  }
> +
> +int cmd_inject_poison(int argc, const char **argv, struct cxl_ctx *ctx)
> +{
> +	int count = memdev_action(
> +		argc, argv, ctx, action_inject_poison, inject_poison_options,
> +		"cxl inject-poison <memdev> -a <dpa> [<options>]");
> +	log_info(&ml, "inject-poison %d mem%s\n", count >= 0 ? count : 0,
> +		 count > 1 ? "s" : "");
> +
> +	return count >= 0 ? 0 : EXIT_FAILURE;
> +}
> -- 
> 2.34.1
> 

Re: [ndctl 2/3] cxl: add inject-poison command to cxl tool

[Junhyeok Im]

[-- Attachment #1: Type: text/plain, Size: 9037 bytes --]

On Sun, Feb 26, 2023 at 07:21:44PM -0800, Alison Schofield wrote:
> On Mon, Feb 20, 2023 at 01:57:08PM +0900, Junhyeok Im wrote:
> > Add new command to cli tool, to inject poison into dpa(-a) on the
> > memory device.
> > 
> > DPA written in sysfs attribute(inject_poison) is converted by
> > kstrtou64 with 0 base by 'inject_poison_store' of CXL driver, so if
> > it begins with 0x the number will be parsed as a hexadecimal
> > (case insensitive), if it otherwise begins with 0, it will be parsed
> > as an octal number. Otherwise it will be parsed as a decimal.
> > 
> > Since the validity verification of the dpa would be done in
> > 'cxl_validate_poison_dpa' of CXL driver, no additional logic
> > is added.
> 
> I'm not sure that zero validity checks on that DPA is best here.
> That validity checks in the driver always returns -EINVAL, and
> the driver emits a dev_dbg message offering more details. So, for
> a user that is not running a debug kernel, figuring out which
> validity check they failed is not so trivial.
> 
> The driver fails the validity check for these reasons:
> 
> device has no dpa resource
> dpa in resource
> dpa is not 64-byte aligned
> dpa mapped in region
> 
> I lean towards the cxl command checking all of these, but I'm not
> clear of the precedence here, so let's see what others say.
> 

Thank you for sharing your thoughts.
Then I will also wait for others opinion for now.

> > 
> > Also since it is expected no use case of injecting poison into the
> > same address for multiple devices, this command targets only one
> > memdev, like write-labels command.
> > 
> >  usage: cxl inject-poison <memdev> -a <dpa> [<options>]
> 
> maybe -m memdev (to be like others)

AFAIK, memdev is to be specified without "-m" except for 'list' cmd,
but I think the reason you said is to distinguish memdev from region(e.g."-r").
(I also found in your previous patch for GET_POISON_LIST that 
memdev and region were separated.)
If so, codes and documentation should be modified like:
    cxl inject-poison -m <memdev> -a <dpa> [<options>]
Thanks.
> > 
> >     -v, --verbose         turn on debug
> >     -S, --serial          use serial numbers to id memdevs
> >     -a, --address <dpa>   DPA to inject poison
> 
> Let's open up the naming discussion again.  This isn't solely about
> your patch here. With the cxl list --media-errors patch, Jonathan and I
> had a discussion about whether that should be --poison or --media-errors
> and stuck with --media-errors to align with the CXL spec and be like ndctl.
> 
> Our prior thoughts:
> https://lore.kernel.org/nvdimm/20221121105711.0000770c@Huawei.com/
> 
> Note that this is all pending work, nothing has been merged. I started
> using 'poison' for the sysfs attributes: trigger_poison_list, inject_poison,
> clear_posion.
> 
> But, in the CXL tool, I went with 'media-errors', cxl list --media-errors.
> 
> Following that pattern, s/inject-poison/inject-media-error  
> 
> I'd like to revisit that, because now it seems like less of a match as
> we grow to include inject_poison and clear_poison.

I agree that naming should be changed and unified if needed.
Personally I do not think it's awkward to use the name 'poison' 
seperately from general 'errors' to indicate that it's inserted artificially.
But still a concern is that spec calls them "Media Error Records", like your
previous comments above.

> 
> > 
> > Link to corresponding kernel patch:
> >   https://patchwork.kernel.org/project/cxl/patch/97a0b128d0d0df56cea1a1a4ead65a40b9cf008e.1674101475.git.alison.schofield@intel.com/
> > 
> > Signed-off-by: Junhyeok Im <junhyeok.im@samsung.com>
> > ---
> >  cxl/builtin.h |  1 +
> >  cxl/cxl.c     |  1 +
> >  cxl/memdev.c  | 53 ++++++++++++++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 52 insertions(+), 3 deletions(-)
> > 
> > diff --git a/cxl/builtin.h b/cxl/builtin.h
> > index 34c5cfb..ddc4da9 100644
> > --- a/cxl/builtin.h
> > +++ b/cxl/builtin.h
> > @@ -23,4 +23,5 @@ 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_monitor(int argc, const char **argv, struct cxl_ctx *ctx);
> > +int cmd_inject_poison(int argc, const char **argv, struct cxl_ctx *ctx);
> >  #endif /* _CXL_BUILTIN_H_ */
> > diff --git a/cxl/cxl.c b/cxl/cxl.c
> > index 3be7026..aa8d090 100644
> > --- a/cxl/cxl.c
> > +++ b/cxl/cxl.c
> > @@ -77,6 +77,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-poison", .c_fn = cmd_inject_poison },
> >  };
> >  
> >  int main(int argc, const char **argv)
> > diff --git a/cxl/memdev.c b/cxl/memdev.c
> > index 0b3ad02..7a10f79 100644
> > --- a/cxl/memdev.c
> > +++ b/cxl/memdev.c
> > @@ -34,6 +34,7 @@ static struct parameters {
> >  	const char *type;
> >  	const char *size;
> >  	const char *decoder_filter;
> > +	const char *poison_address;
> >  } param;
> >  
> >  static struct log_ctx ml;
> > @@ -85,6 +86,10 @@ OPT_STRING('t', "type", &param.type, "type",                   \
> >  OPT_BOOLEAN('f', "force", &param.force,                        \
> >  	    "Attempt 'expected to fail' operations")
> >  
> > +#define INJECT_POISON_OPTIONS()				\
> > +OPT_STRING('a', "address", &param.poison_address, "dpa",	\
> > +	   "DPA to inject poison")
> > +
> 
> git clang-format doesn't like the above.
> 
> -#define INJECT_POISON_OPTIONS()                                \
> -OPT_STRING('a', "address", &param.poison_address, "dpa",       \
> -          "DPA to inject poison")
> +#define INJECT_POISON_OPTIONS()                                  \
> +       OPT_STRING('a', "address", &param.poison_address, "dpa", \
> +                  "DPA to inject poison")
> 
> 
> 

Thank you for check and you're right, clang-format wants a different style 
of macro definition. 
Before defining them, I referred to the other macros in memdev.c
(e.g. DPA_OPTIONS()) and found they were also not indented.
Do you think that we have to modify them all?

> >  static const struct option read_options[] = {
> >  	BASE_OPTIONS(),
> >  	LABEL_OPTIONS(),
> > @@ -135,6 +140,12 @@ static const struct option free_dpa_options[] = {
> >  	OPT_END(),
> >  };
> >  
> > +static const struct option inject_poison_options[] = {
> > +	BASE_OPTIONS(),
> > +	INJECT_POISON_OPTIONS(),
> > +	OPT_END(),
> > +};
> > +
> >  enum reserve_dpa_mode {
> >  	DPA_ALLOC,
> >  	DPA_FREE,
> > @@ -351,6 +362,24 @@ static int action_free_dpa(struct cxl_memdev *memdev,
> >  	return __reserve_dpa(memdev, DPA_FREE, actx);
> >  }
> >  
> > +static int action_inject_poison(struct cxl_memdev *memdev,
> > +				struct action_context *actx)
> > +{
> > +	int rc;
> > +
> > +	if (!param.poison_address) {
> > +		log_err(&ml, "%s: set dpa to inject poison.\n",
> > +			cxl_memdev_get_devname(memdev));
> > +		return -EINVAL;
> > +	}
> > +	rc = cxl_memdev_inject_poison(memdev, param.poison_address);
> > +	if (rc < 0) {
> > +		log_err(&ml, "%s: inject poison failed: %s\n",
> > +			cxl_memdev_get_devname(memdev), strerror(-rc));
> > +	}
> > +	return rc;
> > +}
> > +
> >  static int action_disable(struct cxl_memdev *memdev, struct action_context *actx)
> >  {
> >  	if (!cxl_memdev_is_enabled(memdev))
> > @@ -755,7 +784,8 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
> >  				continue;
> >  			found = true;
> >  
> > -			if (action == action_write) {
> > +			if ((action == action_write) ||
> > +			    (action == action_inject_poison)) {
> >  				single = memdev;
> >  				rc = 0;
> >  			} else
> > @@ -771,9 +801,15 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
> >  	}
> >  	rc = err;
> >  
> > -	if (action == action_write) {
> > +	if ((action == action_write) || (action == action_inject_poison)) {
> >  		if (count > 1) {
> > -			error("write-labels only supports writing a single memdev\n");
> > +			if (action == action_write) {
> > +				error("write-labels only supports writing "
> > +				      "a single memdev\n");
> > +			} else {
> > +				error("inject-poison only supports injection "
> > +				      "of poison into a single memdev\n");
> > +			}
> >  			usage_with_options(u, options);
> >  			return -EINVAL;
> >  		} else if (single) {
> > @@ -893,3 +929,14 @@ int cmd_free_dpa(int argc, const char **argv, struct cxl_ctx *ctx)
> >  
> >  	return count >= 0 ? 0 : EXIT_FAILURE;
> >  }
> > +
> > +int cmd_inject_poison(int argc, const char **argv, struct cxl_ctx *ctx)
> > +{
> > +	int count = memdev_action(
> > +		argc, argv, ctx, action_inject_poison, inject_poison_options,
> > +		"cxl inject-poison <memdev> -a <dpa> [<options>]");
> > +	log_info(&ml, "inject-poison %d mem%s\n", count >= 0 ? count : 0,
> > +		 count > 1 ? "s" : "");
> > +
> > +	return count >= 0 ? 0 : EXIT_FAILURE;
> > +}
> > -- 
> > 2.34.1
> > 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ndctl 2/3] cxl: add inject-poison command to cxl tool

[Verma, Vishal L]

On Tue, 2023-02-28 at 18:43 +0900, Junhyeok Im wrote:
> On Sun, Feb 26, 2023 at 07:21:44PM -0800, Alison Schofield wrote:
> 
> 
> > 
<..>

> > > Also since it is expected no use case of injecting poison into the
> > > same address for multiple devices, this command targets only one
> > > memdev, like write-labels command.
> > > 
> > >  usage: cxl inject-poison <memdev> -a <dpa> [<options>]
> > 
> > maybe -m memdev (to be like others)
> 
> AFAIK, memdev is to be specified without "-m" except for 'list' cmd,
> but I think the reason you said is to distinguish memdev from region(e.g."-r").
> (I also found in your previous patch for GET_POISON_LIST that 
> memdev and region were separated.)
> If so, codes and documentation should be modified like:
>     cxl inject-poison -m <memdev> -a <dpa> [<options>]

Just wanted to chime in on this quickly before having gone through the
rest of the series -

The convention has been if a command can only have one type of
'target', then no need for a -m, or -r etc.

e.g. cxl enable-region region0

But if it can (potentially) have different types of targets, then an
option can be used to determine the target type. e.g. cxl-create-region
was originally intending to allow both memdev and endpoint targets,
hence the -m option to say that the non-option arguments should be
treated as memdevs.

The other place where you'd have options like -m="mem0,mem1" etc is for
filtering or restricting an operation. cxl-list uses the different
options to filter results, but also other commands:

  e.g. cxl disable-region --bus=cxl_test all

will disable all regions under cxl_test, but leave alone regions under
another bus.

Re: [ndctl 2/3] cxl: add inject-poison command to cxl tool

[Alison Schofield]

On Mon, Feb 20, 2023 at 01:57:08PM +0900, Junhyeok Im wrote:
> Add new command to cli tool, to inject poison into dpa(-a) on the
> memory device.
> 
> DPA written in sysfs attribute(inject_poison) is converted by
> kstrtou64 with 0 base by 'inject_poison_store' of CXL driver, so if
> it begins with 0x the number will be parsed as a hexadecimal
> (case insensitive), if it otherwise begins with 0, it will be parsed
> as an octal number. Otherwise it will be parsed as a decimal.
> 
> Since the validity verification of the dpa would be done in
> 'cxl_validate_poison_dpa' of CXL driver, no additional logic
> is added.
> 
> Also since it is expected no use case of injecting poison into the
> same address for multiple devices, this command targets only one
> memdev, like write-labels command.
> 
>  usage: cxl inject-poison <memdev> -a <dpa> [<options>]
> 
>     -v, --verbose         turn on debug
>     -S, --serial          use serial numbers to id memdevs
>     -a, --address <dpa>   DPA to inject poison
> 

Upon successful completion of the command, display the --media-errors
list for that device.

<snip>

Re: [ndctl 2/3] cxl: add inject-poison command to cxl tool

[Junhyeok Im]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

On Sun, Feb 26, 2023 at 07:25:50PM -0800, Alison Schofield wrote:
> On Mon, Feb 20, 2023 at 01:57:08PM +0900, Junhyeok Im wrote:
> > Add new command to cli tool, to inject poison into dpa(-a) on the
> > memory device.
> > 
> > DPA written in sysfs attribute(inject_poison) is converted by
> > kstrtou64 with 0 base by 'inject_poison_store' of CXL driver, so if
> > it begins with 0x the number will be parsed as a hexadecimal
> > (case insensitive), if it otherwise begins with 0, it will be parsed
> > as an octal number. Otherwise it will be parsed as a decimal.
> > 
> > Since the validity verification of the dpa would be done in
> > 'cxl_validate_poison_dpa' of CXL driver, no additional logic
> > is added.
> > 
> > Also since it is expected no use case of injecting poison into the
> > same address for multiple devices, this command targets only one
> > memdev, like write-labels command.
> > 
> >  usage: cxl inject-poison <memdev> -a <dpa> [<options>]
> > 
> >     -v, --verbose         turn on debug
> >     -S, --serial          use serial numbers to id memdevs
> >     -a, --address <dpa>   DPA to inject poison
> > 
> 
> Upon successful completion of the command, display the --media-errors
> list for that device.
> 
> <snip>
> 

So, your comment is that the media-errors list (like $ cxl list --media-erros)
should be printed after command completion. 
I agree in terms of the completeness of the command operation. 
Thank you for your good opinion. I'll make up for it.


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[ndctl 3/3] Documentation: add man page documentation for inject-poison

[Junhyeok Im]

Add man page documentation for inject-poison command in cxl,
also add the content of cxl_memdev_inject_poison in libcxl.

Signed-off-by: Junhyeok Im <junhyeok.im@samsung.com>
---
 Documentation/cxl/cxl-inject-poison.txt | 42 +++++++++++++++++++++++++
 Documentation/cxl/lib/libcxl.txt        |  4 +++
 Documentation/cxl/meson.build           |  1 +
 3 files changed, 47 insertions(+)
 create mode 100644 Documentation/cxl/cxl-inject-poison.txt

diff --git a/Documentation/cxl/cxl-inject-poison.txt b/Documentation/cxl/cxl-inject-poison.txt
new file mode 100644
index 0000000..d438d33
--- /dev/null
+++ b/Documentation/cxl/cxl-inject-poison.txt
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-inject-poison(1)
+====================
+
+NAME
+----
+cxl-inject-poison - send inject poison command to specified CXL memdev
+                    targeting given DPA.
+
+SYNOPSIS
+--------
+[verse]
+'cxl inject-poison <memdev> -a <dpa> [<options>]'
+
+DESCRIPTION
+-----------
+Add the <dpa> to the <memdev> poison list and the error source shall be set
+to an injected error. In addition, the device shall add an appropriate poison
+creation event to its internal Informational Event Log, update the Event Status
+register, and if configured, interrupt the host.
+
+OPTIONS
+-------
+<memory device>::
+        A 'memX' device name, or a memdev id number. Restrict the operation to
+        the specified memdev.
+
+-a::
+--address=::
+        Physical address of a CXL memdev to inject poison into.
+
+-S::
+--serial=::
+        Specify CXL memory device serial number(s) to filter the listing
+
+include::verbose-option.txt[]
+
+
+SEE ALSO
+--------
+CXL-2.0 8.2.9.5.4.2
diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
index f9af376..95f5662 100644
--- a/Documentation/cxl/lib/libcxl.txt
+++ b/Documentation/cxl/lib/libcxl.txt
@@ -132,6 +132,7 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
 			  size_t offset);
 int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
 			   size_t offset);
+int cxl_memdev_inject_poison(struct cxl_memdev *memdev, const char *address);
 struct cxl_cmd *cxl_cmd_new_get_partition(struct cxl_memdev *memdev);
 struct cxl_cmd *cxl_cmd_new_set_partition(struct cxl_memdev *memdev,
 					  unsigned long long volatile_size);
@@ -172,6 +173,9 @@ cxl_memdev{read,write,zero}_label() are helpers for marshaling multiple
 label access commands over an arbitrary extent of the device's label
 area.
 
+cxl_memdev_inject_poison supports injecting poison into a physical address
+on a specified CXL memory device.
+
 cxl_cmd_partition_set_mode() supports selecting NEXTBOOT or IMMEDIATE
 mode. When CXL_SETPART_IMMEDIATE mode is set, it is the caller’s
 responsibility to avoid immediate changes to partitioning when the
diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build
index a6d77ab..a972467 100644
--- a/Documentation/cxl/meson.build
+++ b/Documentation/cxl/meson.build
@@ -46,6 +46,7 @@ cxl_manpages = [
   'cxl-enable-region.txt',
   'cxl-destroy-region.txt',
   'cxl-monitor.txt',
+  'cxl-inject-poison.txt',
 ]
 
 foreach man : cxl_manpages
-- 
2.34.1

Re: [ndctl 0/3] Support for inject poison

[Alison Schofield]

On Mon, Feb 20, 2023 at 01:57:06PM +0900, Junhyeok Im wrote:
> This series adds new ability to inject poison, including library update
> and new command interface(inject-poison).

Hi Junhyeok,
Nice to see this set, and sorry for the delay in trying it out. I'm
using it now while prepping another version of the driver support 
for inject and clear.

> 
> Junhyeok Im (3):
>   libcxl: add memdev inject poison support
>   cxl: add inject-poison command to cxl tool
>   Documentation: add man page documentation for inject-poison

The libcxl.sym and libclx.txt changes in the 3rd patch go better
in the 1st patch that adds the libcxl support.

> 
>  Documentation/cxl/cxl-inject-poison.txt | 42 ++++++++++++++++++++
>  Documentation/cxl/lib/libcxl.txt        |  4 ++
>  Documentation/cxl/meson.build           |  1 +
>  cxl/builtin.h                           |  1 +
>  cxl/cxl.c                               |  1 +
>  cxl/lib/libcxl.c                        | 26 ++++++++++++
>  cxl/lib/libcxl.sym                      |  5 +++
>  cxl/libcxl.h                            |  1 +
>  cxl/memdev.c                            | 53 +++++++++++++++++++++++--
>  9 files changed, 131 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/cxl/cxl-inject-poison.txt
> 
> 
> base-commit: a88bdcfb4202c73aadfee6f83c5502eb5121cbd9
> -- 
> 2.34.1
> 

Re: [ndctl 0/3] Support for inject poison

[Junhyeok Im]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On Sun, Feb 26, 2023 at 06:38:21PM -0800, Alison Schofield wrote:
> On Mon, Feb 20, 2023 at 01:57:06PM +0900, Junhyeok Im wrote:
> > This series adds new ability to inject poison, including library update
> > and new command interface(inject-poison).
> 
> Hi Junhyeok,
> Nice to see this set, and sorry for the delay in trying it out. I'm
> using it now while prepping another version of the driver support 
> for inject and clear.
> 
> > 
> > Junhyeok Im (3):
> >   libcxl: add memdev inject poison support
> >   cxl: add inject-poison command to cxl tool
> >   Documentation: add man page documentation for inject-poison
> 
> The libcxl.sym and libclx.txt changes in the 3rd patch go better
> in the 1st patch that adds the libcxl support.
> 

Thank you Alison for your comments.
Since libcxl.sym changes are already in the 1st patch,
I will include libcxl.txt changes in the 1st patch of next series.

> > 
> >  Documentation/cxl/cxl-inject-poison.txt | 42 ++++++++++++++++++++
> >  Documentation/cxl/lib/libcxl.txt        |  4 ++
> >  Documentation/cxl/meson.build           |  1 +
> >  cxl/builtin.h                           |  1 +
> >  cxl/cxl.c                               |  1 +
> >  cxl/lib/libcxl.c                        | 26 ++++++++++++
> >  cxl/lib/libcxl.sym                      |  5 +++
> >  cxl/libcxl.h                            |  1 +
> >  cxl/memdev.c                            | 53 +++++++++++++++++++++++--
> >  9 files changed, 131 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/cxl/cxl-inject-poison.txt
> > 
> > 
> > base-commit: a88bdcfb4202c73aadfee6f83c5502eb5121cbd9
> > -- 
> > 2.34.1
> > 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ndctl 0/3] Support for inject poison

[Verma, Vishal L]

On Mon, 2023-02-20 at 13:57 +0900, Junhyeok Im wrote:
> This series adds new ability to inject poison, including library update
> and new command interface(inject-poison).

Hi Junhyeok,

With the now queued for the 6.4 kernel, the userspace side of this can
also target the next ndctl release, v78.

I see these patches will at least need a bit of rework to move from the
sysfs interface to the new debugfs interface.

Did you have a new revision of these in progress?

> 
> Junhyeok Im (3):
>   libcxl: add memdev inject poison support
>   cxl: add inject-poison command to cxl tool
>   Documentation: add man page documentation for inject-poison
> 
>  Documentation/cxl/cxl-inject-poison.txt | 42 ++++++++++++++++++++
>  Documentation/cxl/lib/libcxl.txt        |  4 ++
>  Documentation/cxl/meson.build           |  1 +
>  cxl/builtin.h                           |  1 +
>  cxl/cxl.c                               |  1 +
>  cxl/lib/libcxl.c                        | 26 ++++++++++++
>  cxl/lib/libcxl.sym                      |  5 +++
>  cxl/libcxl.h                            |  1 +
>  cxl/memdev.c                            | 53 +++++++++++++++++++++++--
>  9 files changed, 131 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/cxl/cxl-inject-poison.txt
> 
> 
> base-commit: a88bdcfb4202c73aadfee6f83c5502eb5121cbd9

Re: [ndctl 0/3] Support for inject poison

[Junhyeok Im]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

On Mon, May 08, 2023 at 06:39:39PM +0000, Verma, Vishal L wrote:
> On Mon, 2023-02-20 at 13:57 +0900, Junhyeok Im wrote:
> > This series adds new ability to inject poison, including library update
> > and new command interface(inject-poison).
> 
> Hi Junhyeok,
> 
> With the now queued for the 6.4 kernel, the userspace side of this can
> also target the next ndctl release, v78.
> 
> I see these patches will at least need a bit of rework to move from the
> sysfs interface to the new debugfs interface.
> 
> Did you have a new revision of these in progress?
> 

Hi Vishal,

Thank you for confirming.
Yes, I recently found the history including Alison's inject/clear poison patch.
I am now reworking the previous patch including debugfs modification,
and I think I can submit a new patch soon including clear poison interface.

Thanks!

> > 
> > Junhyeok Im (3):
> >   libcxl: add memdev inject poison support
> >   cxl: add inject-poison command to cxl tool
> >   Documentation: add man page documentation for inject-poison
> > 
> >  Documentation/cxl/cxl-inject-poison.txt | 42 ++++++++++++++++++++
> >  Documentation/cxl/lib/libcxl.txt        |  4 ++
> >  Documentation/cxl/meson.build           |  1 +
> >  cxl/builtin.h                           |  1 +
> >  cxl/cxl.c                               |  1 +
> >  cxl/lib/libcxl.c                        | 26 ++++++++++++
> >  cxl/lib/libcxl.sym                      |  5 +++
> >  cxl/libcxl.h                            |  1 +
> >  cxl/memdev.c                            | 53 +++++++++++++++++++++++--
> >  9 files changed, 131 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/cxl/cxl-inject-poison.txt
> > 
> > 
> > base-commit: a88bdcfb4202c73aadfee6f83c5502eb5121cbd9
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[ndctl 1/3] libcxl: add memdev inject poison support

[junhyeok.im]

From: Junhyeok Im <junhyeok.im@samsung.com>

Add ability to inject poison based on the kernel inject poison sysfs interface.

This interface trigger inject poison function by writing DPA to the memory device
sysfs attribute ('inject_poison').

Link to corresponding kernel patchset:
  https://lore.kernel.org/linux-cxl/cover.1674101475.git.alison.schofield@intel.com/

Signed-off-by: Junhyeok Im <junhyeok.im@samsung.com>
---
 cxl/lib/libcxl.c   | 26 ++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  5 +++++
 cxl/libcxl.h       |  1 +
 3 files changed, 32 insertions(+)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 4859bd5..d2b8e19 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1407,6 +1407,32 @@ CXL_EXPORT int cxl_memdev_enable(struct cxl_memdev *memdev)
 	return 0;
 }
 
+CXL_EXPORT int cxl_memdev_inject_poison(struct cxl_memdev *memdev,
+					const char *address)
+{
+	struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
+	char *path = memdev->dev_buf;
+	int len = memdev->buf_len, rc;
+
+	if (snprintf(path, len, "%s/inject_poison",
+		     memdev->dev_path) >= len) {
+		err(ctx, "%s: buffer too small\n",
+		    cxl_memdev_get_devname(memdev));
+		return -ENXIO;
+	}
+	rc = sysfs_write_attr(ctx, path, address);
+	if (rc < 0) {
+		fprintf(stderr, "%s: Failed write sysfs attr inject_poison\n",
+			cxl_memdev_get_devname(memdev));
+		return rc;
+	}
+
+	dbg(ctx, "%s: poison injected at %s\n", cxl_memdev_get_devname(memdev),
+	    address);
+
+	return 0;
+}
+
 static struct cxl_endpoint *cxl_port_find_endpoint(struct cxl_port *parent_port,
 						   struct cxl_memdev *memdev)
 {
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 6bc0810..c6ba248 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -242,3 +242,8 @@ global:
 	cxl_target_get_firmware_node;
 	cxl_dport_get_firmware_node;
 } LIBCXL_3;
+
+LIBCXL_5 {
+global:
+        cxl_memdev_inject_poison;
+} LIBCXL_4;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index d699af8..7a2cc5f 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -68,6 +68,7 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
 		size_t offset);
 int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
 		size_t offset);
+int cxl_memdev_inject_poison(struct cxl_memdev *memdev, const char *address);
 
 #define cxl_memdev_foreach(ctx, memdev) \
         for (memdev = cxl_memdev_get_first(ctx); \
-- 
2.34.1

[ndctl 2/3] cxl: add inject-poison command to cxl tool

[junhyeok.im]

From: Junhyeok Im <junhyeok.im@samsung.com>

Add new command to cli tool, to inject poison into dpa(-a) on the
memory device.

DPA written in sysfs attribute(inject_poison) is converted by
kstrtou64 with 0 base by 'inject_poison_store' of CXL driver, so if
it begins with 0x the number will be parsed as a hexadecimal
(case insensitive), if it otherwise begins with 0, it will be parsed
as an octal number. Otherwise it will be parsed as a decimal.

Since the validity verification of the dpa would be done in
'cxl_validate_poison_dpa' of CXL driver, no additional logic
is added.

Also since it is expected no use case of injecting poison into the
same address for multiple devices, this command targets only one
memdev, like write-labels command.

 usage: cxl inject-poison <memdev> -a <dpa> [<options>]

    -v, --verbose         turn on debug
    -S, --serial          use serial numbers to id memdevs
    -a, --address <dpa>   DPA to inject poison

Link to corresponding kernel patch:
  https://patchwork.kernel.org/project/cxl/patch/97a0b128d0d0df56cea1a1a4ead65a40b9cf008e.1674101475.git.alison.schofield@intel.com/

Signed-off-by: Junhyeok Im <junhyeok.im@samsung.com>
---
 cxl/builtin.h |  1 +
 cxl/cxl.c     |  1 +
 cxl/memdev.c  | 53 ++++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/cxl/builtin.h b/cxl/builtin.h
index 34c5cfb..ddc4da9 100644
--- a/cxl/builtin.h
+++ b/cxl/builtin.h
@@ -23,4 +23,5 @@ 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_monitor(int argc, const char **argv, struct cxl_ctx *ctx);
+int cmd_inject_poison(int argc, const char **argv, struct cxl_ctx *ctx);
 #endif /* _CXL_BUILTIN_H_ */
diff --git a/cxl/cxl.c b/cxl/cxl.c
index 3be7026..aa8d090 100644
--- a/cxl/cxl.c
+++ b/cxl/cxl.c
@@ -77,6 +77,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-poison", .c_fn = cmd_inject_poison },
 };
 
 int main(int argc, const char **argv)
diff --git a/cxl/memdev.c b/cxl/memdev.c
index 0b3ad02..7a10f79 100644
--- a/cxl/memdev.c
+++ b/cxl/memdev.c
@@ -34,6 +34,7 @@ static struct parameters {
 	const char *type;
 	const char *size;
 	const char *decoder_filter;
+	const char *poison_address;
 } param;
 
 static struct log_ctx ml;
@@ -85,6 +86,10 @@ OPT_STRING('t', "type", &param.type, "type",                   \
 OPT_BOOLEAN('f', "force", &param.force,                        \
 	    "Attempt 'expected to fail' operations")
 
+#define INJECT_POISON_OPTIONS()				\
+OPT_STRING('a', "address", &param.poison_address, "dpa",	\
+	   "DPA to inject poison")
+
 static const struct option read_options[] = {
 	BASE_OPTIONS(),
 	LABEL_OPTIONS(),
@@ -135,6 +140,12 @@ static const struct option free_dpa_options[] = {
 	OPT_END(),
 };
 
+static const struct option inject_poison_options[] = {
+	BASE_OPTIONS(),
+	INJECT_POISON_OPTIONS(),
+	OPT_END(),
+};
+
 enum reserve_dpa_mode {
 	DPA_ALLOC,
 	DPA_FREE,
@@ -351,6 +362,24 @@ static int action_free_dpa(struct cxl_memdev *memdev,
 	return __reserve_dpa(memdev, DPA_FREE, actx);
 }
 
+static int action_inject_poison(struct cxl_memdev *memdev,
+				struct action_context *actx)
+{
+	int rc;
+
+	if (!param.poison_address) {
+		log_err(&ml, "%s: set dpa to inject poison.\n",
+			cxl_memdev_get_devname(memdev));
+		return -EINVAL;
+	}
+	rc = cxl_memdev_inject_poison(memdev, param.poison_address);
+	if (rc < 0) {
+		log_err(&ml, "%s: inject poison failed: %s\n",
+			cxl_memdev_get_devname(memdev), strerror(-rc));
+	}
+	return rc;
+}
+
 static int action_disable(struct cxl_memdev *memdev, struct action_context *actx)
 {
 	if (!cxl_memdev_is_enabled(memdev))
@@ -755,7 +784,8 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
 				continue;
 			found = true;
 
-			if (action == action_write) {
+			if ((action == action_write) ||
+			    (action == action_inject_poison)) {
 				single = memdev;
 				rc = 0;
 			} else
@@ -771,9 +801,15 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
 	}
 	rc = err;
 
-	if (action == action_write) {
+	if ((action == action_write) || (action == action_inject_poison)) {
 		if (count > 1) {
-			error("write-labels only supports writing a single memdev\n");
+			if (action == action_write) {
+				error("write-labels only supports writing "
+				      "a single memdev\n");
+			} else {
+				error("inject-poison only supports injection "
+				      "of poison into a single memdev\n");
+			}
 			usage_with_options(u, options);
 			return -EINVAL;
 		} else if (single) {
@@ -893,3 +929,14 @@ int cmd_free_dpa(int argc, const char **argv, struct cxl_ctx *ctx)
 
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
+
+int cmd_inject_poison(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	int count = memdev_action(
+		argc, argv, ctx, action_inject_poison, inject_poison_options,
+		"cxl inject-poison <memdev> -a <dpa> [<options>]");
+	log_info(&ml, "inject-poison %d mem%s\n", count >= 0 ? count : 0,
+		 count > 1 ? "s" : "");
+
+	return count >= 0 ? 0 : EXIT_FAILURE;
+}
-- 
2.34.1