[PATCH v2 0/2] Add log related mailbox commands

[PATCH v2 0/2] Add log related mailbox commands

[sthanneeru.opensrc]

From: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>

Add support to expose following mailbox commands to userspace
for clearing and populating the Vendor debug log and
Component State dump log in certain scenarios,
allowing for the aggregation of results over time.

1. CXL r3.1 8.2.9.5.3 Get Log Capabilities.
2. CXL r3.1 8.2.9.5.4 Clear Log commands.
3. CXL r3.1 8.2.9.5.6 Get Supported Logs Sub-List.

---
Changes in v2:
- Add descption for exposing these mailbox log commands to ioctl.
- Create seperate patch for 'Clear log'.
- Restrict the ‘Clear log’ action to only apply to
Vendor debug logs and Component state dump logs.
- Rename get log sublist to get supported log sublist.
- Link to v1: https://lore.kernel.org/linux-mm/20240207103634.199-1-sthanneeru.opensrc@micron.com/
---

Srinivasulu Thanneeru (2):
  cxl/mbox: Add Get Log Capabilities and Get Supported Logs Sub-List
    commands
  cxl/mbox: Add Clear Log mailbox command

 drivers/cxl/core/mbox.c      | 13 +++++++++++++
 drivers/cxl/cxlmem.h         |  6 ++++++
 include/uapi/linux/cxl_mem.h |  3 +++
 3 files changed, 22 insertions(+)

-- 
2.25.1

[PATCH v2 1/2] cxl/mbox: Add Get Log Capabilities and Get Supported Logs Sub-List commands

[sthanneeru.opensrc]

From: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>

Adding UAPI support for
1. CXL r3.1 8.2.9.5.3 Get Log Capabilities.
2. CXL r3.1 8.2.9.5.6 Get Supported Logs Sub-List.

Signed-off-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
---
 drivers/cxl/core/mbox.c      | 2 ++
 drivers/cxl/cxlmem.h         | 2 ++
 include/uapi/linux/cxl_mem.h | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 27166a411705..30bd8264292f 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -56,6 +56,8 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
 	CXL_CMD(GET_LSA, 0x8, CXL_VARIABLE_PAYLOAD, 0),
 	CXL_CMD(GET_HEALTH_INFO, 0, 0x12, 0),
 	CXL_CMD(GET_LOG, 0x18, CXL_VARIABLE_PAYLOAD, CXL_CMD_FLAG_FORCE_ENABLE),
+	CXL_CMD(GET_LOG_CAPS, 0x10, 0x4, 0),
+	CXL_CMD(GET_SUP_LOG_SUBLIST, 0x2, CXL_VARIABLE_PAYLOAD, 0),
 	CXL_CMD(SET_PARTITION_INFO, 0x0a, 0, 0),
 	CXL_CMD(SET_LSA, CXL_VARIABLE_PAYLOAD, 0, 0),
 	CXL_CMD(GET_ALERT_CONFIG, 0, 0x10, 0),
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 5303d6942b88..671e46538baa 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -529,6 +529,8 @@ enum cxl_opcode {
 	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
 	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
 	CXL_MBOX_OP_GET_LOG		= 0x0401,
+	CXL_MBOX_OP_GET_LOG_CAPS	= 0x0402,
+	CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405,
 	CXL_MBOX_OP_IDENTIFY		= 0x4000,
 	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
 	CXL_MBOX_OP_SET_PARTITION_INFO	= 0x4101,
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index 42066f4eb890..49c25056c222 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -47,6 +47,8 @@
 	___DEPRECATED(SCAN_MEDIA, "Scan Media"),                          \
 	___DEPRECATED(GET_SCAN_MEDIA, "Get Scan Media Results"),          \
 	___C(GET_TIMESTAMP, "Get Timestamp"),                             \
+	___C(GET_LOG_CAPS, "Get Log Capabilities"),			  \
+	___C(GET_SUP_LOG_SUBLIST, "Get Supported Logs Sub-List"),	  \
 	___C(MAX, "invalid / last command")
 
 #define ___C(a, b) CXL_MEM_COMMAND_ID_##a
-- 
2.25.1

[PATCH v2 2/2] cxl/mbox: Add Clear Log mailbox command

[sthanneeru.opensrc]

From: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>

Adding UAPI support for CXL r3.1 8.2.9.5.4
Clear Log command.

This proposed patch will be useful for
1. Clearing and populating the Vendor debug log and Component
state dump log in certain scenarios, allowing for the aggregation
of results over time.
2. Clear the log when the device is shipped from manufacturing.
3. In some device failure cases, customer may want to clear
the log before shipping it back to the vendor.

Signed-off-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
---
 drivers/cxl/core/mbox.c      | 11 +++++++++++
 drivers/cxl/cxlmem.h         |  4 ++++
 include/uapi/linux/cxl_mem.h |  1 +
 3 files changed, 16 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 30bd8264292f..5fb01194f16f 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -57,6 +57,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
 	CXL_CMD(GET_HEALTH_INFO, 0, 0x12, 0),
 	CXL_CMD(GET_LOG, 0x18, CXL_VARIABLE_PAYLOAD, CXL_CMD_FLAG_FORCE_ENABLE),
 	CXL_CMD(GET_LOG_CAPS, 0x10, 0x4, 0),
+	CXL_CMD(CLEAR_LOG, 0x10, 0, 0),
 	CXL_CMD(GET_SUP_LOG_SUBLIST, 0x2, CXL_VARIABLE_PAYLOAD, 0),
 	CXL_CMD(SET_PARTITION_INFO, 0x0a, 0, 0),
 	CXL_CMD(SET_LSA, CXL_VARIABLE_PAYLOAD, 0, 0),
@@ -333,6 +334,16 @@ static bool cxl_payload_from_user_allowed(u16 opcode, void *payload_in)
 			return false;
 		break;
 	}
