Re: [ndctl PATCH v3 02/21] cxl: add a cxl utility and libcxl library

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

On Fri, 2021-07-09 at 18:12 -0700, Dan Williams wrote:
> On Thu, Jul 1, 2021 at 1:10 PM Vishal Verma <vishal.l.verma@intel.com> 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 <ben.widawsky@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> 
> Looks good, just a couple minor quibbles below:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Thanks Dan - addressed as below.

> 
> > ---
> >  Documentation/cxl/cxl-list.txt       |  64 +++++
> >  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                            |  96 ++++++++
> >  cxl/list.c                           | 113 +++++++++
> >  util/filter.c                        |  20 ++
> >  util/json.c                          |  26 ++
> >  .clang-format                        |   1 +
> >  .gitignore                           |   4 +-
> >  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 +++
> >  25 files changed, 978 insertions(+), 4 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 0000000..4e2be87
> > --- /dev/null
> > +++ b/Documentation/cxl/cxl-list.txt
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +cxl-list(1)
> > +===========
> > +
> > +NAME
> > +----
> > +cxl-list - List CXL capable memory devices, and their attributes in json.
> 
> This will also show CXL port topology in the future. I'm fine to fix
> that up later when that support arrives.

Yep I actually removed it based on feedback from Ben saying we should
add those when the actual support for those is added.

> 
> > +
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'cxl list' [<options>]
> > +
> > +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
> 
> Is this from an earlier version, should it be --memdevs?

Yep stale, fixed for v4.

> 
> > +{
> > +  "memdev":"mem0",
> > +  "pmem_size":268435456,
> > +  "ram_size":0,
> > +}
> > +----
> > +
> > +OPTIONS
> > +-------
> > +-d::
> > +--memdev=::
> > +       Specify a cxl memory device name to filter the listing. For example:
> > +----
> > +# cxl list --memdev=mem0
> > +{
> > +  "memdev":"mem0",
> > +  "pmem_size":268435456,
> > +  "ram_size":0,
> > +}
> > +----
> > +
> > +-D::
> > +--memdevs::
> > +       Include all CXL memory 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 0000000..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
> 
> s/device/platforms/ since it should enumerate an entire topology.

Good catch, fixed.