Re: [PATCH] [RFC]cxl: add get-alert-config command

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Wed, 2022-09-07 at 10:00 -0700, Jonathan Zhang wrote:

> Subject: Re: [PATCH] [RFC]cxl: add get-alert-config command

Note adding 'ndctl' to the subject-prefix is recommended to easily pick
out ndctl patches from kernel ones. So the final subject in this case
would look something like: "[ndctl RFC PATCH] cxl: add ..."

> This is a RFC patch. If the idea looks okay, I think this patch
> should be split into 3 patches: one for libcxl, another to add
> the get-alert-config command, another to add the man page.

I think splitting the libcxl part out is probably okay - most of the
time the cxl-cli command and the man page for it can go in the same
patch though.

> 
> TESTED=on a CXL 1.1 host + CXL 2.0 device (RCRB mode) configuration:

"Tested on a" - though this is not strictly necessary. More useful
would be a spec reference to the section number where get-alert-config
is defined.

> [root@suncl2h1 cxlcli-test]# ../tmp/cxl get-alert-config mem0
> alert_config summary
>     valid_alerts: 0x8
>     programmable_alerts: 0x8
>     life_used_critical_alert_threshold: 0x0
>     life_used_prog_warn_threshold: 0x0
>     dev_over_temp_crit_alert_threshold: 0x0
>     dev_under_temp_crit_alert_threshold: 0x0
>     dev_over_temp_prog_warn_threshold: 0x0
>     dev_under_temp_prog_warn_threshold: 0x0
>     corr_vol_mem_err_prog_warn_thresold: 0x0
>     corr_pers_mem_err_prog_warn_threshold: 0x0

I think this needn't be a separate command by itself, instead just an
additional optional listing mode when listing memdevs.

i.e. a new option to:

  # cxl list -m mem0 --alert-config

Which would output:

  [
    {
      "memdev":"mem0",
      "pmem_size":1073741824,
      "ram_size":1073741824,
      "alert_config":{
        "valid_alerts":0x0,
        "programmable_alerts":0x8,
        "life_used_critical_alert_threshold":0x0,
        ... <etc>
      },
      "serial":0,
      "numa_node":0,
      "host":"cxl_mem.0"
    }
  ]

Thios gets us the usual JSON formatted output that is human and machine
consubable instead of free-form output.

> 
> Signed-off-by: Jonathan Zhang <jonzhang@fb.com>
> ---
>  Documentation/cxl/cxl-get-alert-config.txt | 28 ++++++++
>  Documentation/cxl/lib/libcxl.txt           |  1 +
>  Documentation/cxl/meson.build              |  1 +
>  cxl/builtin.h                              |  1 +
>  cxl/cxl.c                                  |  1 +
>  cxl/lib/libcxl.c                           | 76 ++++++++++++++++++++++
>  cxl/lib/libcxl.sym                         |  1 +
>  cxl/libcxl.h                               |  1 +
>  cxl/memdev.c                               | 18 +++++
>  9 files changed, 128 insertions(+)
>  create mode 100644 Documentation/cxl/cxl-get-alert-config.txt
> 
> diff --git a/Documentation/cxl/cxl-get-alert-config.txt b/Documentation/cxl/cxl-get-alert-config.txt
> new file mode 100644
> index 0000000..edebd74
> --- /dev/null
> +++ b/Documentation/cxl/cxl-get-alert-config.txt
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +cxl-get-alert-config(1)
> +=======================
> +
> +NAME
> +----
> +cxl-get-alert-config - Get Alert Configuration
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'cxl get-alert-config' <mem0> [<mem1>..<memN>] [<options>]
> +
> +This command retrieves the device's critical alert and programmable warning
> +configuration. In the multi-memdev case the data is concatenated.
> +
> +OPTIONS
> +-------
> +-o::
> +--output::
> +       output file
> +
> +include::../copyright.txt[]

This man page obviously goes away when converting to a cxl-list option,
but just a note, this would include the default Intel copyright. I
assume you'd want a Facebook copyright line instead.