+	case CXL_MBOX_OP_CLEAR_LOG: {
+		const uuid_t *uuid = (uuid_t *)payload_in;
+
+		/*
+		 * Restrict the ‘Clear log’ action to only apply to
+		 * Vendor debug logs and Component state dump logs.
+		 */
+		return (uuid_equal(uuid, &DEFINE_CXL_VENDOR_DEBUG_UUID) ||
+			uuid_equal(uuid, &DEFINE_CXL_COMPONENT_STATE_DUMP_UUID));
+	}
 	default:
 		break;
 	}
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 671e46538baa..af7f8a4b84b2 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -530,6 +530,7 @@ enum cxl_opcode {
 	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
 	CXL_MBOX_OP_GET_LOG		= 0x0401,
 	CXL_MBOX_OP_GET_LOG_CAPS	= 0x0402,
+	CXL_MBOX_OP_CLEAR_LOG           = 0x0403,
 	CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405,
 	CXL_MBOX_OP_IDENTIFY		= 0x4000,
 	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
@@ -565,6 +566,9 @@ enum cxl_opcode {
 #define DEFINE_CXL_VENDOR_DEBUG_UUID                                           \
 	UUID_INIT(0xe1819d9, 0x11a9, 0x400c, 0x81, 0x1f, 0xd6, 0x07, 0x19,     \
 		  0x40, 0x3d, 0x86)
+#define DEFINE_CXL_COMPONENT_STATE_DUMP_UUID                                   \
+	UUID_INIT(0xb3fab4cf, 0x01b6, 0x4332, 0x94, 0x3e, 0x5e, 0x99, 0x62,    \
+		0xf2, 0x35, 0x67)
 
 struct cxl_mbox_get_supported_logs {
 	__le16 entries;
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index 49c25056c222..c6c0fe27495d 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -48,6 +48,7 @@
 	___DEPRECATED(GET_SCAN_MEDIA, "Get Scan Media Results"),          \
 	___C(GET_TIMESTAMP, "Get Timestamp"),                             \
 	___C(GET_LOG_CAPS, "Get Log Capabilities"),			  \
+	___C(CLEAR_LOG, "Clear Log"),					  \
 	___C(GET_SUP_LOG_SUBLIST, "Get Supported Logs Sub-List"),	  \
 	___C(MAX, "invalid / last command")
 
-- 
2.25.1

Re: [PATCH v2 0/2] Add log related mailbox commands

[Dan Williams]

Any specific reason to include linux-mm on this enabling? I suspect this
topic is only of interest to linux-cxl. Lets drop linux-mm from a v3
posting.

sthanneeru.opensrc@ wrote:
> From: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
> 
> Add support to expose following mailbox commands to userspace
> for clearing and populating the Vendor debug log and
> Component State dump log in certain scenarios,
> allowing for the aggregation of results over time.

What is missing for me here is why the ioctl() ABI is suitable for both
of these. The Vendor Debug Log seems straightforward to enable via
ioctl() since there is no background operation entanglement, no
population complexity, and no reference to other payloads.

The Component State Dump has several caveats in my mind for ioctl() not
being a suitable ABI:

1/ Populate Log has an unreasonable expectation that the submitter can
monopolize the mailbox indefinitely. At least that can be solved by
Linux only supporting automatically populated Component State Dump logs.

2/ Automatic log population requires the caller to handle races with
auto-population. The kernel need not export that complexity to
userspace. This is not fatal to the proposal, but it has interactions
with caveat 3.

3/ The Component State Dump format wants to reference events in the
Event Log, if the Event Log has been cleared then, per the spec, the
Component State Dump must not reference the Event Handle. To me that
implies that the code that clears event records must be careful to not
destroy linkage to component state information. That suggests that the
proper place to dump the component state log is an addendum to the
current trace events, before that code clears the event record.

Something roughly like this:

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 9adda4795eb7..498b2a0b3e76 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -941,7 +941,8 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
 }
 
 static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
-                                   enum cxl_event_log_type type)
+                                   enum cxl_event_log_type type,
+                                   struct cxl_component_state_dump *csd)
 {
        struct cxl_memdev *cxlmd = mds->cxlds.cxlmd;
        struct device *dev = mds->cxlds.dev;
@@ -977,9 +978,12 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
                if (!nr_rec)
                        break;
 
-               for (i = 0; i < nr_rec; i++)
+               for (i = 0; i < nr_rec; i++) {
                        __cxl_event_trace_record(cxlmd, type,
                                                 &payload->records[i]);
+                       if (is_event_referenced(csd, type, &payload->records[i]))
+                               trace_csd(csd, ...);
+               }
 
                if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
                        trace_cxl_overflow(cxlmd, type, payload);

...but in general this cover letter needs to comment on the long term
suitability of the ABI.

Re: [PATCH v2 0/2] Add log related mailbox commands

[Srinivasulu Opensrc]

> -----Original Message-----
> From: Dan Williams <dan.j.williams@intel.com>
> Sent: Tuesday, March 5, 2024 2:10 AM
> To: Srinivasulu Opensrc <sthanneeru.opensrc@micron.com>; linux-
> cxl@vger.kernel.org; linux-mm@kvack.org
> Cc: Jonathan.Cameron@huawei.com; dan.j.williams@intel.com;
> john@jagalactic.com; Eishan Mirakhur <emirakhur@micron.com>; Ajay Joshi
> <ajayjoshi@micron.com>; Ravis OpenSrc <Ravis.OpenSrc@micron.com>;
> Srinivasulu Thanneeru <sthanneeru@micron.com>
> Subject: [EXT] Re: [PATCH v2 0/2] Add log related mailbox commands
> 
> CAUTION: EXTERNAL EMAIL. Do not click links or open attachments unless
> you recognize the sender and were expecting this message.
> 
> 
> Any specific reason to include linux-mm on this enabling? I suspect this
> topic is only of interest to linux-cxl. Lets drop linux-mm from a v3
> posting.

Sorry, it's a copy paste mistake.
Agreed, will drop linux-mm from V3. 
> 
> sthanneeru.opensrc@ wrote:
> > From: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
> >
> > Add support to expose following mailbox commands to userspace
> > for clearing and populating the Vendor debug log and
> > Component State dump log in certain scenarios,
> > allowing for the aggregation of results over time.
> 
> What is missing for me here is why the ioctl() ABI is suitable for both
> of these. The Vendor Debug Log seems straightforward to enable via
> ioctl() since there is no background operation entanglement, no
> population complexity, and no reference to other payloads.
> 
> The Component State Dump has several caveats in my mind for ioctl() not
> being a suitable ABI:
> 
> 1/ Populate Log has an unreasonable expectation that the submitter can
> monopolize the mailbox indefinitely. At least that can be solved by
> Linux only supporting automatically populated Component State Dump logs.
> 
> 2/ Automatic log population requires the caller to handle races with
> auto-population. The kernel need not export that complexity to
> userspace. This is not fatal to the proposal, but it has interactions
> with caveat 3.
> 
> 3/ The Component State Dump format wants to reference events in the
> Event Log, if the Event Log has been cleared then, per the spec, the
> Component State Dump must not reference the Event Handle. To me that
> implies that the code that clears event records must be careful to not
> destroy linkage to component state information. That suggests that the
> proper place to dump the component state log is an addendum to the
> current trace events, before that code clears the event record.
> 
> Something roughly like this:

Thank you for suggesting this approach.
Next version V3, will add "clear log" for Component State Dump separately,
as suggested bellow. Probably as a separate patch.

> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 9adda4795eb7..498b2a0b3e76 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -941,7 +941,8 @@ static int cxl_clear_event_record(struct
> cxl_memdev_state *mds,
>  }
> 
>  static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> -                                   enum cxl_event_log_type type)
> +                                   enum cxl_event_log_type type,
> +                                   struct cxl_component_state_dump *csd)
>  {
>         struct cxl_memdev *cxlmd = mds->cxlds.cxlmd;
>         struct device *dev = mds->cxlds.dev;
> @@ -977,9 +978,12 @@ static void cxl_mem_get_records_log(struct
> cxl_memdev_state *mds,
>                 if (!nr_rec)
>                         break;
> 
> -               for (i = 0; i < nr_rec; i++)
> +               for (i = 0; i < nr_rec; i++) {
>                         __cxl_event_trace_record(cxlmd, type,
>                                                  &payload->records[i]);
> +                       if (is_event_referenced(csd, type, &payload->records[i]))
> +                               trace_csd(csd, ...);
> +               }
> 
>                 if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
>                         trace_cxl_overflow(cxlmd, type, payload);
> 
> ...but in general this cover letter needs to comment on the long term
> suitability of the ABI.
Will add more details to cover letter in V3.