| Message ID | 20230307082102.27195-1-jehoon.park@samsung.com |
|---|---|
| Headers | show |
| Series | add support for IDENTIFY command | expand |
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 > > 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 >
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 > >
Jehoon Park wrote: > 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"? Fundamentally why does user space need to know the event log sizes? I do like the idea of getting the 'raw' results of the identify command in it's entirety. What if list has an '--identify' option which adds the list of Identify values as a child json object. Ira
On Wed, Mar 08, 2023 at 10:22:41AM -0800, Ira Weiny wrote: > Jehoon Park wrote: > > 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"? > > Fundamentally why does user space need to know the event log sizes? > > I do like the idea of getting the 'raw' results of the identify command in > it's entirety. > > What if list has an '--identify' option which adds the list of Identify > values as a child json object. > That's a good way to add it. That'll give you all the fields in one place, and then, we can still think about if we want to spit out specific fields (ie poison related) when we are doing a poison command. You will have added all the libcxl accessors, making those easier to add. Alison > Ira
On Wed, Mar 08, 2023 at 01:58:11PM -0800, Alison Schofield wrote: > On Wed, Mar 08, 2023 at 10:22:41AM -0800, Ira Weiny wrote: > > Jehoon Park wrote: > > > 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"? > > > > Fundamentally why does user space need to know the event log sizes? > > > > I do like the idea of getting the 'raw' results of the identify command in > > it's entirety. > > > > What if list has an '--identify' option which adds the list of Identify > > values as a child json object. > > > That's a good way to add it. That'll give you all the fields in one > place, and then, we can still think about if we want to spit out > specific fields (ie poison related) when we are doing a poison command. > You will have added all the libcxl accessors, making those easier to add. > Alison > > > > Ira The idea adding new list option “—identify” to display raw data from Identify looks good! Providing proper fields to other options will be helpful for users, however, I think it may be covered by different patchset after basic support for Identify command is fully handled. I will revise this patchset by applying your valuable comments. Thank you. Jehoon
Jehoon Park wrote: > On Wed, Mar 08, 2023 at 01:58:11PM -0800, Alison Schofield wrote: > > On Wed, Mar 08, 2023 at 10:22:41AM -0800, Ira Weiny wrote: > > > Jehoon Park wrote: > > > > 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"? > > > > > > Fundamentally why does user space need to know the event log sizes? > > > > > > I do like the idea of getting the 'raw' results of the identify command in > > > it's entirety. > > > > > > What if list has an '--identify' option which adds the list of Identify > > > values as a child json object. > > > > > That's a good way to add it. That'll give you all the fields in one > > place, and then, we can still think about if we want to spit out > > specific fields (ie poison related) when we are doing a poison command. > > You will have added all the libcxl accessors, making those easier to add. > > Alison > > > > > > > Ira > > The idea adding new list option “—identify” to display raw data from Identify > looks good! Providing proper fields to other options will be helpful for users, > however, I think it may be covered by different patchset after basic support > for Identify command is fully handled. > > I will revise this patchset by applying your valuable comments. Thank you. I would prefer that users not need to know about a new --identify option, just include all the fields you would emit in that scenario in "cxl list -M" by default. Firmware version makes sense to add, but I do not see the end user value of emitting the Event Log Size, for example. I am just asking to be thoughtful about not including inactionable information in the output, as most of what identify reports is already available via "cxl list -MI".
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. Example: # 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