Re: [ndctl patch RFC 0/2] add support for IDENTIFY command

[Jehoon Park <jehoon.park@samsung.com>]

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

On Tue, Mar 07, 2023 at 12:18:38PM -0800, Alison Schofield wrote:
> On Tue, Mar 07, 2023 at 05:21:00PM +0900, Jehoon Park wrote:
> > From: jehoon park <jehoon.park@samsung.com>
> > 
> > This patchset supports CXL IDENTIFY mailbox command and corresponding
> > cxl tool interface command.
> > 
> > CXL 3.0 Spec 8.2.9.8.1 defines IDENTIFY command which retrieves basic
> > information about CXL memory device. The information consist of device's
> > firmware version, capacity, LSA size, event log size, poison list size,
> > inject poison limit, poison handling capabilities and QoS telemetry
> > capabilities. Firmware version, capacity and LSA size are already supported
> > and used for partition commands or sysfs attributes while others are not.
> > Since patches about event log [1] and poison [2] are discussed recently,
> > support for those information will be helpful.
> 
> Hi Jehoon,
> 
> Does this need to be a separate command? Identify fields can be included
> in cxl list options. For example, the -I option to cxl list, issues the
> identify command and includes the partition related entries in that json
> output.
> 
> There are other identify fields that need to be picked up, like the
> poison related fields.  They need to be added to the cxl list 
> options. We may want to include some when we list the poison, and
> some as an option in the memdev listing.
> 
> Is there some reasoning behind separating this out? If not, can we look
> to add the missing fields to the various cxl-list options and add
> new cxl-list options where needed?
> 
> Alison
> 

Hi Alison, thank you for comments.

I suggested separate identify command since it retrieves basic information
about memdev. Since cxl-list command lists all cxl objects, I intended to
focus memdev information by separating it. Also, I referred to nvme-cli
which has id-ctrl and id-ns commands.

However, as you commented, some fields were already included in cxl-list.
I think the idea that providing information to proper listing option also
makes sense.

Then, by following the approach, including fields to cxl-list options,
identify fields could be included like below. Do they look fine?

1. FW version and LSA size are included when listing memdev. ("list -m memdev")
2. For poison related fields (poison_list_max size and inject_poison_limit),
include them when listing poison. ("--media-errors" option, patch [1])
3. For capabilities fields, add new option "-C, --capabilities" to the
memdev listing. (I see there exists same option for listing nvdimm device)

However, I'm confused about event_log_size fields. Could they be included
in capabilities option too? or require new option like "--event"?

Jehoon

[1] https://lore.kernel.org/nvdimm/cover.1668133294.git.alison.schofield@intel.com/

> > 
> > Example:
> 
> Users expect json formatted output here.
> 
> 
> > # cxl identify mem0
> > FW Revision                              : BWFW VERSION 00
> > Total Capacity                           : 1.00 GB
> > Volatile Only Capacity                   : 1.00 GB
> > Persistent Only Capacity                 : 0 B
> > Partition Alignment                      : 0 B
> > Informational Event Log Size             : 0
> > Warning Event Log Size                   : 0
> > Failure Event Log Size                   : 0
> > Fatal Event Log Size                     : 0
> > LSA Size                                 : 0 B
> > Poison List Maximum Media Error Records  : 256
> > Inject Poison Limit                      : 0
> > Poison Handling Capabilities
> > Injects Persistent Poison                : Not Supported
> > Scans for Poison                         : Not Supported
> > QoS Telemetry Capabilities
> > Egress Port Congestion                   : Not Supported
> > Temporary Throughput Reduction           : Not Supported
> > cxl memdev: cmd_identify: identified 1 mem
> > 
> > This patch is RFC because some of the information are already provided by
> > "list -m <memdev>" command and sysfs attributes.
> > I think a separate cxl tool interface command for identify is useful to provide
> > device's information clearly. In case of nvme-cli, there are separate
> > interface commands for identifying controller and namespace: id-ctrl and id-ns.
> > 
> > [1] https://lore.kernel.org/linux-cxl/20221216-cxl-ev-log-v7-0-2316a5c8f7d8@intel.com/
> > [2] https://lore.kernel.org/linux-cxl/cover.1676685180.git.alison.schofield@intel.com/
> > 
> > jehoon park (2):
> >   libcxl: add accessors for IDENTIFY command
> >   cxl: add identify command to cxl tool
> > 
> >  Documentation/cxl/cxl-identify.txt |  57 ++++++++++++
> >  Documentation/cxl/meson.build      |   1 +
> >  cxl/builtin.h                      |   1 +
> >  cxl/cxl.c                          |   1 +
> >  cxl/lib/libcxl.c                   |  73 +++++++++++++++
> >  cxl/lib/libcxl.sym                 |  11 +++
> >  cxl/lib/private.h                  |  11 +++
> >  cxl/libcxl.h                       |  16 ++++
> >  cxl/memdev.c                       | 141 +++++++++++++++++++++++++++++
> >  9 files changed, 312 insertions(+)
> >  create mode 100644 Documentation/cxl/cxl-identify.txt
> > 
> > 
> > base-commit: b830c4af984e72e5849c0705669aad2ffa19db13
> > -- 
> > 2.17.1
> > 

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