> +
> +SEE ALSO
> +--------
> +CXL-3.0 8.2.9.8.3.2
> diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> index fd2962a..fd054be 100644
> --- a/Documentation/cxl/lib/libcxl.txt
> +++ b/Documentation/cxl/lib/libcxl.txt
> @@ -125,6 +125,7 @@ struct cxl_cmd *cxl_cmd_new_read_label(struct cxl_memdev *memdev,
>                                         unsigned int offset, unsigned int length);
>  struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev, void *buf,
>                                         unsigned int offset, unsigned int length);
> +int cxl_memdev_get_alert_config(struct cxl_memdev *memdev);
>  int cxl_memdev_zero_label(struct cxl_memdev *memdev, size_t length,
>                           size_t offset);
>  int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
> diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build
> index 147ea71..d284eb9 100644
> --- a/Documentation/cxl/meson.build
> +++ b/Documentation/cxl/meson.build
> @@ -41,6 +41,7 @@ cxl_manpages = [
>    'cxl-set-partition.txt',
>    'cxl-reserve-dpa.txt',
>    'cxl-free-dpa.txt',
> +  'cxl-get-alert-config.txt',
>    'cxl-create-region.txt',
>    'cxl-disable-region.txt',
>    'cxl-enable-region.txt',
> diff --git a/cxl/builtin.h b/cxl/builtin.h
> index b28c221..0ca3c4b 100644
> --- a/cxl/builtin.h
> +++ b/cxl/builtin.h
> @@ -10,6 +10,7 @@ int cmd_read_labels(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_zero_labels(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_init_labels(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_check_labels(int argc, const char **argv, struct cxl_ctx *ctx);
> +int cmd_get_alert_config(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_disable_memdev(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_enable_memdev(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_reserve_dpa(int argc, const char **argv, struct cxl_ctx *ctx);
> diff --git a/cxl/cxl.c b/cxl/cxl.c
> index dd1be7a..5c120c7 100644
> --- a/cxl/cxl.c
> +++ b/cxl/cxl.c
> @@ -64,6 +64,7 @@ static struct cmd_struct commands[] = {
>         { "zero-labels", .c_fn = cmd_zero_labels },
>         { "read-labels", .c_fn = cmd_read_labels },
>         { "write-labels", .c_fn = cmd_write_labels },
> +       { "get-alert-config", .c_fn = cmd_get_alert_config },
>         { "disable-memdev", .c_fn = cmd_disable_memdev },
>         { "enable-memdev", .c_fn = cmd_enable_memdev },
>         { "reserve-dpa", .c_fn = cmd_reserve_dpa },
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index e8c5d44..1db5b2c 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -3140,6 +3140,82 @@ do {                                                                     \
>         return !!(c->field & mask);                                     \
>  } while(0)
>  
> +struct cxl_mbox_get_alert_config_out {
> +       u8 valid_alerts;
> +       u8 programmable_alerts;
> +       u8 life_used_critical_alert_threshold;
> +       u8 life_used_prog_warn_threshold;
> +       __le16 dev_over_temp_crit_alert_threshold;
> +       __le16 dev_under_temp_crit_alert_threshold;
> +       __le16 dev_over_temp_prog_warn_threshold;
> +       __le16 dev_under_temp_prog_warn_threshold;
> +       __le16 corr_vol_mem_err_prog_warn_thresold;
> +       __le16 corr_pers_mem_err_prog_warn_threshold;
> +}  __attribute__((packed));

This definition should go in cxl/lib/private.h

> +
> +CXL_EXPORT int cxl_memdev_get_alert_config(struct cxl_memdev *memdev)

See the get-health-info API for an example to follow here. The general
gist is: cxl-cli will submit one mailbox command to 'fill out' the get-
alert-config output structure, and then that cxl_cmd structure will be
passed into separate accessor functions for each get-alert-config
field.

> +{
> +       struct cxl_cmd *cmd;
> +       struct cxl_mbox_get_alert_config_out *alert_config_out;
> +       int rc = 0;
> +
> +       cmd = cxl_cmd_new_generic(memdev, CXL_MEM_COMMAND_ID_GET_ALERT_CONFIG);
> +       if (!cmd) {
> +               fprintf(stderr, "%s: cxl_cmd_new_raw returned Null output\n",
> +                               cxl_memdev_get_devname(memdev));
> +               return -ENOMEM;
> +       }
> +
> +       rc = cxl_cmd_submit(cmd);
> +       if (rc < 0) {
> +               fprintf(stderr, "%s: cmd submission failed: %d (%s)\n",
> +                               cxl_memdev_get_devname(memdev), rc, strerror(-rc));
> +               goto out;
> +       }
> +
> +       rc = cxl_cmd_get_mbox_status(cmd);
> +       if (rc != 0) {
> +               fprintf(stderr, "%s: firmware status: %d\n",
> +                               cxl_memdev_get_devname(memdev), rc);
> +               rc = -ENXIO;
> +               goto out;
> +       }
> +
> +       if (cmd->send_cmd->id != CXL_MEM_COMMAND_ID_GET_ALERT_CONFIG) {
> +               fprintf(stderr, "%s: invalid command id 0x%x (expecting 0x%x)\n",
> +                               cxl_memdev_get_devname(memdev), cmd->send_cmd->id, CXL_MEM_COMMAND_ID_GET_ALERT_CONFIG);
> +               return -EINVAL;
> +       }
> +
> +       fprintf(stdout, "alert_config summary\n");

This prints would happen as part of the JSON emission from cxl/json.c,
but as a general rule, only JSON formatted output goes to stdout.
Everything else goes to stderr, and use the logging helpers (e.g.
dbg(), err(), etc. ) for any output from libcxl, so that if logging is
being redirected to something other than stderr, everything stays
consistent.

> +
> +       alert_config_out = (void *)cmd->send_cmd->out.payload;
> +
> +       fprintf(stdout, "    valid_alerts: 0x%x\n", alert_config_out->valid_alerts);
> +       fprintf(stdout, "    programmable_alerts: 0x%x\n", alert_config_out->programmable_alerts);
> +       fprintf(stdout, "    life_used_critical_alert_threshold: 0x%x\n",
> +               alert_config_out->life_used_critical_alert_threshold);
> +       fprintf(stdout, "    life_used_prog_warn_threshold: 0x%x\n",
> +               alert_config_out->life_used_prog_warn_threshold);
> +
> +       fprintf(stdout, "    dev_over_temp_crit_alert_threshold: 0x%x\n",
> +               le16_to_cpu(alert_config_out->dev_over_temp_crit_alert_threshold));
> +       fprintf(stdout, "    dev_under_temp_crit_alert_threshold: 0x%x\n",
> +               le16_to_cpu(alert_config_out->dev_under_temp_crit_alert_threshold));
> +       fprintf(stdout, "    dev_over_temp_prog_warn_threshold: 0x%x\n",
> +               le16_to_cpu(alert_config_out->dev_over_temp_prog_warn_threshold));
> +       fprintf(stdout, "    dev_under_temp_prog_warn_threshold: 0x%x\n",
> +               le16_to_cpu(alert_config_out->dev_under_temp_prog_warn_threshold));
> +       fprintf(stdout, "    corr_vol_mem_err_prog_warn_thresold: 0x%x\n",
> +               le16_to_cpu(alert_config_out->corr_vol_mem_err_prog_warn_thresold));
> +       fprintf(stdout, "    corr_pers_mem_err_prog_warn_threshold: 0x%x\n",
> +               le16_to_cpu(alert_config_out->corr_pers_mem_err_prog_warn_threshold));
> +
> +out:
> +       cxl_cmd_unref(cmd);
> +       return rc;
> +}
> +
>  CXL_EXPORT struct cxl_cmd *cxl_cmd_new_get_health_info(
>                 struct cxl_memdev *memdev)
>  {
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 8bb91e0..c6dd658 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -20,6 +20,7 @@ global:
>         cxl_memdev_get_pmem_size;
>         cxl_memdev_get_ram_size;
>         cxl_memdev_get_firmware_verison;
> +       cxl_memdev_get_alert_config;

The way the symbol script works is that each release gets a new
'section' for all the symbols that it adds. So v74's new symbols were
added to the 'LIBCXL_3' section. If this is targeting the next, v75
release, its new APIs will go into a new section - LIBCXL_4. If
LIBCXL_4 already existed on the pending branch, you can append into
that section at the bottom. But in this case since it doesn't exist,
you can create one, and add the new APIs in this patch there.

>         cxl_cmd_get_devname;
>         cxl_cmd_new_raw;
>         cxl_cmd_set_input_payload;
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index 9fe4e99..e0b9418 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -60,6 +60,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_get_alert_config(struct cxl_memdev *memdev);
>  
>  #define cxl_memdev_foreach(ctx, memdev) \
>          for (memdev = cxl_memdev_get_first(ctx); \
> diff --git a/cxl/memdev.c b/cxl/memdev.c
> index 0b3ad02..5d8cf6c 100644
> --- a/cxl/memdev.c
> +++ b/cxl/memdev.c
> @@ -371,6 +371,16 @@ static int action_enable(struct cxl_memdev *memdev, struct action_context *actx)
>         return cxl_memdev_enable(memdev);
>  }
>  
> +static const struct option cmd_get_alert_config_options[] = {
> +       BASE_OPTIONS(),
> +       OPT_END(),
> +};
> +
> +static int action_cmd_get_alert_config(struct cxl_memdev *memdev, struct action_context *actx)
> +{
> +       return cxl_memdev_get_alert_config(memdev);
> +}
> +
>  static int action_zero(struct cxl_memdev *memdev, struct action_context *actx)
>  {
>         size_t size;
> @@ -809,6 +819,14 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
>         return rc;
>  }
>  
> +int cmd_get_alert_config(int argc, const char **argv, struct cxl_ctx *ctx)
> +{
> +       int rc = memdev_action(argc, argv, ctx, action_cmd_get_alert_config, cmd_get_alert_config_options,
> +                       "cxl get-alert-config <mem0> [<mem1>..<memN>] [<options>]");
> +
> +       return rc >= 0 ? 0 : EXIT_FAILURE;
> +}
> +
>  int cmd_write_labels(int argc, const char **argv, struct cxl_ctx *ctx)
>  {
>         int count = memdev_action(argc, argv, ctx, action_write, write_options,