Re: [ndctl PATCH v2 01/13] cxl: add a cxl utility and libcxl library
On Mon, 2021-02-22 at 13:36 -0800, Ben Widawsky wrote:
[..]
>
> > +SYNOPSIS
> > +
> > +[verse]
> > +'cxl list' []
> > +
> > +Walk the CXL capable device hierarchy in the system and list all device
> > +instances along with some of their major attributes.
>
> This doesn't seem to match the above. Here it's just devices and above you
> talk
> about bridges and switches as well.
Good catch - those can be added in later when we have a sysfs
representation for them. I'll change it to say just devices for now.
[..]
> > +
> > +static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
> > +{
> > + const char *devname = devpath_to_devname(cxlmem_base);
> > + char *path = calloc(1, strlen(cxlmem_base) + 100);
> > + struct cxl_ctx *ctx = parent;
> > + struct cxl_memdev *memdev, *memdev_dup;
> > + char buf[SYSFS_ATTR_SIZE];
> > + struct stat st;
> > +
> > + if (!path)
> > + return NULL;
> > + dbg(ctx, "%s: base: \'%s\'\n", __func__, cxlmem_base);
> > +
> > + memdev = calloc(1, sizeof(*memdev));
> > + if (!memdev)
> > + goto err_dev;
> > + memdev->id = id;
> > + memdev->ctx = ctx;
> > +
> > + sprintf(path, "/dev/cxl/%s", devname);
> > + if (stat(path, ) < 0)
> > + goto err_read;
> > + memdev->major = major(st.st_rdev);
> > + memdev->minor = minor(st.st_rdev);
> > +
> > + sprintf(path, "%s/pmem/size", cxlmem_base);
> > + if (sysfs_read_attr(ctx, path, buf) < 0)
> > + goto err_read;
> > + memdev->pmem_size = strtoull(buf, NULL, 0);
>
> For strtoull usage and below - it certainly doesn't matter much but maybe
> using
> 10 for base would better since sysfs is ABI and therefore anything other than
> base 10 is incorrect.
Hm, I followed what libndctl does, but I think there is value in
accepting valid hex even if it is technically 'wrong' per the robustness
principle. How much do we want libcxl/libndctl to be a kernel validation
vehicle vs. just work if you can?
[..]
> > +
> > +static int cmd_help(int argc, const char **argv, struct cxl_ctx *ctx)
> > +{
> > + const char * const builtin_help_subcommands[] = {
> > + "list", NULL,
> > + };
>
> Move NULL to newline.
Yep.
>
> > +int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
> > +{
> > + const struct option options[] = {
> > + OPT_STRING('d', "memdev", , "memory device name",
> > + "filter by CXL memory device name"),
> > + OPT_BOOLEAN('D', "memdevs", ,
> > + "include CXL memory device info"),
> > + OPT_BOOLEAN('i', "idle", , "include idle devices"),
> > + OPT_BOOLEAN('u', "human", ,
> > + "use human friendly number formats "),
> > + OPT_END(),
> > + };
> > + const char * const u[] = {
> > + "cxl list []",
> > + NULL
> > + };
> > + struct json_object *jdevs = NULL;
> > + unsigned long list_flags;
> > + struct cxl_memdev *memdev;
> > + int i;
> > +
> > +argc = parse_options(argc, argv, options, u, 0);
>
> Tab.
>
> /me looks for .clang-format
Thanks - let me see if I can quickly adapt the kernel's .clang-format
for this and add it in for the next revision.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v2 01/13] cxl: add a cxl utility and libcxl library
On 21-02-18 19:03:19, Vishal Verma wrote:
> CXL - or Compute eXpress Link - is a new interconnect that extends PCIe
> to support a wide range of devices, including cache coherent memory
> expanders. As such, these devices can be new sources of 'persistent
> memory', and the 'ndctl' umbrella of tools and libraries needs to be able
> to interact with them.
>
> Add a new utility and library for managing these CXL memory devices. This
> is an initial bring-up for interacting with CXL devices, and only includes
> adding the utility and library infrastructure, parsing device information
> from sysfs for CXL devices, and providing a 'cxl-list' command to
> display this information in JSON formatted output.
>
> Cc: Ben Widawsky
> Cc: Dan Williams
> Signed-off-by: Vishal Verma
A couple of really minor things below
> ---
> Documentation/cxl/cxl-list.txt | 65 +
> Documentation/cxl/cxl.txt| 34 +++
> Documentation/cxl/human-option.txt | 8 +
> Documentation/cxl/verbose-option.txt | 5 +
> configure.ac | 3 +
> Makefile.am | 8 +-
> Makefile.am.in | 4 +
> cxl/lib/private.h| 29 +++
> cxl/lib/libcxl.c | 345 +++
> cxl/builtin.h| 8 +
> cxl/libcxl.h | 55 +
> util/filter.h| 2 +
> util/json.h | 3 +
> util/main.h | 3 +
> cxl/cxl.c| 95
> cxl/list.c | 113 +
> util/filter.c| 20 ++
> util/json.c | 26 ++
> .gitignore | 2 +
> Documentation/cxl/Makefile.am| 58 +
> cxl/Makefile.am | 21 ++
> cxl/lib/Makefile.am | 32 +++
> cxl/lib/libcxl.pc.in | 11 +
> cxl/lib/libcxl.sym | 29 +++
> 24 files changed, 976 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/cxl/cxl-list.txt
> create mode 100644 Documentation/cxl/cxl.txt
> create mode 100644 Documentation/cxl/human-option.txt
> create mode 100644 Documentation/cxl/verbose-option.txt
> create mode 100644 cxl/lib/private.h
> create mode 100644 cxl/lib/libcxl.c
> create mode 100644 cxl/builtin.h
> create mode 100644 cxl/libcxl.h
> create mode 100644 cxl/cxl.c
> create mode 100644 cxl/list.c
> create mode 100644 Documentation/cxl/Makefile.am
> create mode 100644 cxl/Makefile.am
> create mode 100644 cxl/lib/Makefile.am
> create mode 100644 cxl/lib/libcxl.pc.in
> create mode 100644 cxl/lib/libcxl.sym
>
> diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> new file mode 100644
> index 000..107b388
> --- /dev/null
> +++ b/Documentation/cxl/cxl-list.txt
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +cxl-list(1)
> +===
> +
> +NAME
> +
> +cxl-list - CXL capable host bridges, switches, devices, and their attributes
> +in json.
> +
> +SYNOPSIS
> +
> +[verse]
> +'cxl list' []
> +
> +Walk the CXL capable device hierarchy in the system and list all device
> +instances along with some of their major attributes.
This doesn't seem to match the above. Here it's just devices and above you talk
about bridges and switches as well.
> +
> +Options can be specified to limit the output to specific devices.
> +By default, 'cxl list' with no options is equivalent to:
> +[verse]
> +cxl list --devices
> +
> +EXAMPLE
> +---
> +
> +# cxl list --devices
> +{
> + "memdev":"mem0",
> + "pmem_size":268435456,
> + "ram_size":0,
> +}
> +
> +
> +OPTIONS
> +---
> +-d::
> +--dev=::
> + Specify a cxl device name to filter the listing. For example:
> +
> +# cxl list --dev=mem0
> +{
> + "memdev":"mem0",
> + "pmem_size":268435456,
> + "ram_size":0,
> +}
> +
> +
> +-D::
> +--devices::
> + Include all CXL devices in the listing
> +
> +-i::
> +--idle::
> + Include idle (not enabled / zero-sized) devices in the listing
> +
> +include::human-option.txt[]
> +
> +include::verbose-option.txt[]
> +
> +include::../copyright.txt[]
> +
> +SEE ALSO
> +
> +linkcxl:ndctl-list[1]
> diff --git a/Documentation/cxl/cxl.txt b/Documentation/cxl/cxl.txt
> new file mode 100644
> index 000..e99e61b
> --- /dev/null
> +++ b/Documentation/cxl/cxl.txt
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +cxl(1)
> +==
> +
> +NAME
> +
> +cxl - Provides enumeration and provisioning commands for CXL devices
> +
> +SYNOPSIS
> +
> +[verse]
> +'cxl' [--version] [--help] COMMAND [ARGS]
> +
> +OPTIONS
> +---
> +-v::
> +--version::
> + Display the version of the 'cxl' utility.
> +
> +-h::
> +--help::
> + Run the 'cxl help' command.
> +
> +DESCRIPTION
> +---
> +The cxl utility provides enumeration and provisioning
[ndctl PATCH v2 01/13] cxl: add a cxl utility and libcxl library
CXL - or Compute eXpress Link - is a new interconnect that extends PCIe
to support a wide range of devices, including cache coherent memory
expanders. As such, these devices can be new sources of 'persistent
memory', and the 'ndctl' umbrella of tools and libraries needs to be able
to interact with them.
Add a new utility and library for managing these CXL memory devices. This
is an initial bring-up for interacting with CXL devices, and only includes
adding the utility and library infrastructure, parsing device information
from sysfs for CXL devices, and providing a 'cxl-list' command to
display this information in JSON formatted output.
Cc: Ben Widawsky
Cc: Dan Williams
Signed-off-by: Vishal Verma
---
Documentation/cxl/cxl-list.txt | 65 +
Documentation/cxl/cxl.txt| 34 +++
Documentation/cxl/human-option.txt | 8 +
Documentation/cxl/verbose-option.txt | 5 +
configure.ac | 3 +
Makefile.am | 8 +-
Makefile.am.in | 4 +
cxl/lib/private.h| 29 +++
cxl/lib/libcxl.c | 345 +++
cxl/builtin.h| 8 +
cxl/libcxl.h | 55 +
util/filter.h| 2 +
util/json.h | 3 +
util/main.h | 3 +
cxl/cxl.c| 95
cxl/list.c | 113 +
util/filter.c| 20 ++
util/json.c | 26 ++
.gitignore | 2 +
Documentation/cxl/Makefile.am| 58 +
cxl/Makefile.am | 21 ++
cxl/lib/Makefile.am | 32 +++
cxl/lib/libcxl.pc.in | 11 +
cxl/lib/libcxl.sym | 29 +++
24 files changed, 976 insertions(+), 3 deletions(-)
create mode 100644 Documentation/cxl/cxl-list.txt
create mode 100644 Documentation/cxl/cxl.txt
create mode 100644 Documentation/cxl/human-option.txt
create mode 100644 Documentation/cxl/verbose-option.txt
create mode 100644 cxl/lib/private.h
create mode 100644 cxl/lib/libcxl.c
create mode 100644 cxl/builtin.h
create mode 100644 cxl/libcxl.h
create mode 100644 cxl/cxl.c
create mode 100644 cxl/list.c
create mode 100644 Documentation/cxl/Makefile.am
create mode 100644 cxl/Makefile.am
create mode 100644 cxl/lib/Makefile.am
create mode 100644 cxl/lib/libcxl.pc.in
create mode 100644 cxl/lib/libcxl.sym
diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
new file mode 100644
index 000..107b388
--- /dev/null
+++ b/Documentation/cxl/cxl-list.txt
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-list(1)
+===
+
+NAME
+
+cxl-list - CXL capable host bridges, switches, devices, and their attributes
+in json.
+
+SYNOPSIS
+
+[verse]
+'cxl list' []
+
+Walk the CXL capable device hierarchy in the system and list all device
+instances along with some of their major attributes.
+
+Options can be specified to limit the output to specific devices.
+By default, 'cxl list' with no options is equivalent to:
+[verse]
+cxl list --devices
+
+EXAMPLE
+---
+
+# cxl list --devices
+{
+ "memdev":"mem0",
+ "pmem_size":268435456,
+ "ram_size":0,
+}
+
+
+OPTIONS
+---
+-d::
+--dev=::
+ Specify a cxl device name to filter the listing. For example:
+
+# cxl list --dev=mem0
+{
+ "memdev":"mem0",
+ "pmem_size":268435456,
+ "ram_size":0,
+}
+
+
+-D::
+--devices::
+ Include all CXL devices in the listing
+
+-i::
+--idle::
+ Include idle (not enabled / zero-sized) devices in the listing
+
+include::human-option.txt[]
+
+include::verbose-option.txt[]
+
+include::../copyright.txt[]
+
+SEE ALSO
+
+linkcxl:ndctl-list[1]
diff --git a/Documentation/cxl/cxl.txt b/Documentation/cxl/cxl.txt
new file mode 100644
index 000..e99e61b
--- /dev/null
+++ b/Documentation/cxl/cxl.txt
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl(1)
+==
+
+NAME
+
+cxl - Provides enumeration and provisioning commands for CXL devices
+
+SYNOPSIS
+
+[verse]
+'cxl' [--version] [--help] COMMAND [ARGS]
+
+OPTIONS
+---
+-v::
+--version::
+ Display the version of the 'cxl' utility.
+
+-h::
+--help::
+ Run the 'cxl help' command.
+
+DESCRIPTION
+---
+The cxl utility provides enumeration and provisioning commands for
+the CXL devices managed by the Linux kernel.
+
+include::../copyright.txt[]
+
+SEE ALSO
+
+linkcxl:ndctl[1]
diff --git a/Documentation/cxl/human-option.txt
b/Documentation/cxl/human-option.txt
new file mode 100644
index 000..2f4de7a
--- /dev/null
+++ b/Documentation/cxl/human-option.txt
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+-u::
+--human::
+ By default the command will output machine-friendly raw-integer
+ data. Instead, with this flag, numbers representing
