[PATCH v2 0/3] cxl: BG operations and device sanitation

[PATCH v2 0/3] cxl: BG operations and device sanitation

[Davidlohr Bueso]

Hi,

This series adds support for the rest of the security related cxl operation
which are not strictly dependent on PMEM.

Patch 1 adds the required background cmd handling bits. While this is currently
only polling support, the idea is to follow up with an irq support once the
bits land in Ira's event work. All polling-specific code has been marked
as __maybe_unused.

Patch 2 implements the sanitation commands (overwrite and secure erase).

Patch 3 adds a support for secure erase mocking.

As for testing, while I used the mock device to test the secure erase command, I
ended up hacking up a prototype[0] for qemu to support overwrite and bg commands.
So while the some of the paths this series introduces have been exercised, there
is of course a lot more to do.

Applies on top of the 'for-6.2/cxl-security' in cxl.git.

Changes from rfc (https://lore.kernel.org/linux-cxl/20220804045029.2905056-1-dave@stgolabs.net/):

* Patch 1:
 - Added a cxlds->mbox_irq flag in order to separate the polling-specific requirements.
 - Made bg workqueue concurrent.
 - Address iteration mem commands style (Jonathan). Now just rely on cxl_mem_find_command + check
   bg flag + callback.
 - Don't error in cxl_mbox_check_cmd_bgcmd() if conflicts() callback is not defined (Jonathan).
 - Error out in cxl_mbox_send_cmd() upon corruption in cxl_mbox_bgcmd_begin() (Jonathan).
 - Do not prematurely add all bg capable opcodes (Jonathan).

* Patch 2:
  - Rework polling delay hinting (Dave).
  - Rely on CEL to see if command is supported instead of unconditionally sending it (Jonathan).

* Added patch 3.

[0]: https://github.com/davidlohr/qemu/commit/64a93a5b824b59d3b547f06f7fbb1269fb4790ce

Thanks!

Davidlohr Bueso (3):
  cxl/mbox: Add background operation handling machinery
  cxl/mem: Support sanitation commands
  tools/testing/cxl: Add "Secure Erase" opcode support

 Documentation/ABI/testing/sysfs-bus-cxl |  19 ++
 drivers/cxl/core/core.h                 |   3 +-
 drivers/cxl/core/mbox.c                 | 317 +++++++++++++++++++++++-
 drivers/cxl/core/memdev.c               |  54 ++++
 drivers/cxl/core/port.c                 |   1 +
 drivers/cxl/cxl.h                       |   8 +
 drivers/cxl/cxlmem.h                    |  65 ++++-
 drivers/cxl/pci.c                       |   4 +
 drivers/cxl/pmem.c                      |   5 +-
 drivers/cxl/security.c                  |  13 +-
 include/uapi/linux/cxl_mem.h            |   2 +
 tools/testing/cxl/test/mem.c            |  27 ++
 12 files changed, 497 insertions(+), 21 deletions(-)

--
2.38.1

[PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Davidlohr Bueso]

This adds support for handling background operations, as defined in
the CXL 3.0 spec. Commands that can take too long (over ~2 seconds)
can run asynchronously in the background, these are limited to:
transfer/activate firmware, scan media, sanitize (aka overwrite)
and VPPB bind/unbind.

Completion of background operations, successful or not, can be handled
via irq or poll based. This patch deals only with polling, which
introduces some complexities as we need to handle the window in which
the original background command completed, then a new one is
successfully started before the poller wakes up and checks. This,
in theory, can occur any amount of times. One of the problems is
that previous states cannot be tracked as hw background status
registers always deal with the last command.

So in order to keep hardware and the driver in sync, there can be
windows where the hardware is ready but the driver won't be, and
thus must serialize/handle it accordingly. While this polling cost
may not be ideal: 1) background commands are rare/limited and
2) the extra busy time should be small compared to the overall
command duration and 3) devices that support mailbox interrupts
do not use this.

The implementation extends struct cxl_mem_command to become aware
of background-capable commands in a generic fashion and presents
some interfaces:

- Calls for bg operations, where each bg command can choose to implement
  whatever needed based on the nature of the operation. For example,
  while running, overwrite may only allow certain related commands to
  occur, while scan media does not have any such limitations.
  Delay times can also be different, for which ad-hoc hinting can
  be defined - for example, scan media could use some value based on
  GET_SCAN_MEDIA_CAPS and overwrite has predefined values based on
  pmem DSM specs[0]. Similarly some commands need to execute tasks
  once the command finishes, such as overwriting requires CPU flushing
  when successfully done. These are:

  cxl_mbox_bgcmd_conflicts()
  cxl_mbox_bgcmd_delay()
  cxl_mbox_bgcmd_post()

- cxl_mbox_send_cmd() is extended such that callers can distinguish,
  upon rc == 0, between completed and successfully started in the
  background.

- cxl_mbox_bgcmd_running() will atomically tell which command is
  running in the background, if any. This allows easy querying
  functionality. Similarly, there are cxl_mbox_bgcmd_start() and
  cxl_mbox_bgcmd_done() to safely mark the in-flight operation.
  While x86 serializes atomics, care must be taken with arm64, for
  example, ensuring, minimally, release/acquire semantics.

There are currently no supported commands.

[0] https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/core.h |   3 +-
 drivers/cxl/core/mbox.c | 213 ++++++++++++++++++++++++++++++++++++++--
 drivers/cxl/core/port.c |   1 +
 drivers/cxl/cxl.h       |   8 ++
 drivers/cxl/cxlmem.h    |  55 ++++++++++-
 drivers/cxl/pci.c       |   4 +
 drivers/cxl/pmem.c      |   5 +-
 drivers/cxl/security.c  |  13 +--
 8 files changed, 282 insertions(+), 20 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 1d8f87be283f..2a71664a0668 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -69,6 +69,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
 
 int cxl_memdev_init(void);
 void cxl_memdev_exit(void);
-void cxl_mbox_init(void);
+int cxl_mbox_init(void);
+void cxl_mbox_exit(void);
 
 #endif /* __CXL_CORE_H__ */
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 35dd889f1d3a..bfee9251c81c 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -10,6 +10,7 @@
 #include "core.h"
 
 static bool cxl_raw_allow_all;
+static struct workqueue_struct *cxl_mbox_bgpoll_wq;
 
 /**
  * DOC: cxl mbox
@@ -24,7 +25,7 @@ static bool cxl_raw_allow_all;
 	for ((cmd) = &cxl_mem_commands[0];                                     \
 	     ((cmd) - cxl_mem_commands) < ARRAY_SIZE(cxl_mem_commands); (cmd)++)
 
-#define CXL_CMD(_id, sin, sout, _flags)                                        \
+#define __CXL_CMD(_id, sin, sout, _flags, _bgops)                              \
 	[CXL_MEM_COMMAND_ID_##_id] = {                                         \
 	.info =	{                                                              \
 			.id = CXL_MEM_COMMAND_ID_##_id,                        \
@@ -33,8 +34,13 @@ static bool cxl_raw_allow_all;
 		},                                                             \
 	.opcode = CXL_MBOX_OP_##_id,                                           \
 	.flags = _flags,                                                       \
+	.bgops = _bgops,						       \
 	}
 
+#define CXL_CMD(_id, sin, sout, _flags) __CXL_CMD(_id, sin, sout, _flags, NULL)
+#define CXL_BGCMD(_id, sin, sout, _flags, _bgops)                              \
+	__CXL_CMD(_id, sin, sout, _flags | CXL_CMD_FLAG_BACKGROUND, _bgops)
+
 #define CXL_VARIABLE_PAYLOAD	~0U
 /*
  * This table defines the supported mailbox commands for the driver. This table
@@ -63,7 +69,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
 	CXL_CMD(INJECT_POISON, 0x8, 0, 0),
 	CXL_CMD(CLEAR_POISON, 0x48, 0, 0),
 	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
-	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
+	CXL_BGCMD(SCAN_MEDIA, 0x11, 0, 0, NULL),
 	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
 	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
 	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
@@ -145,6 +151,136 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
 	return cxl_command_names[c->info.id].name;
 }
 
+static unsigned long __maybe_unused
+cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode)
+{
+	struct cxl_mem_command *c;
+	unsigned long ret = 0;
+
+	c = cxl_mem_find_command(opcode);
+	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
+		dev_WARN_ONCE(cxlds->dev, true,
+			      "Not a background command\n");
+		return 0;
+	}
+
+	if (c->bgops && c->bgops->delay)
+		ret = c->bgops->delay(cxlds);
+
+	return ret * HZ;
+}
+
+static void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds,
+				u16 opcode, bool success)
+{
+	struct cxl_mem_command *c;
+
+	c = cxl_mem_find_command(opcode);
+	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
+		dev_WARN_ONCE(cxlds->dev, true,
+			      "Not a background command\n");
+		return;
+	}
+
+	if (c->bgops && c->bgops->post)
+		c->bgops->post(cxlds, success);
+}
+
+/*
+ * Ensure that ->mbox_send(@new) can run safely when a background
+ * command is running. If so, returns zero, otherwise error.
+ *
+ * check 1. bg cmd running. If not running it is fine to
+ *	    send any command. If one is running then there
+ *	    may be restrictions.
+ * check 2. @new incoming command is capable of running
+ *          in the background. If there is an in-flight bg
+ *          operation, all these are forbidden as we need
+ *          to serialize when polling.
+ * check 3. @new incoming command is not blacklisted by the
+ *          current running command.
+ */
+static int __maybe_unused
+cxl_mbox_check_cmd_bgcmd(struct cxl_dev_state *cxlds, u16 new)
+{
+	struct cxl_mem_command *c;
+
+	/* 1 */
+	if (likely(!cxl_mbox_bgcmd_running(cxlds)))
+		return 0;
+
+	c = cxl_mem_find_command(new);
+	if (!c)
+		return -EINVAL;
+
+	/* 2 */
+	if (c->flags & CXL_CMD_FLAG_BACKGROUND)
+		return -EBUSY;
+
+	/* 3 */
+	if (c->bgops && c->bgops->conflicts)
+		return c->bgops->conflicts(new);
+
+	return 0;
+}
+
+/*
+ * Background operation polling mode.
+ */
+void cxl_mbox_bgcmd_work(struct work_struct *work)
+{
+	struct cxl_dev_state *cxlds;
+	u64 bgcmd_status_reg;
+	u16 opcode;
+	u32 pct;
+
+	cxlds = container_of(work, struct cxl_dev_state, mbox_bgpoll.work);
+
+	dev_WARN_ONCE(cxlds->dev, !!cxlds->mbox_irq,
+		      "Polling mailbox when IRQs are supported\n");
+
+	bgcmd_status_reg = readq(cxlds->regs.mbox +
+				 CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
+	opcode = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK,
+			   bgcmd_status_reg);
+
+	pct = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK, bgcmd_status_reg);
+	if (pct != 100) {
+		unsigned long hint;
+		u64 status_reg;
+
+		status_reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_STATUS_OFFSET);
+		if (!FIELD_GET(CXLDEV_MBOX_STATUS_BACKGROUND_OPERATION,
+			       status_reg))
+			dev_WARN(cxlds->dev,
+				 "No background operation running (%u%%).\n",
+				 pct);
+
+		hint = cxl_mbox_bgcmd_delay(cxlds, opcode);
+		if (hint == 0) {
+			/*
+			 * TODO: try to do better(?). pct is updated every
+			 * ~1 sec, maybe use a heurstic based on that.
+			 */
+			hint = 5 * HZ;
+		}
+
+		queue_delayed_work(cxl_mbox_bgpoll_wq,
+				   &cxlds->mbox_bgpoll, hint);
+	} else { /* bg operation completed */
+		u16 return_code;
+
+		return_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
+					bgcmd_status_reg);
+		cxl_mbox_bgcmd_post(cxlds, opcode,
+				    return_code == CXL_MBOX_CMD_RC_SUCCESS);
+
+		put_device(cxlds->dev);
+		cxl_mbox_bgcmd_done(cxlds);
+	}
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mbox_bgcmd_work, CXL);
+
 /**
  * cxl_mbox_send_cmd() - Send a mailbox command to a device.
  * @cxlds: The device data for the operation
@@ -153,6 +289,7 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
  * @in_size: The length of the input payload
  * @out: Caller allocated buffer for the output.
  * @out_size: Expected size of output.
+ * @return_code: (optional output) HW returned code.
  *
  * Context: Any context.
  * Return:
@@ -167,8 +304,9 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
  * error. While this distinction can be useful for commands from userspace, the
  * kernel will only be able to use results when both are successful.
  */
-int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
-		      size_t in_size, void *out, size_t out_size)
+int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode,
+		      void *in, size_t in_size,
+		      void *out, size_t out_size, u16 *return_code)
 {
 	const struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
 	struct cxl_mbox_cmd mbox_cmd = {
@@ -183,12 +321,53 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
 	if (in_size > cxlds->payload_size || out_size > cxlds->payload_size)
 		return -E2BIG;
 
+	/*
+	 * With bg polling this can overlap a scenario where the
+	 * hardware can receive new requests but the driver is
+	 * not ready. Handle accordingly.
+	 */
+	if (!cxlds->mbox_irq) {
+		rc = cxl_mbox_check_cmd_bgcmd(cxlds, opcode);
+		if (rc)
+			return rc;
+	}
+
 	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
+	if (return_code)
+		*return_code = mbox_cmd.return_code;
 	if (rc)
 		return rc;
 
-	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
+	switch (mbox_cmd.return_code) {
+	case CXL_MBOX_CMD_RC_BACKGROUND:
+	{
+		int err;
+
+		dev_dbg(cxlds->dev, "Opcode 0x%04x: %s\n", opcode,
+			cxl_mbox_cmd_rc2str(&mbox_cmd));
+
+		err = cxl_mbox_bgcmd_begin(cxlds, opcode);
+		if (err) {
+			dev_WARN(cxlds->dev,
+				 "Corrupted background cmd (opcode 0x%04x)\n",
+				 err);
+			return -ENXIO;
+		}
+
+		get_device(cxlds->dev);
+
+		if (!cxlds->mbox_irq) {
+			/* do first poll check asap before using any hinting */
+			queue_delayed_work(cxl_mbox_bgpoll_wq,
+					   &cxlds->mbox_bgpoll, 0);
+		}
+		break;
+	}
+	case CXL_MBOX_CMD_RC_SUCCESS:
+		break;
+	default:
 		return cxl_mbox_cmd_rc2errno(&mbox_cmd);
+	}
 
 	/*
 	 * Variable sized commands can't be validated and so it's up to the
@@ -575,7 +754,7 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
 		int rc;
 
 		rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LOG, &log, sizeof(log),
-				       out, xfer_size);
+				       out, xfer_size, NULL);
 		if (rc < 0)
 			return rc;
 
@@ -628,7 +807,7 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxl
 		return ERR_PTR(-ENOMEM);
 
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SUPPORTED_LOGS, NULL, 0, ret,
-			       cxlds->payload_size);
+			       cxlds->payload_size, NULL);
 	if (rc < 0) {
 		kvfree(ret);
 		return ERR_PTR(rc);
@@ -738,7 +917,7 @@ static int cxl_mem_get_partition_info(struct cxl_dev_state *cxlds)
 	int rc;
 
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_PARTITION_INFO, NULL, 0,
-			       &pi, sizeof(pi));
+			       &pi, sizeof(pi), NULL);
 
 	if (rc)
 		return rc;
@@ -771,7 +950,7 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
 	int rc;
 
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_IDENTIFY, NULL, 0, &id,
-			       sizeof(id));
+			       sizeof(id), NULL);
 	if (rc < 0)
 		return rc;
 
@@ -868,11 +1047,25 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_dev_state_create, CXL);
 
-void __init cxl_mbox_init(void)
+int __init cxl_mbox_init(void)
 {
 	struct dentry *mbox_debugfs;
 
 	mbox_debugfs = cxl_debugfs_create_dir("mbox");
 	debugfs_create_bool("raw_allow_all", 0600, mbox_debugfs,
 			    &cxl_raw_allow_all);
+
+	/* background commands can run concurrently on different devices */
+	cxl_mbox_bgpoll_wq = alloc_workqueue("cxl_mbox_bgcmd", WQ_UNBOUND, 0);
+	if (!cxl_mbox_bgpoll_wq) {
+		debugfs_remove_recursive(mbox_debugfs);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void cxl_mbox_exit(void)
+{
+	destroy_workqueue(cxl_mbox_bgpoll_wq);
 }
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 0d2f5eaaca7d..449767bfb5aa 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1945,6 +1945,7 @@ static void cxl_core_exit(void)
 	destroy_workqueue(cxl_bus_wq);
 	cxl_memdev_exit();
 	debugfs_remove_recursive(cxl_debugfs);
+	cxl_mbox_exit();
 }
 
 module_init(cxl_core_init);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index e5e1abceeca7..a10e979ef3a4 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -139,14 +139,22 @@ static inline int ways_to_cxl(unsigned int ways, u8 *iw)
 /* CXL 2.0 8.2.8.4 Mailbox Registers */
 #define CXLDEV_MBOX_CAPS_OFFSET 0x00
 #define   CXLDEV_MBOX_CAP_PAYLOAD_SIZE_MASK GENMASK(4, 0)
+#define   CXLDEV_MBOX_CAP_BG_CMD_IRQNUM_MASK GENMASK(10, 7)
+#define   CXLDEV_MBOX_CAP_BG_CMD_IRQ BIT(6)
 #define CXLDEV_MBOX_CTRL_OFFSET 0x04
 #define   CXLDEV_MBOX_CTRL_DOORBELL BIT(0)
+#define   CXLDEV_MBOX_CTRL_BGCMD_IRQ BIT(2)
 #define CXLDEV_MBOX_CMD_OFFSET 0x08
 #define   CXLDEV_MBOX_CMD_COMMAND_OPCODE_MASK GENMASK_ULL(15, 0)
 #define   CXLDEV_MBOX_CMD_PAYLOAD_LENGTH_MASK GENMASK_ULL(36, 16)
 #define CXLDEV_MBOX_STATUS_OFFSET 0x10
+#define   CXLDEV_MBOX_STATUS_BACKGROUND_OPERATION BIT(0)
 #define   CXLDEV_MBOX_STATUS_RET_CODE_MASK GENMASK_ULL(47, 32)
+/* CXL 3.0 8.2.8.4.7 Background Command Status Register */
 #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
+#define   CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK GENMASK_ULL(15, 0)
+#define   CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK GENMASK_ULL(22, 16)
+#define   CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK GENMASK_ULL(47, 32)
 #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
 
 /*
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 75baeb0bbe57..e7cb2f2fadc4 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -212,6 +212,9 @@ struct cxl_endpoint_dvsec_info {
  * @serial: PCIe Device Serial Number
  * @doe_mbs: PCI DOE mailbox array
  * @mbox_send: @dev specific transport for transmitting mailbox commands
+ * @mbox_irq: @dev supports mailbox interrupts
+ * @mbox_bg: opcode for the in-flight background operation on @dev
+ * @mbox_bgpoll: self-polling delayed work item
  *
  * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
  * details on capacity parameters.
@@ -248,6 +251,9 @@ struct cxl_dev_state {
 	struct xarray doe_mbs;
 
 	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
+	bool mbox_irq;
+	atomic_t mbox_bg;
+	struct delayed_work __maybe_unused mbox_bgpoll;
 };
 
 enum cxl_opcode {
@@ -290,6 +296,26 @@ enum cxl_opcode {
 	UUID_INIT(0xe1819d9, 0x11a9, 0x400c, 0x81, 0x1f, 0xd6, 0x07, 0x19,     \
 		  0x40, 0x3d, 0x86)
 
+/*
+ * Supported CXL mailbox commands that can run in the background.
+ *
+ * Barriers pair with release/acquire semantics. When done,
+ * clearing ->mbox_bg must be the very last operation before
+ * finishing the command.
+ */
+static inline int cxl_mbox_bgcmd_begin(struct cxl_dev_state *cxlds, u16 opcode)
+{
+	return atomic_xchg(&cxlds->mbox_bg, opcode);
+}
+static inline int cxl_mbox_bgcmd_running(struct cxl_dev_state *cxlds)
+{
+	return atomic_read_acquire(&cxlds->mbox_bg);
+}
+static inline void cxl_mbox_bgcmd_done(struct cxl_dev_state *cxlds)
+{
+	atomic_set_release(&cxlds->mbox_bg, 0);
+}
+
 struct cxl_mbox_get_supported_logs {
 	__le16 entries;
 	u8 rsvd[6];
@@ -353,16 +379,38 @@ struct cxl_mbox_set_partition_info {
 
 #define  CXL_SET_PARTITION_IMMEDIATE_FLAG	BIT(0)
 
+/**
+ * struct cxl_mbox_bgcmd_ops - Optional ad-hoc handling of background cmds
+ * @post: Execute after the command completes
+ * @conflicts: How to handle a @new command when one is currently executing
+ *             (only for polling mode)
+ * @delay: Delay hint for polling on command completion
+ */
+struct cxl_mem_bgcommand_ops {
+	void (*post)(struct cxl_dev_state *cxlds, bool success);
+	int (*conflicts)(u16 new);
+	unsigned long (*delay)(struct cxl_dev_state *cxlds);
+};
+
 /**
  * struct cxl_mem_command - Driver representation of a memory device command
  * @info: Command information as it exists for the UAPI
  * @opcode: The actual bits used for the mailbox protocol
  * @flags: Set of flags effecting driver behavior.
+ * @bg_ops: Set of optional callbacks to handle commands that can run in
+ *          the background.
  *
  *  * %CXL_CMD_FLAG_FORCE_ENABLE: In cases of error, commands with this flag
  *    will be enabled by the driver regardless of what hardware may have
  *    advertised.
  *
+ *  * %CXL_CMD_FLAG_BACKGROUND: The command may execute asynchronously in the
+ *    background if the operation takes longer than ~2 seconds. The semantics
+ *    are:
+ *      - Only a single of these commands can run at a time.
+ *      - Depending on the nature of the operation, devices may continue to
+ *        accept new non-background commands.
+ *
  * The cxl_mem_command is the driver's internal representation of commands that
  * are supported by the driver. Some of these commands may not be supported by
  * the hardware. The driver will use @info to validate the fields passed in by
@@ -374,8 +422,10 @@ struct cxl_mem_command {
 	struct cxl_command_info info;
 	enum cxl_opcode opcode;
 	u32 flags;
+	struct cxl_mem_bgcommand_ops __maybe_unused *bgops;
 #define CXL_CMD_FLAG_NONE 0
 #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
+#define CXL_CMD_FLAG_BACKGROUND BIT(1)
 };
 
 #define CXL_PMEM_SEC_STATE_USER_PASS_SET	0x01
@@ -414,7 +464,8 @@ enum {
 };
 
 int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
-		      size_t in_size, void *out, size_t out_size);
+		      size_t in_size, void *out, size_t out_size,
+		      u16 *return_code);
 int cxl_dev_state_identify(struct cxl_dev_state *cxlds);
 int cxl_await_media_ready(struct cxl_dev_state *cxlds);
 int cxl_enumerate_cmds(struct cxl_dev_state *cxlds);
@@ -434,6 +485,8 @@ static inline void cxl_mem_active_dec(void)
 }
 #endif
 
+void cxl_mbox_bgcmd_work(struct work_struct *work);
+
 struct cxl_hdm {
 	struct cxl_component_regs regs;
 	unsigned int decoder_count;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 621a0522b554..b21d8681beac 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -268,6 +268,10 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
 		return -ENXIO;
 	}
 
+	atomic_set(&cxlds->mbox_bg, 0);
+	if (!cxlds->mbox_irq)
+		INIT_DELAYED_WORK(&cxlds->mbox_bgpoll, cxl_mbox_bgcmd_work);
+
 	dev_dbg(cxlds->dev, "Mailbox payload sized %zu",
 		cxlds->payload_size);
 
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index ab40c93c44e5..c6a6d3fd6883 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -173,7 +173,8 @@ static int cxl_pmem_get_config_data(struct cxl_dev_state *cxlds,
 	};
 
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LSA, &get_lsa,
-			       sizeof(get_lsa), cmd->out_buf, cmd->in_length);
+			       sizeof(get_lsa), cmd->out_buf,
+			       cmd->in_length, NULL);
 	cmd->status = 0;
 
 	return rc;
@@ -205,7 +206,7 @@ static int cxl_pmem_set_config_data(struct cxl_dev_state *cxlds,
 
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_LSA, set_lsa,
 			       struct_size(set_lsa, data, cmd->in_length),
-			       NULL, 0);
+			       NULL, 0, NULL);
 
 	/*
 	 * Set "firmware" status (4-packed bytes at the end of the input
diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
index 5484d4eecfd1..825d1bd1dd16 100644
--- a/drivers/cxl/security.c
+++ b/drivers/cxl/security.c
@@ -20,7 +20,7 @@ static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
 	int rc;
 
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE, NULL, 0,
-			       &sec_out, sizeof(sec_out));
+			       &sec_out, sizeof(sec_out), NULL);
 	if (rc < 0)
 		return 0;
 
@@ -67,7 +67,7 @@ static int cxl_pmem_security_change_key(struct nvdimm *nvdimm,
 	memcpy(set_pass.new_pass, new_data->data, NVDIMM_PASSPHRASE_LEN);
 
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_PASSPHRASE,
-			       &set_pass, sizeof(set_pass), NULL, 0);
+			       &set_pass, sizeof(set_pass), NULL, 0, NULL);
 	return rc;
 }
 
@@ -86,7 +86,7 @@ static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
 	memcpy(dis_pass.pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
 
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_DISABLE_PASSPHRASE,
-			       &dis_pass, sizeof(dis_pass), NULL, 0);
+			       &dis_pass, sizeof(dis_pass), NULL, 0, NULL);
 	return rc;
 }
 
@@ -108,7 +108,8 @@ static int cxl_pmem_security_freeze(struct nvdimm *nvdimm)
 	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 
-	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0);
+	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY,
+				 NULL, 0, NULL, 0, NULL);
 }
 
 static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
@@ -122,7 +123,7 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
 
 	memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_UNLOCK,
-			       pass, NVDIMM_PASSPHRASE_LEN, NULL, 0);
+			       pass, NVDIMM_PASSPHRASE_LEN, NULL, 0, NULL);
 	if (rc < 0)
 		return rc;
 
@@ -143,7 +144,7 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
 		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
 	memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN);
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
-			       &erase, sizeof(erase), NULL, 0);
+			       &erase, sizeof(erase), NULL, 0, NULL);
 	if (rc < 0)
 		return rc;
 
-- 
2.38.1

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Dave Jiang]

On 12/5/2022 6:14 PM, Davidlohr Bueso wrote:
> This adds support for handling background operations, as defined in
> the CXL 3.0 spec. Commands that can take too long (over ~2 seconds)
> can run asynchronously in the background, these are limited to:
> transfer/activate firmware, scan media, sanitize (aka overwrite)
> and VPPB bind/unbind.
> 
> Completion of background operations, successful or not, can be handled
> via irq or poll based. This patch deals only with polling, which
> introduces some complexities as we need to handle the window in which
> the original background command completed, then a new one is
> successfully started before the poller wakes up and checks. This,
> in theory, can occur any amount of times. One of the problems is
> that previous states cannot be tracked as hw background status
> registers always deal with the last command.
> 
> So in order to keep hardware and the driver in sync, there can be
> windows where the hardware is ready but the driver won't be, and
> thus must serialize/handle it accordingly. While this polling cost
> may not be ideal: 1) background commands are rare/limited and
> 2) the extra busy time should be small compared to the overall
> command duration and 3) devices that support mailbox interrupts
> do not use this.
> 
> The implementation extends struct cxl_mem_command to become aware
> of background-capable commands in a generic fashion and presents
> some interfaces:
> 
> - Calls for bg operations, where each bg command can choose to implement
>    whatever needed based on the nature of the operation. For example,
>    while running, overwrite may only allow certain related commands to
>    occur, while scan media does not have any such limitations.
>    Delay times can also be different, for which ad-hoc hinting can
>    be defined - for example, scan media could use some value based on
>    GET_SCAN_MEDIA_CAPS and overwrite has predefined values based on
>    pmem DSM specs[0]. Similarly some commands need to execute tasks
>    once the command finishes, such as overwriting requires CPU flushing
>    when successfully done. These are:
> 
>    cxl_mbox_bgcmd_conflicts()
>    cxl_mbox_bgcmd_delay()
>    cxl_mbox_bgcmd_post()
> 
> - cxl_mbox_send_cmd() is extended such that callers can distinguish,
>    upon rc == 0, between completed and successfully started in the
>    background.
> 
> - cxl_mbox_bgcmd_running() will atomically tell which command is
>    running in the background, if any. This allows easy querying
>    functionality. Similarly, there are cxl_mbox_bgcmd_start() and
>    cxl_mbox_bgcmd_done() to safely mark the in-flight operation.
>    While x86 serializes atomics, care must be taken with arm64, for
>    example, ensuring, minimally, release/acquire semantics.
> 
> There are currently no supported commands.
> 
> [0] https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>   drivers/cxl/core/core.h |   3 +-
>   drivers/cxl/core/mbox.c | 213 ++++++++++++++++++++++++++++++++++++++--
>   drivers/cxl/core/port.c |   1 +
>   drivers/cxl/cxl.h       |   8 ++
>   drivers/cxl/cxlmem.h    |  55 ++++++++++-
>   drivers/cxl/pci.c       |   4 +
>   drivers/cxl/pmem.c      |   5 +-
>   drivers/cxl/security.c  |  13 +--
>   8 files changed, 282 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 1d8f87be283f..2a71664a0668 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -69,6 +69,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>   
>   int cxl_memdev_init(void);
>   void cxl_memdev_exit(void);
> -void cxl_mbox_init(void);
> +int cxl_mbox_init(void);
> +void cxl_mbox_exit(void);
>   
>   #endif /* __CXL_CORE_H__ */
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 35dd889f1d3a..bfee9251c81c 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -10,6 +10,7 @@
>   #include "core.h"
>   
>   static bool cxl_raw_allow_all;
> +static struct workqueue_struct *cxl_mbox_bgpoll_wq;

Do we want per device wq rather than a global one? Just thinking if you 
have a device go away and you need to flush outstanding commands, you 
might impact the other devices.

>   
>   /**
>    * DOC: cxl mbox
> @@ -24,7 +25,7 @@ static bool cxl_raw_allow_all;
>   	for ((cmd) = &cxl_mem_commands[0];                                     \
>   	     ((cmd) - cxl_mem_commands) < ARRAY_SIZE(cxl_mem_commands); (cmd)++)
>   
> -#define CXL_CMD(_id, sin, sout, _flags)                                        \
> +#define __CXL_CMD(_id, sin, sout, _flags, _bgops)                              \
>   	[CXL_MEM_COMMAND_ID_##_id] = {                                         \
>   	.info =	{                                                              \
>   			.id = CXL_MEM_COMMAND_ID_##_id,                        \
> @@ -33,8 +34,13 @@ static bool cxl_raw_allow_all;
>   		},                                                             \
>   	.opcode = CXL_MBOX_OP_##_id,                                           \
>   	.flags = _flags,                                                       \
> +	.bgops = _bgops,						       \
>   	}
>   
> +#define CXL_CMD(_id, sin, sout, _flags) __CXL_CMD(_id, sin, sout, _flags, NULL)
> +#define CXL_BGCMD(_id, sin, sout, _flags, _bgops)                              \
> +	__CXL_CMD(_id, sin, sout, _flags | CXL_CMD_FLAG_BACKGROUND, _bgops)
> +
>   #define CXL_VARIABLE_PAYLOAD	~0U
>   /*
>    * This table defines the supported mailbox commands for the driver. This table
> @@ -63,7 +69,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>   	CXL_CMD(INJECT_POISON, 0x8, 0, 0),
>   	CXL_CMD(CLEAR_POISON, 0x48, 0, 0),
>   	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
> -	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
> +	CXL_BGCMD(SCAN_MEDIA, 0x11, 0, 0, NULL),
>   	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
>   	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
>   	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
> @@ -145,6 +151,136 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>   	return cxl_command_names[c->info.id].name;
>   }
>   
> +static unsigned long __maybe_unused
> +cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode)
> +{
> +	struct cxl_mem_command *c;
> +	unsigned long ret = 0;
> +
> +	c = cxl_mem_find_command(opcode);
> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
> +		dev_WARN_ONCE(cxlds->dev, true,
> +			      "Not a background command\n");
> +		return 0;
> +	}
> +
> +	if (c->bgops && c->bgops->delay)
> +		ret = c->bgops->delay(cxlds);
> +
> +	return ret * HZ;
> +}
> +
> +static void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds,
> +				u16 opcode, bool success)
> +{
> +	struct cxl_mem_command *c;
> +
> +	c = cxl_mem_find_command(opcode);
> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
> +		dev_WARN_ONCE(cxlds->dev, true,
> +			      "Not a background command\n");
> +		return;
> +	}
> +
> +	if (c->bgops && c->bgops->post)
> +		c->bgops->post(cxlds, success);
> +}
> +
> +/*
> + * Ensure that ->mbox_send(@new) can run safely when a background
> + * command is running. If so, returns zero, otherwise error.
> + *
> + * check 1. bg cmd running. If not running it is fine to
> + *	    send any command. If one is running then there
> + *	    may be restrictions.
> + * check 2. @new incoming command is capable of running
> + *          in the background. If there is an in-flight bg
> + *          operation, all these are forbidden as we need
> + *          to serialize when polling.
> + * check 3. @new incoming command is not blacklisted by the
> + *          current running command.
> + */
> +static int __maybe_unused
> +cxl_mbox_check_cmd_bgcmd(struct cxl_dev_state *cxlds, u16 new)
> +{
> +	struct cxl_mem_command *c;
> +
> +	/* 1 */
> +	if (likely(!cxl_mbox_bgcmd_running(cxlds)))
> +		return 0;
> +
> +	c = cxl_mem_find_command(new);
> +	if (!c)
> +		return -EINVAL;
> +
> +	/* 2 */
> +	if (c->flags & CXL_CMD_FLAG_BACKGROUND)
> +		return -EBUSY;
> +
> +	/* 3 */
> +	if (c->bgops && c->bgops->conflicts)
> +		return c->bgops->conflicts(new);
> +
> +	return 0;
> +}
> +
> +/*
> + * Background operation polling mode.
> + */
> +void cxl_mbox_bgcmd_work(struct work_struct *work)
> +{
> +	struct cxl_dev_state *cxlds;
> +	u64 bgcmd_status_reg;
> +	u16 opcode;
> +	u32 pct;
> +
> +	cxlds = container_of(work, struct cxl_dev_state, mbox_bgpoll.work);
> +
> +	dev_WARN_ONCE(cxlds->dev, !!cxlds->mbox_irq,
> +		      "Polling mailbox when IRQs are supported\n");
> +
> +	bgcmd_status_reg = readq(cxlds->regs.mbox +
> +				 CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> +	opcode = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK,
> +			   bgcmd_status_reg);
> +
> +	pct = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK, bgcmd_status_reg);
> +	if (pct != 100) {
> +		unsigned long hint;
> +		u64 status_reg;
> +
> +		status_reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_STATUS_OFFSET);
> +		if (!FIELD_GET(CXLDEV_MBOX_STATUS_BACKGROUND_OPERATION,
> +			       status_reg))
> +			dev_WARN(cxlds->dev,
> +				 "No background operation running (%u%%).\n",
> +				 pct);
> +
> +		hint = cxl_mbox_bgcmd_delay(cxlds, opcode);
> +		if (hint == 0) {
> +			/*
> +			 * TODO: try to do better(?). pct is updated every
> +			 * ~1 sec, maybe use a heurstic based on that.
> +			 */
> +			hint = 5 * HZ;
> +		}
> +
> +		queue_delayed_work(cxl_mbox_bgpoll_wq,
> +				   &cxlds->mbox_bgpoll, hint);
> +	} else { /* bg operation completed */
> +		u16 return_code;
> +
> +		return_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
> +					bgcmd_status_reg);
> +		cxl_mbox_bgcmd_post(cxlds, opcode,
> +				    return_code == CXL_MBOX_CMD_RC_SUCCESS);
> +
> +		put_device(cxlds->dev);
> +		cxl_mbox_bgcmd_done(cxlds);
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mbox_bgcmd_work, CXL);
> +
>   /**
>    * cxl_mbox_send_cmd() - Send a mailbox command to a device.
>    * @cxlds: The device data for the operation
> @@ -153,6 +289,7 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>    * @in_size: The length of the input payload
>    * @out: Caller allocated buffer for the output.
>    * @out_size: Expected size of output.
> + * @return_code: (optional output) HW returned code.
>    *
>    * Context: Any context.
>    * Return:
> @@ -167,8 +304,9 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>    * error. While this distinction can be useful for commands from userspace, the
>    * kernel will only be able to use results when both are successful.
>    */
> -int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
> -		      size_t in_size, void *out, size_t out_size)
> +int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode,
> +		      void *in, size_t in_size,
> +		      void *out, size_t out_size, u16 *return_code)
>   {
>   	const struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>   	struct cxl_mbox_cmd mbox_cmd = {
> @@ -183,12 +321,53 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>   	if (in_size > cxlds->payload_size || out_size > cxlds->payload_size)
>   		return -E2BIG;
>   
> +	/*
> +	 * With bg polling this can overlap a scenario where the
> +	 * hardware can receive new requests but the driver is
> +	 * not ready. Handle accordingly.
> +	 */
> +	if (!cxlds->mbox_irq) {
> +		rc = cxl_mbox_check_cmd_bgcmd(cxlds, opcode);
> +		if (rc)
> +			return rc;
> +	}
> +
>   	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
> +	if (return_code)
> +		*return_code = mbox_cmd.return_code;
>   	if (rc)
>   		return rc;
>   
> -	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
> +	switch (mbox_cmd.return_code) {
> +	case CXL_MBOX_CMD_RC_BACKGROUND:
> +	{
> +		int err;
> +
> +		dev_dbg(cxlds->dev, "Opcode 0x%04x: %s\n", opcode,
> +			cxl_mbox_cmd_rc2str(&mbox_cmd));
> +
> +		err = cxl_mbox_bgcmd_begin(cxlds, opcode);
> +		if (err) {
> +			dev_WARN(cxlds->dev,
> +				 "Corrupted background cmd (opcode 0x%04x)\n",
> +				 err);
> +			return -ENXIO;
> +		}
> +
> +		get_device(cxlds->dev);
> +
> +		if (!cxlds->mbox_irq) {
> +			/* do first poll check asap before using any hinting */
> +			queue_delayed_work(cxl_mbox_bgpoll_wq,
> +					   &cxlds->mbox_bgpoll, 0);
> +		}
> +		break;
> +	}
> +	case CXL_MBOX_CMD_RC_SUCCESS:
> +		break;
> +	default:
>   		return cxl_mbox_cmd_rc2errno(&mbox_cmd);
> +	}
>   
>   	/*
>   	 * Variable sized commands can't be validated and so it's up to the
> @@ -575,7 +754,7 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
>   		int rc;
>   
>   		rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LOG, &log, sizeof(log),
> -				       out, xfer_size);
> +				       out, xfer_size, NULL);
>   		if (rc < 0)
>   			return rc;
>   
> @@ -628,7 +807,7 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxl
>   		return ERR_PTR(-ENOMEM);
>   
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SUPPORTED_LOGS, NULL, 0, ret,
> -			       cxlds->payload_size);
> +			       cxlds->payload_size, NULL);
>   	if (rc < 0) {
>   		kvfree(ret);
>   		return ERR_PTR(rc);
> @@ -738,7 +917,7 @@ static int cxl_mem_get_partition_info(struct cxl_dev_state *cxlds)
>   	int rc;
>   
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_PARTITION_INFO, NULL, 0,
> -			       &pi, sizeof(pi));
> +			       &pi, sizeof(pi), NULL);
>   
>   	if (rc)
>   		return rc;
> @@ -771,7 +950,7 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
>   	int rc;
>   
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_IDENTIFY, NULL, 0, &id,
> -			       sizeof(id));
> +			       sizeof(id), NULL);
>   	if (rc < 0)
>   		return rc;
>   
> @@ -868,11 +1047,25 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
>   }
>   EXPORT_SYMBOL_NS_GPL(cxl_dev_state_create, CXL);
>   
> -void __init cxl_mbox_init(void)
> +int __init cxl_mbox_init(void)
>   {
>   	struct dentry *mbox_debugfs;
>   
>   	mbox_debugfs = cxl_debugfs_create_dir("mbox");
>   	debugfs_create_bool("raw_allow_all", 0600, mbox_debugfs,
>   			    &cxl_raw_allow_all);
> +
> +	/* background commands can run concurrently on different devices */
> +	cxl_mbox_bgpoll_wq = alloc_workqueue("cxl_mbox_bgcmd", WQ_UNBOUND, 0);
> +	if (!cxl_mbox_bgpoll_wq) {
> +		debugfs_remove_recursive(mbox_debugfs);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +void cxl_mbox_exit(void)
> +{
> +	destroy_workqueue(cxl_mbox_bgpoll_wq);
>   }
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 0d2f5eaaca7d..449767bfb5aa 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1945,6 +1945,7 @@ static void cxl_core_exit(void)
>   	destroy_workqueue(cxl_bus_wq);
>   	cxl_memdev_exit();
>   	debugfs_remove_recursive(cxl_debugfs);
> +	cxl_mbox_exit();
>   }
>   
>   module_init(cxl_core_init);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index e5e1abceeca7..a10e979ef3a4 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -139,14 +139,22 @@ static inline int ways_to_cxl(unsigned int ways, u8 *iw)
>   /* CXL 2.0 8.2.8.4 Mailbox Registers */
>   #define CXLDEV_MBOX_CAPS_OFFSET 0x00
>   #define   CXLDEV_MBOX_CAP_PAYLOAD_SIZE_MASK GENMASK(4, 0)
> +#define   CXLDEV_MBOX_CAP_BG_CMD_IRQNUM_MASK GENMASK(10, 7)
> +#define   CXLDEV_MBOX_CAP_BG_CMD_IRQ BIT(6)
>   #define CXLDEV_MBOX_CTRL_OFFSET 0x04
>   #define   CXLDEV_MBOX_CTRL_DOORBELL BIT(0)
> +#define   CXLDEV_MBOX_CTRL_BGCMD_IRQ BIT(2)
>   #define CXLDEV_MBOX_CMD_OFFSET 0x08
>   #define   CXLDEV_MBOX_CMD_COMMAND_OPCODE_MASK GENMASK_ULL(15, 0)
>   #define   CXLDEV_MBOX_CMD_PAYLOAD_LENGTH_MASK GENMASK_ULL(36, 16)
>   #define CXLDEV_MBOX_STATUS_OFFSET 0x10
> +#define   CXLDEV_MBOX_STATUS_BACKGROUND_OPERATION BIT(0)
>   #define   CXLDEV_MBOX_STATUS_RET_CODE_MASK GENMASK_ULL(47, 32)
> +/* CXL 3.0 8.2.8.4.7 Background Command Status Register */
>   #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
> +#define   CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK GENMASK_ULL(15, 0)
> +#define   CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK GENMASK_ULL(22, 16)
> +#define   CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK GENMASK_ULL(47, 32)
>   #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
>   
>   /*
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 75baeb0bbe57..e7cb2f2fadc4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -212,6 +212,9 @@ struct cxl_endpoint_dvsec_info {
>    * @serial: PCIe Device Serial Number
>    * @doe_mbs: PCI DOE mailbox array
>    * @mbox_send: @dev specific transport for transmitting mailbox commands
> + * @mbox_irq: @dev supports mailbox interrupts
> + * @mbox_bg: opcode for the in-flight background operation on @dev
> + * @mbox_bgpoll: self-polling delayed work item
>    *
>    * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
>    * details on capacity parameters.
> @@ -248,6 +251,9 @@ struct cxl_dev_state {
>   	struct xarray doe_mbs;
>   
>   	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> +	bool mbox_irq;
> +	atomic_t mbox_bg;
> +	struct delayed_work __maybe_unused mbox_bgpoll;
>   };
>   
>   enum cxl_opcode {
> @@ -290,6 +296,26 @@ enum cxl_opcode {
>   	UUID_INIT(0xe1819d9, 0x11a9, 0x400c, 0x81, 0x1f, 0xd6, 0x07, 0x19,     \
>   		  0x40, 0x3d, 0x86)
>   
> +/*
> + * Supported CXL mailbox commands that can run in the background.
> + *
> + * Barriers pair with release/acquire semantics. When done,
> + * clearing ->mbox_bg must be the very last operation before
> + * finishing the command.
> + */
> +static inline int cxl_mbox_bgcmd_begin(struct cxl_dev_state *cxlds, u16 opcode)
> +{
> +	return atomic_xchg(&cxlds->mbox_bg, opcode);
> +}
> +static inline int cxl_mbox_bgcmd_running(struct cxl_dev_state *cxlds)
> +{
> +	return atomic_read_acquire(&cxlds->mbox_bg);
> +}
> +static inline void cxl_mbox_bgcmd_done(struct cxl_dev_state *cxlds)
> +{
> +	atomic_set_release(&cxlds->mbox_bg, 0);
> +}
> +
>   struct cxl_mbox_get_supported_logs {
>   	__le16 entries;
>   	u8 rsvd[6];
> @@ -353,16 +379,38 @@ struct cxl_mbox_set_partition_info {
>   
>   #define  CXL_SET_PARTITION_IMMEDIATE_FLAG	BIT(0)
>   
> +/**
> + * struct cxl_mbox_bgcmd_ops - Optional ad-hoc handling of background cmds
> + * @post: Execute after the command completes
> + * @conflicts: How to handle a @new command when one is currently executing
> + *             (only for polling mode)
> + * @delay: Delay hint for polling on command completion
> + */
> +struct cxl_mem_bgcommand_ops {
> +	void (*post)(struct cxl_dev_state *cxlds, bool success);

"post" here I think conveys different meaning than "after". Typically it 
is used in the context of issuing the command. To remove confusion, 
maybe call it "end_cmd" or "finish"?



> +	int (*conflicts)(u16 new);

has_conflicts() may be better. Also, it can return a bool to indicate 
whether there are conflicts.

> +	unsigned long (*delay)(struct cxl_dev_state *cxlds);

"poll_delay"?

> +};
> +
>   /**
>    * struct cxl_mem_command - Driver representation of a memory device command
>    * @info: Command information as it exists for the UAPI
>    * @opcode: The actual bits used for the mailbox protocol
>    * @flags: Set of flags effecting driver behavior.
> + * @bg_ops: Set of optional callbacks to handle commands that can run in
> + *          the background.
>    *
>    *  * %CXL_CMD_FLAG_FORCE_ENABLE: In cases of error, commands with this flag
>    *    will be enabled by the driver regardless of what hardware may have
>    *    advertised.
>    *
> + *  * %CXL_CMD_FLAG_BACKGROUND: The command may execute asynchronously in the
> + *    background if the operation takes longer than ~2 seconds. The semantics
> + *    are:
> + *      - Only a single of these commands can run at a time.
> + *      - Depending on the nature of the operation, devices may continue to
> + *        accept new non-background commands.
> + *
>    * The cxl_mem_command is the driver's internal representation of commands that
>    * are supported by the driver. Some of these commands may not be supported by
>    * the hardware. The driver will use @info to validate the fields passed in by
> @@ -374,8 +422,10 @@ struct cxl_mem_command {
>   	struct cxl_command_info info;
>   	enum cxl_opcode opcode;
>   	u32 flags;
> +	struct cxl_mem_bgcommand_ops __maybe_unused *bgops;
>   #define CXL_CMD_FLAG_NONE 0
>   #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
> +#define CXL_CMD_FLAG_BACKGROUND BIT(1)
>   };
>   
>   #define CXL_PMEM_SEC_STATE_USER_PASS_SET	0x01
> @@ -414,7 +464,8 @@ enum {
>   };
>   
>   int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
> -		      size_t in_size, void *out, size_t out_size);
> +		      size_t in_size, void *out, size_t out_size,
> +		      u16 *return_code);

Given the existing commands don't care about return_code and only the 
new ones do, and to minimize code changes and having to chase down every 
call of the existing command, you can maybe do something like:

int cxl_mbox_send_cmd_with_return(struct cxl_dev_state *cxlds,
				  u16 opcode, void *in, size_t in_size,
				  void *out, size_t out_size,
				  u16 *return_code)
{
	.....
}

int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds,
		      u16 opcode, void *in, size_t in_size,
		      void *out, size_t out_size)
{
	return cxl_mbox_send_cmd_with_return(cxlds, opcode, in, in_size,
					     out, out_size, NULL);
}



>   int cxl_dev_state_identify(struct cxl_dev_state *cxlds);
>   int cxl_await_media_ready(struct cxl_dev_state *cxlds);
>   int cxl_enumerate_cmds(struct cxl_dev_state *cxlds);
> @@ -434,6 +485,8 @@ static inline void cxl_mem_active_dec(void)
>   }
>   #endif
>   
> +void cxl_mbox_bgcmd_work(struct work_struct *work);
> +
>   struct cxl_hdm {
>   	struct cxl_component_regs regs;
>   	unsigned int decoder_count;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 621a0522b554..b21d8681beac 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -268,6 +268,10 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>   		return -ENXIO;
>   	}
>   
> +	atomic_set(&cxlds->mbox_bg, 0);
> +	if (!cxlds->mbox_irq)
> +		INIT_DELAYED_WORK(&cxlds->mbox_bgpoll, cxl_mbox_bgcmd_work);
> +
>   	dev_dbg(cxlds->dev, "Mailbox payload sized %zu",
>   		cxlds->payload_size);
>   
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index ab40c93c44e5..c6a6d3fd6883 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -173,7 +173,8 @@ static int cxl_pmem_get_config_data(struct cxl_dev_state *cxlds,
>   	};
>   
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LSA, &get_lsa,
> -			       sizeof(get_lsa), cmd->out_buf, cmd->in_length);
> +			       sizeof(get_lsa), cmd->out_buf,
> +			       cmd->in_length, NULL);
>   	cmd->status = 0;
>   
>   	return rc;
> @@ -205,7 +206,7 @@ static int cxl_pmem_set_config_data(struct cxl_dev_state *cxlds,
>   
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_LSA, set_lsa,
>   			       struct_size(set_lsa, data, cmd->in_length),
> -			       NULL, 0);
> +			       NULL, 0, NULL);
>   
>   	/*
>   	 * Set "firmware" status (4-packed bytes at the end of the input
> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> index 5484d4eecfd1..825d1bd1dd16 100644
> --- a/drivers/cxl/security.c
> +++ b/drivers/cxl/security.c
> @@ -20,7 +20,7 @@ static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
>   	int rc;
>   
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE, NULL, 0,
> -			       &sec_out, sizeof(sec_out));
> +			       &sec_out, sizeof(sec_out), NULL);
>   	if (rc < 0)
>   		return 0;
>   
> @@ -67,7 +67,7 @@ static int cxl_pmem_security_change_key(struct nvdimm *nvdimm,
>   	memcpy(set_pass.new_pass, new_data->data, NVDIMM_PASSPHRASE_LEN);
>   
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_PASSPHRASE,
> -			       &set_pass, sizeof(set_pass), NULL, 0);
> +			       &set_pass, sizeof(set_pass), NULL, 0, NULL);
>   	return rc;
>   }
>   
> @@ -86,7 +86,7 @@ static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
>   	memcpy(dis_pass.pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
>   
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_DISABLE_PASSPHRASE,
> -			       &dis_pass, sizeof(dis_pass), NULL, 0);
> +			       &dis_pass, sizeof(dis_pass), NULL, 0, NULL);
>   	return rc;
>   }
>   
> @@ -108,7 +108,8 @@ static int cxl_pmem_security_freeze(struct nvdimm *nvdimm)
>   	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>   	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>   
> -	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0);
> +	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY,
> +				 NULL, 0, NULL, 0, NULL);
>   }
>   
>   static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
> @@ -122,7 +123,7 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
>   
>   	memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_UNLOCK,
> -			       pass, NVDIMM_PASSPHRASE_LEN, NULL, 0);
> +			       pass, NVDIMM_PASSPHRASE_LEN, NULL, 0, NULL);
>   	if (rc < 0)
>   		return rc;
>   
> @@ -143,7 +144,7 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
>   		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
>   	memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN);
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
> -			       &erase, sizeof(erase), NULL, 0);
> +			       &erase, sizeof(erase), NULL, 0, NULL);
>   	if (rc < 0)
>   		return rc;
>   

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Davidlohr Bueso]

On Tue, 06 Dec 2022, Dave Jiang wrote:

>On 12/5/2022 6:14 PM, Davidlohr Bueso wrote:
>>+static struct workqueue_struct *cxl_mbox_bgpoll_wq;
>
>Do we want per device wq rather than a global one? Just thinking if
>you have a device go away and you need to flush outstanding commands,
>you might impact the other devices.

I'm not sure I completely follow here as the device is refcounted
so it doesn't go away while the op is in progress. But this actually
makes me think that the current usage is broken as nothing is actually
serializing access to queue on the cxl_mbox_bgpoll_wq. So two different
devices could be adding themselves concurrently.

And doing this per-device actually solves the above nicely, and no
queue is actually needed as self would always be the only one.

...

>>+/**
>>+ * struct cxl_mbox_bgcmd_ops - Optional ad-hoc handling of background cmds
>>+ * @post: Execute after the command completes
>>+ * @conflicts: How to handle a @new command when one is currently executing
>>+ *             (only for polling mode)
>>+ * @delay: Delay hint for polling on command completion
>>+ */
>>+struct cxl_mem_bgcommand_ops {
>>+	void (*post)(struct cxl_dev_state *cxlds, bool success);
>
>"post" here I think conveys different meaning than "after". Typically
>it is used in the context of issuing the command. To remove confusion,
>maybe call it "end_cmd" or "finish"?

Sure.

>
>
>>+	int (*conflicts)(u16 new);
>
>has_conflicts() may be better. Also, it can return a bool to indicate
>whether there are conflicts.

I'd rather have the callback return the error directly, which is why
it's an int.

>
>>+	unsigned long (*delay)(struct cxl_dev_state *cxlds);
>
>"poll_delay"?

Sure.

>
>>+};
>>+
>>  /**
>>   * struct cxl_mem_command - Driver representation of a memory device command
>>   * @info: Command information as it exists for the UAPI
>>   * @opcode: The actual bits used for the mailbox protocol
>>   * @flags: Set of flags effecting driver behavior.
>>+ * @bg_ops: Set of optional callbacks to handle commands that can run in
>>+ *          the background.
>>   *
>>   *  * %CXL_CMD_FLAG_FORCE_ENABLE: In cases of error, commands with this flag
>>   *    will be enabled by the driver regardless of what hardware may have
>>   *    advertised.
>>   *
>>+ *  * %CXL_CMD_FLAG_BACKGROUND: The command may execute asynchronously in the
>>+ *    background if the operation takes longer than ~2 seconds. The semantics
>>+ *    are:
>>+ *      - Only a single of these commands can run at a time.
>>+ *      - Depending on the nature of the operation, devices may continue to
>>+ *        accept new non-background commands.
>>+ *
>>   * The cxl_mem_command is the driver's internal representation of commands that
>>   * are supported by the driver. Some of these commands may not be supported by
>>   * the hardware. The driver will use @info to validate the fields passed in by
>>@@ -374,8 +422,10 @@ struct cxl_mem_command {
>>	struct cxl_command_info info;
>>	enum cxl_opcode opcode;
>>	u32 flags;
>>+	struct cxl_mem_bgcommand_ops __maybe_unused *bgops;
>>  #define CXL_CMD_FLAG_NONE 0
>>  #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
>>+#define CXL_CMD_FLAG_BACKGROUND BIT(1)
>>  };
>>  #define CXL_PMEM_SEC_STATE_USER_PASS_SET	0x01
>>@@ -414,7 +464,8 @@ enum {
>>  };
>>  int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>>-		      size_t in_size, void *out, size_t out_size);
>>+		      size_t in_size, void *out, size_t out_size,
>>+		      u16 *return_code);
>
>Given the existing commands don't care about return_code and only the
>new ones do, and to minimize code changes and having to chase down
>every call of the existing command, you can maybe do something like:
>
>int cxl_mbox_send_cmd_with_return(struct cxl_dev_state *cxlds,
>				  u16 opcode, void *in, size_t in_size,
>				  void *out, size_t out_size,
>				  u16 *return_code)
>{
>	.....
>}
>
>int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds,
>		      u16 opcode, void *in, size_t in_size,
>		      void *out, size_t out_size)
>{
>	return cxl_mbox_send_cmd_with_return(cxlds, opcode, in, in_size,
>					     out, out_size, NULL);
>}

I thought about this as Jonathan wasn't too fond of this either. But
considering Dan's recent rework of cxl_mbox_send_cmd() I have to redo
these and should be cleaner anyway.

Thanks,
Davidlohr

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Dave Jiang]

On 12/6/2022 8:42 PM, Davidlohr Bueso wrote:
> On Tue, 06 Dec 2022, Dave Jiang wrote:
> 
>> On 12/5/2022 6:14 PM, Davidlohr Bueso wrote:
>>> +static struct workqueue_struct *cxl_mbox_bgpoll_wq;
>>
>> Do we want per device wq rather than a global one? Just thinking if
>> you have a device go away and you need to flush outstanding commands,
>> you might impact the other devices.
> 
> I'm not sure I completely follow here as the device is refcounted
> so it doesn't go away while the op is in progress. But this actually

i.e. somebody unbind the driver to a device for whatever reason.

DJ

> makes me think that the current usage is broken as nothing is actually
> serializing access to queue on the cxl_mbox_bgpoll_wq. So two different
> devices could be adding themselves concurrently.
> 
> And doing this per-device actually solves the above nicely, and no
> queue is actually needed as self would always be the only one.
> 
> ...
> 
>>> +/**
>>> + * struct cxl_mbox_bgcmd_ops - Optional ad-hoc handling of 
>>> background cmds
>>> + * @post: Execute after the command completes
>>> + * @conflicts: How to handle a @new command when one is currently 
>>> executing
>>> + *             (only for polling mode)
>>> + * @delay: Delay hint for polling on command completion
>>> + */
>>> +struct cxl_mem_bgcommand_ops {
>>> +    void (*post)(struct cxl_dev_state *cxlds, bool success);
>>
>> "post" here I think conveys different meaning than "after". Typically
>> it is used in the context of issuing the command. To remove confusion,
>> maybe call it "end_cmd" or "finish"?
> 
> Sure.
> 
>>
>>
>>> +    int (*conflicts)(u16 new);
>>
>> has_conflicts() may be better. Also, it can return a bool to indicate
>> whether there are conflicts.
> 
> I'd rather have the callback return the error directly, which is why
> it's an int.
> 
>>
>>> +    unsigned long (*delay)(struct cxl_dev_state *cxlds);
>>
>> "poll_delay"?
> 
> Sure.
> 
>>
>>> +};
>>> +
>>>  /**
>>>   * struct cxl_mem_command - Driver representation of a memory device 
>>> command
>>>   * @info: Command information as it exists for the UAPI
>>>   * @opcode: The actual bits used for the mailbox protocol
>>>   * @flags: Set of flags effecting driver behavior.
>>> + * @bg_ops: Set of optional callbacks to handle commands that can 
>>> run in
>>> + *          the background.
>>>   *
>>>   *  * %CXL_CMD_FLAG_FORCE_ENABLE: In cases of error, commands with 
>>> this flag
>>>   *    will be enabled by the driver regardless of what hardware may 
>>> have
>>>   *    advertised.
>>>   *
>>> + *  * %CXL_CMD_FLAG_BACKGROUND: The command may execute 
>>> asynchronously in the
>>> + *    background if the operation takes longer than ~2 seconds. The 
>>> semantics
>>> + *    are:
>>> + *      - Only a single of these commands can run at a time.
>>> + *      - Depending on the nature of the operation, devices may 
>>> continue to
>>> + *        accept new non-background commands.
>>> + *
>>>   * The cxl_mem_command is the driver's internal representation of 
>>> commands that
>>>   * are supported by the driver. Some of these commands may not be 
>>> supported by
>>>   * the hardware. The driver will use @info to validate the fields 
>>> passed in by
>>> @@ -374,8 +422,10 @@ struct cxl_mem_command {
>>>     struct cxl_command_info info;
>>>     enum cxl_opcode opcode;
>>>     u32 flags;
>>> +    struct cxl_mem_bgcommand_ops __maybe_unused *bgops;
>>>  #define CXL_CMD_FLAG_NONE 0
>>>  #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
>>> +#define CXL_CMD_FLAG_BACKGROUND BIT(1)
>>>  };
>>>  #define CXL_PMEM_SEC_STATE_USER_PASS_SET    0x01
>>> @@ -414,7 +464,8 @@ enum {
>>>  };
>>>  int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void 
>>> *in,
>>> -              size_t in_size, void *out, size_t out_size);
>>> +              size_t in_size, void *out, size_t out_size,
>>> +              u16 *return_code);
>>
>> Given the existing commands don't care about return_code and only the
>> new ones do, and to minimize code changes and having to chase down
>> every call of the existing command, you can maybe do something like:
>>
>> int cxl_mbox_send_cmd_with_return(struct cxl_dev_state *cxlds,
>>                   u16 opcode, void *in, size_t in_size,
>>                   void *out, size_t out_size,
>>                   u16 *return_code)
>> {
>>     .....
>> }
>>
>> int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds,
>>               u16 opcode, void *in, size_t in_size,
>>               void *out, size_t out_size)
>> {
>>     return cxl_mbox_send_cmd_with_return(cxlds, opcode, in, in_size,
>>                          out, out_size, NULL);
>> }
> 
> I thought about this as Jonathan wasn't too fond of this either. But
> considering Dan's recent rework of cxl_mbox_send_cmd() I have to redo
> these and should be cleaner anyway.
> 
> Thanks,
> Davidlohr

RE: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Dan Williams]

Davidlohr Bueso wrote:
> This adds support for handling background operations, as defined in
> the CXL 3.0 spec. Commands that can take too long (over ~2 seconds)
> can run asynchronously in the background, these are limited to:
> transfer/activate firmware, scan media, sanitize (aka overwrite)
> and VPPB bind/unbind.
> 
> Completion of background operations, successful or not, can be handled
> via irq or poll based. This patch deals only with polling, which
> introduces some complexities as we need to handle the window in which
> the original background command completed, then a new one is
> successfully started before the poller wakes up and checks. This,
> in theory, can occur any amount of times. One of the problems is
> that previous states cannot be tracked as hw background status
> registers always deal with the last command.
> 
> So in order to keep hardware and the driver in sync, there can be
> windows where the hardware is ready but the driver won't be, and
> thus must serialize/handle it accordingly. While this polling cost
> may not be ideal: 1) background commands are rare/limited and
> 2) the extra busy time should be small compared to the overall
> command duration and 3) devices that support mailbox interrupts
> do not use this.
> 
> The implementation extends struct cxl_mem_command to become aware
> of background-capable commands in a generic fashion and presents
> some interfaces:
> 
> - Calls for bg operations, where each bg command can choose to implement
>   whatever needed based on the nature of the operation. For example,
>   while running, overwrite may only allow certain related commands to
>   occur, while scan media does not have any such limitations.
>   Delay times can also be different, for which ad-hoc hinting can
>   be defined - for example, scan media could use some value based on
>   GET_SCAN_MEDIA_CAPS and overwrite has predefined values based on
>   pmem DSM specs[0]. Similarly some commands need to execute tasks
>   once the command finishes, such as overwriting requires CPU flushing
>   when successfully done. These are:
> 
>   cxl_mbox_bgcmd_conflicts()
>   cxl_mbox_bgcmd_delay()
>   cxl_mbox_bgcmd_post()
> 
> - cxl_mbox_send_cmd() is extended such that callers can distinguish,
>   upon rc == 0, between completed and successfully started in the
>   background.
> 
> - cxl_mbox_bgcmd_running() will atomically tell which command is
>   running in the background, if any. This allows easy querying
>   functionality. Similarly, there are cxl_mbox_bgcmd_start() and
>   cxl_mbox_bgcmd_done() to safely mark the in-flight operation.
>   While x86 serializes atomics, care must be taken with arm64, for
>   example, ensuring, minimally, release/acquire semantics.
> 
> There are currently no supported commands.
> 
> [0] https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/core.h |   3 +-
>  drivers/cxl/core/mbox.c | 213 ++++++++++++++++++++++++++++++++++++++--
>  drivers/cxl/core/port.c |   1 +
>  drivers/cxl/cxl.h       |   8 ++
>  drivers/cxl/cxlmem.h    |  55 ++++++++++-
>  drivers/cxl/pci.c       |   4 +
>  drivers/cxl/pmem.c      |   5 +-
>  drivers/cxl/security.c  |  13 +--
>  8 files changed, 282 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 1d8f87be283f..2a71664a0668 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -69,6 +69,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>  
>  int cxl_memdev_init(void);
>  void cxl_memdev_exit(void);
> -void cxl_mbox_init(void);
> +int cxl_mbox_init(void);
> +void cxl_mbox_exit(void);
>  
>  #endif /* __CXL_CORE_H__ */
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 35dd889f1d3a..bfee9251c81c 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -10,6 +10,7 @@
>  #include "core.h"
>  
>  static bool cxl_raw_allow_all;
> +static struct workqueue_struct *cxl_mbox_bgpoll_wq;
>  
>  /**
>   * DOC: cxl mbox
> @@ -24,7 +25,7 @@ static bool cxl_raw_allow_all;
>  	for ((cmd) = &cxl_mem_commands[0];                                     \
>  	     ((cmd) - cxl_mem_commands) < ARRAY_SIZE(cxl_mem_commands); (cmd)++)
>  
> -#define CXL_CMD(_id, sin, sout, _flags)                                        \
> +#define __CXL_CMD(_id, sin, sout, _flags, _bgops)                              \
>  	[CXL_MEM_COMMAND_ID_##_id] = {                                         \
>  	.info =	{                                                              \
>  			.id = CXL_MEM_COMMAND_ID_##_id,                        \
> @@ -33,8 +34,13 @@ static bool cxl_raw_allow_all;
>  		},                                                             \
>  	.opcode = CXL_MBOX_OP_##_id,                                           \
>  	.flags = _flags,                                                       \
> +	.bgops = _bgops,						       \
>  	}
>  
> +#define CXL_CMD(_id, sin, sout, _flags) __CXL_CMD(_id, sin, sout, _flags, NULL)
> +#define CXL_BGCMD(_id, sin, sout, _flags, _bgops)                              \
> +	__CXL_CMD(_id, sin, sout, _flags | CXL_CMD_FLAG_BACKGROUND, _bgops)
> +
>  #define CXL_VARIABLE_PAYLOAD	~0U
>  /*
>   * This table defines the supported mailbox commands for the driver. This table
> @@ -63,7 +69,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(INJECT_POISON, 0x8, 0, 0),
>  	CXL_CMD(CLEAR_POISON, 0x48, 0, 0),
>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
> -	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
> +	CXL_BGCMD(SCAN_MEDIA, 0x11, 0, 0, NULL),

One of the recent revelations with this patch:

https://lore.kernel.org/linux-cxl/167030056464.4044561.11486507095384253833.stgit@dwillia2-xfh.jf.intel.com/

...is that commands that are not meant to be called from userspace
should simply never appear in CXL_CMDS nor cxl_mem_commands. The
motivation for defining SCAN_MEDIA and friends was to use their
definitions to block the RAW command passthrough, but ironically that
enables the ioctl path which is also unwanted.

So I think this patchset first needs to mark those uapi command numbers
deprecated. Then ensure that all other background commands can not be
submitted via the ioctl path. 

>  	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
>  	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
>  	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
> @@ -145,6 +151,136 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>  	return cxl_command_names[c->info.id].name;
>  }
>  
> +static unsigned long __maybe_unused
> +cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode)
> +{
> +	struct cxl_mem_command *c;
> +	unsigned long ret = 0;
> +
> +	c = cxl_mem_find_command(opcode);

Per that change above, cxl_mem_find_command() is only for the uapi.

> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
> +		dev_WARN_ONCE(cxlds->dev, true,
> +			      "Not a background command\n");
> +		return 0;
> +	}
> +
> +	if (c->bgops && c->bgops->delay)
> +		ret = c->bgops->delay(cxlds);
> +
> +	return ret * HZ;
> +}

In general I worry that this is a bit too flexible. Instead of callback
ops I think this wants to be straight line code with something like a
poll_interval argument in 'struct cxl_mbox_cmd' where 0 means the
command is immediate and non-zero is a number of jiffies between
background operation polling intervals.

> +
> +static void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds,
> +				u16 opcode, bool success)
> +{
> +	struct cxl_mem_command *c;
> +
> +	c = cxl_mem_find_command(opcode);
> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
> +		dev_WARN_ONCE(cxlds->dev, true,
> +			      "Not a background command\n");
> +		return;
> +	}
> +
> +	if (c->bgops && c->bgops->post)
> +		c->bgops->post(cxlds, success);
> +}
> +
> +/*
> + * Ensure that ->mbox_send(@new) can run safely when a background
> + * command is running. If so, returns zero, otherwise error.
> + *
> + * check 1. bg cmd running. If not running it is fine to
> + *	    send any command. If one is running then there
> + *	    may be restrictions.
> + * check 2. @new incoming command is capable of running
> + *          in the background. If there is an in-flight bg
> + *          operation, all these are forbidden as we need
> + *          to serialize when polling.
> + * check 3. @new incoming command is not blacklisted by the
> + *          current running command.
> + */
> +static int __maybe_unused
> +cxl_mbox_check_cmd_bgcmd(struct cxl_dev_state *cxlds, u16 new)
> +{
> +	struct cxl_mem_command *c;
> +
> +	/* 1 */
> +	if (likely(!cxl_mbox_bgcmd_running(cxlds)))
> +		return 0;
> +
> +	c = cxl_mem_find_command(new);
> +	if (!c)
> +		return -EINVAL;
> +
> +	/* 2 */
> +	if (c->flags & CXL_CMD_FLAG_BACKGROUND)
> +		return -EBUSY;
> +
> +	/* 3 */
> +	if (c->bgops && c->bgops->conflicts)
> +		return c->bgops->conflicts(new);
> +
> +	return 0;
> +}
> +
> +/*
> + * Background operation polling mode.
> + */
> +void cxl_mbox_bgcmd_work(struct work_struct *work)

Is a workqueue needed? If the goal is to make it so that all the kernel
background command use cases chop up their work so that one command can
not monopolize the mailbox for a long amount of time then the
implementation can also assume that foreground commands that collide
with an in-flight background operation can just wait synchronously for
their time slice.

That makes the handling simple, acquire lock, poll for completion,
and everyone else just queues up on the lock.

> +{
> +	struct cxl_dev_state *cxlds;
> +	u64 bgcmd_status_reg;
> +	u16 opcode;
> +	u32 pct;
> +
> +	cxlds = container_of(work, struct cxl_dev_state, mbox_bgpoll.work);
> +
> +	dev_WARN_ONCE(cxlds->dev, !!cxlds->mbox_irq,
> +		      "Polling mailbox when IRQs are supported\n");
> +
> +	bgcmd_status_reg = readq(cxlds->regs.mbox +
> +				 CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> +	opcode = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK,
> +			   bgcmd_status_reg);
> +
> +	pct = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK, bgcmd_status_reg);
> +	if (pct != 100) {
> +		unsigned long hint;
> +		u64 status_reg;
> +
> +		status_reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_STATUS_OFFSET);
> +		if (!FIELD_GET(CXLDEV_MBOX_STATUS_BACKGROUND_OPERATION,
> +			       status_reg))
> +			dev_WARN(cxlds->dev,
> +				 "No background operation running (%u%%).\n",
> +				 pct);
> +
> +		hint = cxl_mbox_bgcmd_delay(cxlds, opcode);
> +		if (hint == 0) {
> +			/*
> +			 * TODO: try to do better(?). pct is updated every
> +			 * ~1 sec, maybe use a heurstic based on that.
> +			 */
> +			hint = 5 * HZ;
> +		}
> +
> +		queue_delayed_work(cxl_mbox_bgpoll_wq,
> +				   &cxlds->mbox_bgpoll, hint);
> +	} else { /* bg operation completed */
> +		u16 return_code;
> +
> +		return_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
> +					bgcmd_status_reg);
> +		cxl_mbox_bgcmd_post(cxlds, opcode,
> +				    return_code == CXL_MBOX_CMD_RC_SUCCESS);
> +
> +		put_device(cxlds->dev);
> +		cxl_mbox_bgcmd_done(cxlds);
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mbox_bgcmd_work, CXL);
> +
>  /**
>   * cxl_mbox_send_cmd() - Send a mailbox command to a device.
>   * @cxlds: The device data for the operation
> @@ -153,6 +289,7 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>   * @in_size: The length of the input payload
>   * @out: Caller allocated buffer for the output.
>   * @out_size: Expected size of output.
> + * @return_code: (optional output) HW returned code.
>   *
>   * Context: Any context.
>   * Return:
> @@ -167,8 +304,9 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>   * error. While this distinction can be useful for commands from userspace, the
>   * kernel will only be able to use results when both are successful.
>   */
> -int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
> -		      size_t in_size, void *out, size_t out_size)
> +int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode,
> +		      void *in, size_t in_size,
> +		      void *out, size_t out_size, u16 *return_code)
>  {
>  	const struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>  	struct cxl_mbox_cmd mbox_cmd = {
> @@ -183,12 +321,53 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>  	if (in_size > cxlds->payload_size || out_size > cxlds->payload_size)
>  		return -E2BIG;
>  
> +	/*
> +	 * With bg polling this can overlap a scenario where the
> +	 * hardware can receive new requests but the driver is
> +	 * not ready. Handle accordingly.
> +	 */
> +	if (!cxlds->mbox_irq) {
> +		rc = cxl_mbox_check_cmd_bgcmd(cxlds, opcode);
> +		if (rc)
> +			return rc;
> +	}

Can't happen with the straight-line approach as all an interrupt will do
is wake up a wait_event_timeout() instance for the currently executing
background command.

> +
>  	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
> +	if (return_code)
> +		*return_code = mbox_cmd.return_code;
>  	if (rc)
>  		return rc;
>  
> -	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
> +	switch (mbox_cmd.return_code) {
> +	case CXL_MBOX_CMD_RC_BACKGROUND:
> +	{
> +		int err;
> +
> +		dev_dbg(cxlds->dev, "Opcode 0x%04x: %s\n", opcode,
> +			cxl_mbox_cmd_rc2str(&mbox_cmd));
> +
> +		err = cxl_mbox_bgcmd_begin(cxlds, opcode);
> +		if (err) {
> +			dev_WARN(cxlds->dev,
> +				 "Corrupted background cmd (opcode 0x%04x)\n",
> +				 err);
> +			return -ENXIO;
> +		}
> +
> +		get_device(cxlds->dev);
> +
> +		if (!cxlds->mbox_irq) {
> +			/* do first poll check asap before using any hinting */
> +			queue_delayed_work(cxl_mbox_bgpoll_wq,
> +					   &cxlds->mbox_bgpoll, 0);
> +		}
> +		break;
> +	}
> +	case CXL_MBOX_CMD_RC_SUCCESS:
> +		break;
> +	default:
>  		return cxl_mbox_cmd_rc2errno(&mbox_cmd);
> +	}
>  
>  	/*
>  	 * Variable sized commands can't be validated and so it's up to the
> @@ -575,7 +754,7 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
>  		int rc;
>  
>  		rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LOG, &log, sizeof(log),
> -				       out, xfer_size);
> +				       out, xfer_size, NULL);

In the above referenced patch I fixed up cxl_internal_send_cmd() to
derive all its parameters from a passed in 'struct cxl_mbox_cmd', that
should make it easier to add new parameters.

I'll stop here until we can come to consensus on how general this
solution needs to be. I think this really wants to have a single
background command use case in mind to answer some of the use case case
questions.

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Dave Jiang]

On 12/6/2022 6:55 PM, Dan Williams wrote:
> Davidlohr Bueso wrote:
>> This adds support for handling background operations, as defined in
>> the CXL 3.0 spec. Commands that can take too long (over ~2 seconds)
>> can run asynchronously in the background, these are limited to:
>> transfer/activate firmware, scan media, sanitize (aka overwrite)
>> and VPPB bind/unbind.
>>
>> Completion of background operations, successful or not, can be handled
>> via irq or poll based. This patch deals only with polling, which
>> introduces some complexities as we need to handle the window in which
>> the original background command completed, then a new one is
>> successfully started before the poller wakes up and checks. This,
>> in theory, can occur any amount of times. One of the problems is
>> that previous states cannot be tracked as hw background status
>> registers always deal with the last command.
>>
>> So in order to keep hardware and the driver in sync, there can be
>> windows where the hardware is ready but the driver won't be, and
>> thus must serialize/handle it accordingly. While this polling cost
>> may not be ideal: 1) background commands are rare/limited and
>> 2) the extra busy time should be small compared to the overall
>> command duration and 3) devices that support mailbox interrupts
>> do not use this.
>>
>> The implementation extends struct cxl_mem_command to become aware
>> of background-capable commands in a generic fashion and presents
>> some interfaces:
>>
>> - Calls for bg operations, where each bg command can choose to implement
>>    whatever needed based on the nature of the operation. For example,
>>    while running, overwrite may only allow certain related commands to
>>    occur, while scan media does not have any such limitations.
>>    Delay times can also be different, for which ad-hoc hinting can
>>    be defined - for example, scan media could use some value based on
>>    GET_SCAN_MEDIA_CAPS and overwrite has predefined values based on
>>    pmem DSM specs[0]. Similarly some commands need to execute tasks
>>    once the command finishes, such as overwriting requires CPU flushing
>>    when successfully done. These are:
>>
>>    cxl_mbox_bgcmd_conflicts()
>>    cxl_mbox_bgcmd_delay()
>>    cxl_mbox_bgcmd_post()
>>
>> - cxl_mbox_send_cmd() is extended such that callers can distinguish,
>>    upon rc == 0, between completed and successfully started in the
>>    background.
>>
>> - cxl_mbox_bgcmd_running() will atomically tell which command is
>>    running in the background, if any. This allows easy querying
>>    functionality. Similarly, there are cxl_mbox_bgcmd_start() and
>>    cxl_mbox_bgcmd_done() to safely mark the in-flight operation.
>>    While x86 serializes atomics, care must be taken with arm64, for
>>    example, ensuring, minimally, release/acquire semantics.
>>
>> There are currently no supported commands.
>>
>> [0] https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>> ---
>>   drivers/cxl/core/core.h |   3 +-
>>   drivers/cxl/core/mbox.c | 213 ++++++++++++++++++++++++++++++++++++++--
>>   drivers/cxl/core/port.c |   1 +
>>   drivers/cxl/cxl.h       |   8 ++
>>   drivers/cxl/cxlmem.h    |  55 ++++++++++-
>>   drivers/cxl/pci.c       |   4 +
>>   drivers/cxl/pmem.c      |   5 +-
>>   drivers/cxl/security.c  |  13 +--
>>   8 files changed, 282 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index 1d8f87be283f..2a71664a0668 100644
>> --- a/drivers/cxl/core/core.h
>> +++ b/drivers/cxl/core/core.h
>> @@ -69,6 +69,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>>   
>>   int cxl_memdev_init(void);
>>   void cxl_memdev_exit(void);
>> -void cxl_mbox_init(void);
>> +int cxl_mbox_init(void);
>> +void cxl_mbox_exit(void);
>>   
>>   #endif /* __CXL_CORE_H__ */
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index 35dd889f1d3a..bfee9251c81c 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -10,6 +10,7 @@
>>   #include "core.h"
>>   
>>   static bool cxl_raw_allow_all;
>> +static struct workqueue_struct *cxl_mbox_bgpoll_wq;
>>   
>>   /**
>>    * DOC: cxl mbox
>> @@ -24,7 +25,7 @@ static bool cxl_raw_allow_all;
>>   	for ((cmd) = &cxl_mem_commands[0];                                     \
>>   	     ((cmd) - cxl_mem_commands) < ARRAY_SIZE(cxl_mem_commands); (cmd)++)
>>   
>> -#define CXL_CMD(_id, sin, sout, _flags)                                        \
>> +#define __CXL_CMD(_id, sin, sout, _flags, _bgops)                              \
>>   	[CXL_MEM_COMMAND_ID_##_id] = {                                         \
>>   	.info =	{                                                              \
>>   			.id = CXL_MEM_COMMAND_ID_##_id,                        \
>> @@ -33,8 +34,13 @@ static bool cxl_raw_allow_all;
>>   		},                                                             \
>>   	.opcode = CXL_MBOX_OP_##_id,                                           \
>>   	.flags = _flags,                                                       \
>> +	.bgops = _bgops,						       \
>>   	}
>>   
>> +#define CXL_CMD(_id, sin, sout, _flags) __CXL_CMD(_id, sin, sout, _flags, NULL)
>> +#define CXL_BGCMD(_id, sin, sout, _flags, _bgops)                              \
>> +	__CXL_CMD(_id, sin, sout, _flags | CXL_CMD_FLAG_BACKGROUND, _bgops)
>> +
>>   #define CXL_VARIABLE_PAYLOAD	~0U
>>   /*
>>    * This table defines the supported mailbox commands for the driver. This table
>> @@ -63,7 +69,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>>   	CXL_CMD(INJECT_POISON, 0x8, 0, 0),
>>   	CXL_CMD(CLEAR_POISON, 0x48, 0, 0),
>>   	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>> -	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
>> +	CXL_BGCMD(SCAN_MEDIA, 0x11, 0, 0, NULL),
> 
> One of the recent revelations with this patch:
> 
> https://lore.kernel.org/linux-cxl/167030056464.4044561.11486507095384253833.stgit@dwillia2-xfh.jf.intel.com/
> 
> ...is that commands that are not meant to be called from userspace
> should simply never appear in CXL_CMDS nor cxl_mem_commands. The
> motivation for defining SCAN_MEDIA and friends was to use their
> definitions to block the RAW command passthrough, but ironically that
> enables the ioctl path which is also unwanted.
> 
> So I think this patchset first needs to mark those uapi command numbers
> deprecated. Then ensure that all other background commands can not be
> submitted via the ioctl path.
> 
>>   	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
>>   	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
>>   	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
>> @@ -145,6 +151,136 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>>   	return cxl_command_names[c->info.id].name;
>>   }
>>   
>> +static unsigned long __maybe_unused
>> +cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode)
>> +{
>> +	struct cxl_mem_command *c;
>> +	unsigned long ret = 0;
>> +
>> +	c = cxl_mem_find_command(opcode);
> 
> Per that change above, cxl_mem_find_command() is only for the uapi.
> 
>> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
>> +		dev_WARN_ONCE(cxlds->dev, true,
>> +			      "Not a background command\n");
>> +		return 0;
>> +	}
>> +
>> +	if (c->bgops && c->bgops->delay)
>> +		ret = c->bgops->delay(cxlds);
>> +
>> +	return ret * HZ;
>> +}
> 
> In general I worry that this is a bit too flexible. Instead of callback
> ops I think this wants to be straight line code with something like a
> poll_interval argument in 'struct cxl_mbox_cmd' where 0 means the
> command is immediate and non-zero is a number of jiffies between
> background operation polling intervals.
> 
>> +
>> +static void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds,
>> +				u16 opcode, bool success)
>> +{
>> +	struct cxl_mem_command *c;
>> +
>> +	c = cxl_mem_find_command(opcode);
>> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
>> +		dev_WARN_ONCE(cxlds->dev, true,
>> +			      "Not a background command\n");
>> +		return;
>> +	}
>> +
>> +	if (c->bgops && c->bgops->post)
>> +		c->bgops->post(cxlds, success);
>> +}
>> +
>> +/*
>> + * Ensure that ->mbox_send(@new) can run safely when a background
>> + * command is running. If so, returns zero, otherwise error.
>> + *
>> + * check 1. bg cmd running. If not running it is fine to
>> + *	    send any command. If one is running then there
>> + *	    may be restrictions.
>> + * check 2. @new incoming command is capable of running
>> + *          in the background. If there is an in-flight bg
>> + *          operation, all these are forbidden as we need
>> + *          to serialize when polling.
>> + * check 3. @new incoming command is not blacklisted by the
>> + *          current running command.
>> + */
>> +static int __maybe_unused
>> +cxl_mbox_check_cmd_bgcmd(struct cxl_dev_state *cxlds, u16 new)
>> +{
>> +	struct cxl_mem_command *c;
>> +
>> +	/* 1 */
>> +	if (likely(!cxl_mbox_bgcmd_running(cxlds)))
>> +		return 0;
>> +
>> +	c = cxl_mem_find_command(new);
>> +	if (!c)
>> +		return -EINVAL;
>> +
>> +	/* 2 */
>> +	if (c->flags & CXL_CMD_FLAG_BACKGROUND)
>> +		return -EBUSY;
>> +
>> +	/* 3 */
>> +	if (c->bgops && c->bgops->conflicts)
>> +		return c->bgops->conflicts(new);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Background operation polling mode.
>> + */
>> +void cxl_mbox_bgcmd_work(struct work_struct *work)
> 
> Is a workqueue needed? If the goal is to make it so that all the kernel
> background command use cases chop up their work so that one command can
> not monopolize the mailbox for a long amount of time then the
> implementation can also assume that foreground commands that collide
> with an in-flight background operation can just wait synchronously for
> their time slice.
> 
> That makes the handling simple, acquire lock, poll for completion,
> and everyone else just queues up on the lock.

Aren't certain commands valid while a background command is on going? 
Just thinking if you are doing a sanitize (overwrite), for a pmem it may 
take a LONG time. And maybe you don't want to block everything for the 
duration of that command completion? Maybe I'm not understanding what 
you are saying here.

DJ

> 
>> +{
>> +	struct cxl_dev_state *cxlds;
>> +	u64 bgcmd_status_reg;
>> +	u16 opcode;
>> +	u32 pct;
>> +
>> +	cxlds = container_of(work, struct cxl_dev_state, mbox_bgpoll.work);
>> +
>> +	dev_WARN_ONCE(cxlds->dev, !!cxlds->mbox_irq,
>> +		      "Polling mailbox when IRQs are supported\n");
>> +
>> +	bgcmd_status_reg = readq(cxlds->regs.mbox +
>> +				 CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
>> +	opcode = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK,
>> +			   bgcmd_status_reg);
>> +
>> +	pct = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK, bgcmd_status_reg);
>> +	if (pct != 100) {
>> +		unsigned long hint;
>> +		u64 status_reg;
>> +
>> +		status_reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_STATUS_OFFSET);
>> +		if (!FIELD_GET(CXLDEV_MBOX_STATUS_BACKGROUND_OPERATION,
>> +			       status_reg))
>> +			dev_WARN(cxlds->dev,
>> +				 "No background operation running (%u%%).\n",
>> +				 pct);
>> +
>> +		hint = cxl_mbox_bgcmd_delay(cxlds, opcode);
>> +		if (hint == 0) {
>> +			/*
>> +			 * TODO: try to do better(?). pct is updated every
>> +			 * ~1 sec, maybe use a heurstic based on that.
>> +			 */
>> +			hint = 5 * HZ;
>> +		}
>> +
>> +		queue_delayed_work(cxl_mbox_bgpoll_wq,
>> +				   &cxlds->mbox_bgpoll, hint);
>> +	} else { /* bg operation completed */
>> +		u16 return_code;
>> +
>> +		return_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
>> +					bgcmd_status_reg);
>> +		cxl_mbox_bgcmd_post(cxlds, opcode,
>> +				    return_code == CXL_MBOX_CMD_RC_SUCCESS);
>> +
>> +		put_device(cxlds->dev);
>> +		cxl_mbox_bgcmd_done(cxlds);
>> +	}
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_mbox_bgcmd_work, CXL);
>> +
>>   /**
>>    * cxl_mbox_send_cmd() - Send a mailbox command to a device.
>>    * @cxlds: The device data for the operation
>> @@ -153,6 +289,7 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>>    * @in_size: The length of the input payload
>>    * @out: Caller allocated buffer for the output.
>>    * @out_size: Expected size of output.
>> + * @return_code: (optional output) HW returned code.
>>    *
>>    * Context: Any context.
>>    * Return:
>> @@ -167,8 +304,9 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>>    * error. While this distinction can be useful for commands from userspace, the
>>    * kernel will only be able to use results when both are successful.
>>    */
>> -int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>> -		      size_t in_size, void *out, size_t out_size)
>> +int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode,
>> +		      void *in, size_t in_size,
>> +		      void *out, size_t out_size, u16 *return_code)
>>   {
>>   	const struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>>   	struct cxl_mbox_cmd mbox_cmd = {
>> @@ -183,12 +321,53 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>>   	if (in_size > cxlds->payload_size || out_size > cxlds->payload_size)
>>   		return -E2BIG;
>>   
>> +	/*
>> +	 * With bg polling this can overlap a scenario where the
>> +	 * hardware can receive new requests but the driver is
>> +	 * not ready. Handle accordingly.
>> +	 */
>> +	if (!cxlds->mbox_irq) {
>> +		rc = cxl_mbox_check_cmd_bgcmd(cxlds, opcode);
>> +		if (rc)
>> +			return rc;
>> +	}
> 
> Can't happen with the straight-line approach as all an interrupt will do
> is wake up a wait_event_timeout() instance for the currently executing
> background command.
> 
>> +
>>   	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
>> +	if (return_code)
>> +		*return_code = mbox_cmd.return_code;
>>   	if (rc)
>>   		return rc;
>>   
>> -	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
>> +	switch (mbox_cmd.return_code) {
>> +	case CXL_MBOX_CMD_RC_BACKGROUND:
>> +	{
>> +		int err;
>> +
>> +		dev_dbg(cxlds->dev, "Opcode 0x%04x: %s\n", opcode,
>> +			cxl_mbox_cmd_rc2str(&mbox_cmd));
>> +
>> +		err = cxl_mbox_bgcmd_begin(cxlds, opcode);
>> +		if (err) {
>> +			dev_WARN(cxlds->dev,
>> +				 "Corrupted background cmd (opcode 0x%04x)\n",
>> +				 err);
>> +			return -ENXIO;
>> +		}
>> +
>> +		get_device(cxlds->dev);
>> +
>> +		if (!cxlds->mbox_irq) {
>> +			/* do first poll check asap before using any hinting */
>> +			queue_delayed_work(cxl_mbox_bgpoll_wq,
>> +					   &cxlds->mbox_bgpoll, 0);
>> +		}
>> +		break;
>> +	}
>> +	case CXL_MBOX_CMD_RC_SUCCESS:
>> +		break;
>> +	default:
>>   		return cxl_mbox_cmd_rc2errno(&mbox_cmd);
>> +	}
>>   
>>   	/*
>>   	 * Variable sized commands can't be validated and so it's up to the
>> @@ -575,7 +754,7 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
>>   		int rc;
>>   
>>   		rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LOG, &log, sizeof(log),
>> -				       out, xfer_size);
>> +				       out, xfer_size, NULL);
> 
> In the above referenced patch I fixed up cxl_internal_send_cmd() to
> derive all its parameters from a passed in 'struct cxl_mbox_cmd', that
> should make it easier to add new parameters.
> 
> I'll stop here until we can come to consensus on how general this
> solution needs to be. I think this really wants to have a single
> background command use case in mind to answer some of the use case case
> questions.

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Dan Williams]

Dave Jiang wrote:
> 
> 
> On 12/6/2022 6:55 PM, Dan Williams wrote:
> > Davidlohr Bueso wrote:
> >> This adds support for handling background operations, as defined in
> >> the CXL 3.0 spec. Commands that can take too long (over ~2 seconds)
> >> can run asynchronously in the background, these are limited to:
> >> transfer/activate firmware, scan media, sanitize (aka overwrite)
> >> and VPPB bind/unbind.
> >>
> >> Completion of background operations, successful or not, can be handled
> >> via irq or poll based. This patch deals only with polling, which
> >> introduces some complexities as we need to handle the window in which
> >> the original background command completed, then a new one is
> >> successfully started before the poller wakes up and checks. This,
> >> in theory, can occur any amount of times. One of the problems is
> >> that previous states cannot be tracked as hw background status
> >> registers always deal with the last command.
> >>
> >> So in order to keep hardware and the driver in sync, there can be
> >> windows where the hardware is ready but the driver won't be, and
> >> thus must serialize/handle it accordingly. While this polling cost
> >> may not be ideal: 1) background commands are rare/limited and
> >> 2) the extra busy time should be small compared to the overall
> >> command duration and 3) devices that support mailbox interrupts
> >> do not use this.
> >>
> >> The implementation extends struct cxl_mem_command to become aware
> >> of background-capable commands in a generic fashion and presents
> >> some interfaces:
> >>
> >> - Calls for bg operations, where each bg command can choose to implement
> >>    whatever needed based on the nature of the operation. For example,
> >>    while running, overwrite may only allow certain related commands to
> >>    occur, while scan media does not have any such limitations.
> >>    Delay times can also be different, for which ad-hoc hinting can
> >>    be defined - for example, scan media could use some value based on
> >>    GET_SCAN_MEDIA_CAPS and overwrite has predefined values based on
> >>    pmem DSM specs[0]. Similarly some commands need to execute tasks
> >>    once the command finishes, such as overwriting requires CPU flushing
> >>    when successfully done. These are:
> >>
> >>    cxl_mbox_bgcmd_conflicts()
> >>    cxl_mbox_bgcmd_delay()
> >>    cxl_mbox_bgcmd_post()
> >>
> >> - cxl_mbox_send_cmd() is extended such that callers can distinguish,
> >>    upon rc == 0, between completed and successfully started in the
> >>    background.
> >>
> >> - cxl_mbox_bgcmd_running() will atomically tell which command is
> >>    running in the background, if any. This allows easy querying
> >>    functionality. Similarly, there are cxl_mbox_bgcmd_start() and
> >>    cxl_mbox_bgcmd_done() to safely mark the in-flight operation.
> >>    While x86 serializes atomics, care must be taken with arm64, for
> >>    example, ensuring, minimally, release/acquire semantics.
> >>
> >> There are currently no supported commands.
> >>
> >> [0] https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> >>
> >> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> >> ---
> >>   drivers/cxl/core/core.h |   3 +-
> >>   drivers/cxl/core/mbox.c | 213 ++++++++++++++++++++++++++++++++++++++--
> >>   drivers/cxl/core/port.c |   1 +
> >>   drivers/cxl/cxl.h       |   8 ++
> >>   drivers/cxl/cxlmem.h    |  55 ++++++++++-
> >>   drivers/cxl/pci.c       |   4 +
> >>   drivers/cxl/pmem.c      |   5 +-
> >>   drivers/cxl/security.c  |  13 +--
> >>   8 files changed, 282 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> >> index 1d8f87be283f..2a71664a0668 100644
> >> --- a/drivers/cxl/core/core.h
> >> +++ b/drivers/cxl/core/core.h
> >> @@ -69,6 +69,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
> >>   
> >>   int cxl_memdev_init(void);
> >>   void cxl_memdev_exit(void);
> >> -void cxl_mbox_init(void);
> >> +int cxl_mbox_init(void);
> >> +void cxl_mbox_exit(void);
> >>   
> >>   #endif /* __CXL_CORE_H__ */
> >> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> >> index 35dd889f1d3a..bfee9251c81c 100644
> >> --- a/drivers/cxl/core/mbox.c
> >> +++ b/drivers/cxl/core/mbox.c
> >> @@ -10,6 +10,7 @@
> >>   #include "core.h"
> >>   
> >>   static bool cxl_raw_allow_all;
> >> +static struct workqueue_struct *cxl_mbox_bgpoll_wq;
> >>   
> >>   /**
> >>    * DOC: cxl mbox
> >> @@ -24,7 +25,7 @@ static bool cxl_raw_allow_all;
> >>   	for ((cmd) = &cxl_mem_commands[0];                                     \
> >>   	     ((cmd) - cxl_mem_commands) < ARRAY_SIZE(cxl_mem_commands); (cmd)++)
> >>   
> >> -#define CXL_CMD(_id, sin, sout, _flags)                                        \
> >> +#define __CXL_CMD(_id, sin, sout, _flags, _bgops)                              \
> >>   	[CXL_MEM_COMMAND_ID_##_id] = {                                         \
> >>   	.info =	{                                                              \
> >>   			.id = CXL_MEM_COMMAND_ID_##_id,                        \
> >> @@ -33,8 +34,13 @@ static bool cxl_raw_allow_all;
> >>   		},                                                             \
> >>   	.opcode = CXL_MBOX_OP_##_id,                                           \
> >>   	.flags = _flags,                                                       \
> >> +	.bgops = _bgops,						       \
> >>   	}
> >>   
> >> +#define CXL_CMD(_id, sin, sout, _flags) __CXL_CMD(_id, sin, sout, _flags, NULL)
> >> +#define CXL_BGCMD(_id, sin, sout, _flags, _bgops)                              \
> >> +	__CXL_CMD(_id, sin, sout, _flags | CXL_CMD_FLAG_BACKGROUND, _bgops)
> >> +
> >>   #define CXL_VARIABLE_PAYLOAD	~0U
> >>   /*
> >>    * This table defines the supported mailbox commands for the driver. This table
> >> @@ -63,7 +69,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
> >>   	CXL_CMD(INJECT_POISON, 0x8, 0, 0),
> >>   	CXL_CMD(CLEAR_POISON, 0x48, 0, 0),
> >>   	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
> >> -	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
> >> +	CXL_BGCMD(SCAN_MEDIA, 0x11, 0, 0, NULL),
> > 
> > One of the recent revelations with this patch:
> > 
> > https://lore.kernel.org/linux-cxl/167030056464.4044561.11486507095384253833.stgit@dwillia2-xfh.jf.intel.com/
> > 
> > ...is that commands that are not meant to be called from userspace
> > should simply never appear in CXL_CMDS nor cxl_mem_commands. The
> > motivation for defining SCAN_MEDIA and friends was to use their
> > definitions to block the RAW command passthrough, but ironically that
> > enables the ioctl path which is also unwanted.
> > 
> > So I think this patchset first needs to mark those uapi command numbers
> > deprecated. Then ensure that all other background commands can not be
> > submitted via the ioctl path.
> > 
> >>   	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
> >>   	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
> >>   	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
> >> @@ -145,6 +151,136 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
> >>   	return cxl_command_names[c->info.id].name;
> >>   }
> >>   
> >> +static unsigned long __maybe_unused
> >> +cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode)
> >> +{
> >> +	struct cxl_mem_command *c;
> >> +	unsigned long ret = 0;
> >> +
> >> +	c = cxl_mem_find_command(opcode);
> > 
> > Per that change above, cxl_mem_find_command() is only for the uapi.
> > 
> >> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
> >> +		dev_WARN_ONCE(cxlds->dev, true,
> >> +			      "Not a background command\n");
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (c->bgops && c->bgops->delay)
> >> +		ret = c->bgops->delay(cxlds);
> >> +
> >> +	return ret * HZ;
> >> +}
> > 
> > In general I worry that this is a bit too flexible. Instead of callback
> > ops I think this wants to be straight line code with something like a
> > poll_interval argument in 'struct cxl_mbox_cmd' where 0 means the
> > command is immediate and non-zero is a number of jiffies between
> > background operation polling intervals.
> > 
> >> +
> >> +static void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds,
> >> +				u16 opcode, bool success)
> >> +{
> >> +	struct cxl_mem_command *c;
> >> +
> >> +	c = cxl_mem_find_command(opcode);
> >> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
> >> +		dev_WARN_ONCE(cxlds->dev, true,
> >> +			      "Not a background command\n");
> >> +		return;
> >> +	}
> >> +
> >> +	if (c->bgops && c->bgops->post)
> >> +		c->bgops->post(cxlds, success);
> >> +}
> >> +
> >> +/*
> >> + * Ensure that ->mbox_send(@new) can run safely when a background
> >> + * command is running. If so, returns zero, otherwise error.
> >> + *
> >> + * check 1. bg cmd running. If not running it is fine to
> >> + *	    send any command. If one is running then there
> >> + *	    may be restrictions.
> >> + * check 2. @new incoming command is capable of running
> >> + *          in the background. If there is an in-flight bg
> >> + *          operation, all these are forbidden as we need
> >> + *          to serialize when polling.
> >> + * check 3. @new incoming command is not blacklisted by the
> >> + *          current running command.
> >> + */
> >> +static int __maybe_unused
> >> +cxl_mbox_check_cmd_bgcmd(struct cxl_dev_state *cxlds, u16 new)
> >> +{
> >> +	struct cxl_mem_command *c;
> >> +
> >> +	/* 1 */
> >> +	if (likely(!cxl_mbox_bgcmd_running(cxlds)))
> >> +		return 0;
> >> +
> >> +	c = cxl_mem_find_command(new);
> >> +	if (!c)
> >> +		return -EINVAL;
> >> +
> >> +	/* 2 */
> >> +	if (c->flags & CXL_CMD_FLAG_BACKGROUND)
> >> +		return -EBUSY;
> >> +
> >> +	/* 3 */
> >> +	if (c->bgops && c->bgops->conflicts)
> >> +		return c->bgops->conflicts(new);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/*
> >> + * Background operation polling mode.
> >> + */
> >> +void cxl_mbox_bgcmd_work(struct work_struct *work)
> > 
> > Is a workqueue needed? If the goal is to make it so that all the kernel
> > background command use cases chop up their work so that one command can
> > not monopolize the mailbox for a long amount of time then the
> > implementation can also assume that foreground commands that collide
> > with an in-flight background operation can just wait synchronously for
> > their time slice.
> > 
> > That makes the handling simple, acquire lock, poll for completion,
> > and everyone else just queues up on the lock.
> 
> Aren't certain commands valid while a background command is on going? 
> Just thinking if you are doing a sanitize (overwrite), for a pmem it may 
> take a LONG time. And maybe you don't want to block everything for the 
> duration of that command completion? Maybe I'm not understanding what 
> you are saying here.
> 

I am saying the possibility that a device implementation *might* allow
foreground commands to proceed while background commands are operating
is close to useless the way the specification defines it. The fact that
the implementation needs to be prepared for any foreground command to
return "Busy" due to background operation conflict and the fact that
background operations do not stack leads me to say, no, let's not play
those games and just handle everything in-order with no overlap.

While it is true that overwrite can occupy ->mbox_send() for a long
time, it also specifies that commands that depend on the media being in
a 'not disabled' state will also fail. So again, for simplicity, if you
start an overwrite one of the consequences is all other command requests
block or fail until that completes. Likely 'fail' is better than 'block'
to encourage user space to be mindful that it cannot overlap overwrite
and any other device usage.

Otherwise, the complexity needs to have an exceedingly good reason, not
just "because the spec might allow it".

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Davidlohr Bueso]

On Tue, 06 Dec 2022, Dan Williams wrote:

>One of the recent revelations with this patch:
>
>https://lore.kernel.org/linux-cxl/167030056464.4044561.11486507095384253833.stgit@dwillia2-xfh.jf.intel.com/
>
>...is that commands that are not meant to be called from userspace
>should simply never appear in CXL_CMDS nor cxl_mem_commands. The
>motivation for defining SCAN_MEDIA and friends was to use their
>definitions to block the RAW command passthrough, but ironically that
>enables the ioctl path which is also unwanted.
>
>So I think this patchset first needs to mark those uapi command numbers
>deprecated. Then ensure that all other background commands can not be
>submitted via the ioctl path.

Ok I will have a look.

>
>>	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
>>	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
>>	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
>> @@ -145,6 +151,136 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>>	return cxl_command_names[c->info.id].name;
>>  }
>>
>> +static unsigned long __maybe_unused
>> +cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode)
>> +{
>> +	struct cxl_mem_command *c;
>> +	unsigned long ret = 0;
>> +
>> +	c = cxl_mem_find_command(opcode);
>
>Per that change above, cxl_mem_find_command() is only for the uapi.
>
>> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
>> +		dev_WARN_ONCE(cxlds->dev, true,
>> +			      "Not a background command\n");
>> +		return 0;
>> +	}
>> +
>> +	if (c->bgops && c->bgops->delay)
>> +		ret = c->bgops->delay(cxlds);
>> +
>> +	return ret * HZ;
>> +}
>
>In general I worry that this is a bit too flexible. Instead of callback
>ops I think this wants to be straight line code with something like a
>poll_interval argument in 'struct cxl_mbox_cmd' where 0 means the
>command is immediate and non-zero is a number of jiffies between
>background operation polling intervals.

Are you refering _only_ to the delay callback being too flexible or the
the whole set of callbacks? If only delay, I don't like the idea of
having a poll_interval argument *and* a bgops structure with post and
conflicts callbacks. We could also move conflicts functionality to
another argument in cxl_mbox_cmd structure, but not delay functionality,
which makes consolidating everything in a callback structure nicer imo.

>
>> +
>> +static void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds,
>> +				u16 opcode, bool success)
>> +{
>> +	struct cxl_mem_command *c;
>> +
>> +	c = cxl_mem_find_command(opcode);
>> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
>> +		dev_WARN_ONCE(cxlds->dev, true,
>> +			      "Not a background command\n");
>> +		return;
>> +	}
>> +
>> +	if (c->bgops && c->bgops->post)
>> +		c->bgops->post(cxlds, success);
>> +}
>> +
>> +/*
>> + * Ensure that ->mbox_send(@new) can run safely when a background
>> + * command is running. If so, returns zero, otherwise error.
>> + *
>> + * check 1. bg cmd running. If not running it is fine to
>> + *	    send any command. If one is running then there
>> + *	    may be restrictions.
>> + * check 2. @new incoming command is capable of running
>> + *          in the background. If there is an in-flight bg
>> + *          operation, all these are forbidden as we need
>> + *          to serialize when polling.
>> + * check 3. @new incoming command is not blacklisted by the
>> + *          current running command.
>> + */
>> +static int __maybe_unused
>> +cxl_mbox_check_cmd_bgcmd(struct cxl_dev_state *cxlds, u16 new)
>> +{
>> +	struct cxl_mem_command *c;
>> +
>> +	/* 1 */
>> +	if (likely(!cxl_mbox_bgcmd_running(cxlds)))
>> +		return 0;
>> +
>> +	c = cxl_mem_find_command(new);
>> +	if (!c)
>> +		return -EINVAL;
>> +
>> +	/* 2 */
>> +	if (c->flags & CXL_CMD_FLAG_BACKGROUND)
>> +		return -EBUSY;
>> +
>> +	/* 3 */
>> +	if (c->bgops && c->bgops->conflicts)
>> +		return c->bgops->conflicts(new);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Background operation polling mode.
>> + */
>> +void cxl_mbox_bgcmd_work(struct work_struct *work)
>
>Is a workqueue needed? If the goal is to make it so that all the kernel
>background command use cases chop up their work so that one command can
>not monopolize the mailbox for a long amount of time then the
>implementation can also assume that foreground commands that collide
>with an in-flight background operation can just wait synchronously for
>their time slice.
>
>That makes the handling simple, acquire lock, poll for completion,
>and everyone else just queues up on the lock.

I don't see how associating a lock to the bg op will make it simpler. I
happen to think that my approach is simpler than anything involving locking.
I'll explore getting rid of the wq in favor of per-device as suggested by
Dave, but fundamentally would be along the same lines.

If we really want to make things simple, than maybe just not supporting
polling would be better - I assume most devices will have irq support
anyway.

...

>> +	/*
>> +	 * With bg polling this can overlap a scenario where the
>> +	 * hardware can receive new requests but the driver is
>> +	 * not ready. Handle accordingly.
>> +	 */
>> +	if (!cxlds->mbox_irq) {
>> +		rc = cxl_mbox_check_cmd_bgcmd(cxlds, opcode);
>> +		if (rc)
>> +			return rc;
>> +	}
>
>Can't happen with the straight-line approach as all an interrupt will do
>is wake up a wait_event_timeout() instance for the currently executing
>background command.

I don't follow here. Upon mbox irq support, getting an irq will have have
the isr simply do a:

     cxl_mbox_bgcmd_post();
     put_device(cxlds->dev);
     cxl_mbox_bgcmd_done(cxlds);

     return IRQ_HANDLED;

... to terminate the bg command and no wakeup is needed. But polling won't
be able to do this hence the check above. Maybe I'm just missing your point.

Thanks,
Davidlohr

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Davidlohr Bueso]

On Wed, 07 Dec 2022, Davidlohr Bueso wrote:

>>>+	/*
>>>+	 * With bg polling this can overlap a scenario where the
>>>+	 * hardware can receive new requests but the driver is
>>>+	 * not ready. Handle accordingly.
>>>+	 */
>>>+	if (!cxlds->mbox_irq) {
>>>+		rc = cxl_mbox_check_cmd_bgcmd(cxlds, opcode);
>>>+		if (rc)
>>>+			return rc;
>>>+	}
>>
>>Can't happen with the straight-line approach as all an interrupt will do
>>is wake up a wait_event_timeout() instance for the currently executing
>>background command.
>
>I don't follow here. Upon mbox irq support, getting an irq will have have
>the isr simply do a:
>
>    cxl_mbox_bgcmd_post();
>    put_device(cxlds->dev);
>    cxl_mbox_bgcmd_done(cxlds);
>
>    return IRQ_HANDLED;
>
>... to terminate the bg command and no wakeup is needed. But polling won't
>be able to do this hence the check above. Maybe I'm just missing your point.

Ok so I'm assuming here cxl_mbox_bgcmd_post() can run in irq context (this
would of course need to be documented). If for example doing a wbinvd_on_all_cpus()
requires a threaded irq (I have seen usage in the kernel that uses it is sleepable
contexts), then we cannot avoid the cxl_mbox_check_cmd_bgcmd() check. The same would
apply for any other bg command in the future that implements ->post() callback.

Thanks,
Davidlohr

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Dan Williams]

Davidlohr Bueso wrote:
> On Tue, 06 Dec 2022, Dan Williams wrote:
> 
> >One of the recent revelations with this patch:
> >
> >https://lore.kernel.org/linux-cxl/167030056464.4044561.11486507095384253833.stgit@dwillia2-xfh.jf.intel.com/
> >
> >...is that commands that are not meant to be called from userspace
> >should simply never appear in CXL_CMDS nor cxl_mem_commands. The
> >motivation for defining SCAN_MEDIA and friends was to use their
> >definitions to block the RAW command passthrough, but ironically that
> >enables the ioctl path which is also unwanted.
> >
> >So I think this patchset first needs to mark those uapi command numbers
> >deprecated. Then ensure that all other background commands can not be
> >submitted via the ioctl path.
> 
> Ok I will have a look.
> 
> >
> >>	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
> >>	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
> >>	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
> >> @@ -145,6 +151,136 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
> >>	return cxl_command_names[c->info.id].name;
> >>  }
> >>
> >> +static unsigned long __maybe_unused
> >> +cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode)
> >> +{
> >> +	struct cxl_mem_command *c;
> >> +	unsigned long ret = 0;
> >> +
> >> +	c = cxl_mem_find_command(opcode);
> >
> >Per that change above, cxl_mem_find_command() is only for the uapi.
> >
> >> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
> >> +		dev_WARN_ONCE(cxlds->dev, true,
> >> +			      "Not a background command\n");
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (c->bgops && c->bgops->delay)
> >> +		ret = c->bgops->delay(cxlds);
> >> +
> >> +	return ret * HZ;
> >> +}
> >
> >In general I worry that this is a bit too flexible. Instead of callback
> >ops I think this wants to be straight line code with something like a
> >poll_interval argument in 'struct cxl_mbox_cmd' where 0 means the
> >command is immediate and non-zero is a number of jiffies between
> >background operation polling intervals.
> 
> Are you refering _only_ to the delay callback being too flexible or the
> the whole set of callbacks? If only delay, I don't like the idea of
> having a poll_interval argument *and* a bgops structure with post and
> conflicts callbacks. We could also move conflicts functionality to
> another argument in cxl_mbox_cmd structure, but not delay functionality,
> which makes consolidating everything in a callback structure nicer imo.

I am refering to all of the callback operations, because the callback
design assumes that the CXL specification actually defined a functional
background command facility which it did not. A functional background
mechanism would arrange for queueing to overlap with result reaping,
like an NVME ring buffer*. As it stands the driver needs to be careful to
reap previous results before issuing the next command which complicates
the implementation for little benefit.

A straight-line implementation eliminates the conflict concept. It moves
the details to the caller about how to time-slice the operation, or in
the case of overwrite, allow it monopolize the mailbox since the device
is effectively dead during the overwrite.

---

* it turns out the Bind vPPB command got this right. The command
completion is queued to an event not to the singular background command
status that needs collision protection.

> 
> >
> >> +
> >> +static void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds,
> >> +				u16 opcode, bool success)
> >> +{
> >> +	struct cxl_mem_command *c;
> >> +
> >> +	c = cxl_mem_find_command(opcode);
> >> +	if (!c || !(c->flags & CXL_CMD_FLAG_BACKGROUND)) {
> >> +		dev_WARN_ONCE(cxlds->dev, true,
> >> +			      "Not a background command\n");
> >> +		return;
> >> +	}
> >> +
> >> +	if (c->bgops && c->bgops->post)
> >> +		c->bgops->post(cxlds, success);
> >> +}
> >> +
> >> +/*
> >> + * Ensure that ->mbox_send(@new) can run safely when a background
> >> + * command is running. If so, returns zero, otherwise error.
> >> + *
> >> + * check 1. bg cmd running. If not running it is fine to
> >> + *	    send any command. If one is running then there
> >> + *	    may be restrictions.
> >> + * check 2. @new incoming command is capable of running
> >> + *          in the background. If there is an in-flight bg
> >> + *          operation, all these are forbidden as we need
> >> + *          to serialize when polling.
> >> + * check 3. @new incoming command is not blacklisted by the
> >> + *          current running command.
> >> + */
> >> +static int __maybe_unused
> >> +cxl_mbox_check_cmd_bgcmd(struct cxl_dev_state *cxlds, u16 new)
> >> +{
> >> +	struct cxl_mem_command *c;
> >> +
> >> +	/* 1 */
> >> +	if (likely(!cxl_mbox_bgcmd_running(cxlds)))
> >> +		return 0;
> >> +
> >> +	c = cxl_mem_find_command(new);
> >> +	if (!c)
> >> +		return -EINVAL;
> >> +
> >> +	/* 2 */
> >> +	if (c->flags & CXL_CMD_FLAG_BACKGROUND)
> >> +		return -EBUSY;
> >> +
> >> +	/* 3 */
> >> +	if (c->bgops && c->bgops->conflicts)
> >> +		return c->bgops->conflicts(new);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/*
> >> + * Background operation polling mode.
> >> + */
> >> +void cxl_mbox_bgcmd_work(struct work_struct *work)
> >
> >Is a workqueue needed? If the goal is to make it so that all the kernel
> >background command use cases chop up their work so that one command can
> >not monopolize the mailbox for a long amount of time then the
> >implementation can also assume that foreground commands that collide
> >with an in-flight background operation can just wait synchronously for
> >their time slice.
> >
> >That makes the handling simple, acquire lock, poll for completion,
> >and everyone else just queues up on the lock.
> 
> I don't see how associating a lock to the bg op will make it simpler. I
> happen to think that my approach is simpler than anything involving locking.
> I'll explore getting rid of the wq in favor of per-device as suggested by
> Dave, but fundamentally would be along the same lines.
> 
> If we really want to make things simple, than maybe just not supporting
> polling would be better - I assume most devices will have irq support
> anyway.

Hmm, I'm just not (yet) grokking how the callbacks or irq support
simplify the implementation. Perhaps I can explain better in code...

I see irq support as a small incremental extension of polling support,
not something that precludes polling support. For example (untested /
unfinished):

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 3a66aadb4df0..30b21ae4cbe5 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -178,6 +178,19 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
        mbox_cmd->return_code =
                FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
 
+       if (mbox_cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
+               int i;
+
+               for (i = 0; i < mbox_cmd->poll_count; i++)
+                       rc = wait_event_interruptible_timeout(
+                               &mbox_wait, cxl_background_complete(cxlds),
+                               mbox_cmd->poll_interval);
+
+               if (!cxl_background_complete(cxlds))
+                       cxl_cmd_err(cxlds->dev, mbox_cmd, md_status,
+                                   "background timeout");
+       }
+
        if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
                dev_dbg(dev, "Mailbox operation had an error: %s\n",
                        cxl_mbox_cmd_rc2str(mbox_cmd));

...where all irq support adds is a small handler that does:

    wake_up(&mbox_wait);
    return IRQ_HANDLED;

...to short-circuit the wait in __cxl_pci_mbox_send_cmd().

The cxl_internal_send_cmd() caller gets a synchronous response and can
handle it from there. No need for conflict detection nor async execution
contexts. Caller is responsible for making sure ->poll_count and
->poll_interval are reasonable.

> 
> ...
> 
> >> +	/*
> >> +	 * With bg polling this can overlap a scenario where the
> >> +	 * hardware can receive new requests but the driver is
> >> +	 * not ready. Handle accordingly.
> >> +	 */
> >> +	if (!cxlds->mbox_irq) {
> >> +		rc = cxl_mbox_check_cmd_bgcmd(cxlds, opcode);
> >> +		if (rc)
> >> +			return rc;
> >> +	}
> >
> >Can't happen with the straight-line approach as all an interrupt will do
> >is wake up a wait_event_timeout() instance for the currently executing
> >background command.
> 
> I don't follow here. Upon mbox irq support, getting an irq will have have
> the isr simply do a:
> 
>      cxl_mbox_bgcmd_post();
>      put_device(cxlds->dev);

Side comment: an isr should never need put_device() because the irq must
be shutdown before the device is unregistered.

>      cxl_mbox_bgcmd_done(cxlds);

The complexity I see here is the synchronization needed to make sure
that the isr is aligned with the current background command queued for
execution and in turn synchronized with incoming commands. My
observation is that current mailbox state synchronization is
sufficient for:

- Transfer FW: broken up into multiple transfers
- Scan Media: broken up by Scan Media Physical Address Length values
  that try to get the Estimated Scan Media Time below
  CXL_MAILBOX_TIMEOUT_MS per iteration.
- Sanitize: the device is effectively dead during this operation, so
  there is little need to time-share

I do have questions about:

- Activate FW: not clear how long an activation could take in the online
  case, and no way to minimize it. Might be something that ends up being
  handled by Documentation about specific device behavior.
- Populate Log: ugh, any log for any reason for an indefinite amount of
  time. This one needs an example use case of what the kernel would do
  with it.
- Maintenance: I think this is in the same category as Sanitize in terms
  of recovery of a failed / offline device where command collisions do
  not matter.

...but I do not see those questions as prohibitive to the one-at-a-time
approach for the commands that are going to be enabled in the kernel in
the near to medium term.

> 
>      return IRQ_HANDLED;
> 
> ... to terminate the bg command and no wakeup is needed. But polling won't
> be able to do this hence the check above. Maybe I'm just missing your point.
> 
> Thanks,
> Davidlohr

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Davidlohr Bueso]

On Wed, 07 Dec 2022, Dan Williams wrote:

>Hmm, I'm just not (yet) grokking how the callbacks or irq support
>simplify the implementation. Perhaps I can explain better in code...

So when I say that no polling at all makes the implementation simpler I'm
referring to the fact that only some form of ->post() would be needed (in
an async context of course). As such, most of the infrastructure in this
series is to support polling, and without it we could do away with the
other callbacks as well as the atomics. The hardware would take care
"synchronizing" everything - which is similar to what Jonathan had
highlighted in the rfc version and was wondering about the explicit
checks.

>I see irq support as a small incremental extension of polling support,
>not something that precludes polling support. For example (untested /
>unfinished):
>
>diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>index 3a66aadb4df0..30b21ae4cbe5 100644
>--- a/drivers/cxl/pci.c
>+++ b/drivers/cxl/pci.c
>@@ -178,6 +178,19 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>        mbox_cmd->return_code =
>                FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
>
>+       if (mbox_cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
>+               int i;
>+
>+               for (i = 0; i < mbox_cmd->poll_count; i++)
>+                       rc = wait_event_interruptible_timeout(
>+                               &mbox_wait, cxl_background_complete(cxlds),
>+                               mbox_cmd->poll_interval);
>+
>+               if (!cxl_background_complete(cxlds))
>+                       cxl_cmd_err(cxlds->dev, mbox_cmd, md_status,
>+                                   "background timeout");
>+       }
>+
>        if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
>                dev_dbg(dev, "Mailbox operation had an error: %s\n",
>                        cxl_mbox_cmd_rc2str(mbox_cmd));
>
>...where all irq support adds is a small handler that does:
>
>    wake_up(&mbox_wait);
>    return IRQ_HANDLED;
>
>...to short-circuit the wait in __cxl_pci_mbox_send_cmd().

I agree that under this synchronous scheme polling and irq are quite
straightforward. I had always considered an async approach and not
playing with timeouts, but just let the whole thing run to completion.
I was also guided by the fact that the spec says:

"It is strongly recommended that devices continue to accept new non-
background commands while the background operation is running."

What I don't like about a synchronous approach is that you get the
situation where the driver timesout, but the hw is still doing the
background command, no? And there is no way to "cancel" it. Sure,
any new incoming mbox commands during that time would self-serialize
naturally by hw. But when the bg command is done and we have irqs enabled,
the isr will be triggered and we no longer have the context. You would
also have a discrepancy in a returned timeout error vs a different
return code from the actual hw, specially if for example the operation
was successful. afaict the only thing that would allow users to know
the real status of the sent bg command would be the Background Operation
Status command, but that is far from ideal.

>The cxl_internal_send_cmd() caller gets a synchronous response and can
>handle it from there. No need for conflict detection nor async execution
>contexts. Caller is responsible for making sure ->poll_count and
>->poll_interval are reasonable.

...

>The complexity I see here is the synchronization needed to make sure
>that the isr is aligned with the current background command queued for
>execution and in turn synchronized with incoming commands. My
>observation is that current mailbox state synchronization is
>sufficient for:
>
>- Transfer FW: broken up into multiple transfers
>- Scan Media: broken up by Scan Media Physical Address Length values
>  that try to get the Estimated Scan Media Time below
>  CXL_MAILBOX_TIMEOUT_MS per iteration.
>- Sanitize: the device is effectively dead during this operation, so
>  there is little need to time-share

But tbh I find it much more intuitive to get an error then having to
block for who knows how long. Either way this would still require special
cases such as sanitize where the poller should not timeout. Of course
there is nothing in the approach from this series that prevents users
from being mindful of the background command and breaking things up,
where possible.

Thanks,
Davidlohr

Re: [PATCH 1/3] cxl/mbox: Add background operation handling machinery

[Dan Williams]

Davidlohr Bueso wrote:
> On Wed, 07 Dec 2022, Dan Williams wrote:
> 
> >Hmm, I'm just not (yet) grokking how the callbacks or irq support
> >simplify the implementation. Perhaps I can explain better in code...
> 
> So when I say that no polling at all makes the implementation simpler I'm
> referring to the fact that only some form of ->post() would be needed (in
> an async context of course). As such, most of the infrastructure in this
> series is to support polling, and without it we could do away with the
> other callbacks as well as the atomics. The hardware would take care
> "synchronizing" everything - which is similar to what Jonathan had
> highlighted in the rfc version and was wondering about the explicit
> checks.
> 
> >I see irq support as a small incremental extension of polling support,
> >not something that precludes polling support. For example (untested /
> >unfinished):
> >
> >diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> >index 3a66aadb4df0..30b21ae4cbe5 100644
> >--- a/drivers/cxl/pci.c
> >+++ b/drivers/cxl/pci.c
> >@@ -178,6 +178,19 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
> >        mbox_cmd->return_code =
> >                FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
> >
> >+       if (mbox_cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
> >+               int i;
> >+
> >+               for (i = 0; i < mbox_cmd->poll_count; i++)
> >+                       rc = wait_event_interruptible_timeout(
> >+                               &mbox_wait, cxl_background_complete(cxlds),
> >+                               mbox_cmd->poll_interval);
> >+
> >+               if (!cxl_background_complete(cxlds))
> >+                       cxl_cmd_err(cxlds->dev, mbox_cmd, md_status,
> >+                                   "background timeout");
> >+       }
> >+
> >        if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
> >                dev_dbg(dev, "Mailbox operation had an error: %s\n",
> >                        cxl_mbox_cmd_rc2str(mbox_cmd));
> >
> >...where all irq support adds is a small handler that does:
> >
> >    wake_up(&mbox_wait);
> >    return IRQ_HANDLED;
> >
> >...to short-circuit the wait in __cxl_pci_mbox_send_cmd().
> 
> I agree that under this synchronous scheme polling and irq are quite
> straightforward. I had always considered an async approach and not
> playing with timeouts, but just let the whole thing run to completion.
> I was also guided by the fact that the spec says:
> 
> "It is strongly recommended that devices continue to accept new non-
> background commands while the background operation is running."

A "strong recommendation" without a specification defined mechanism to
know when a device adheres to that is effectively useless for the
driver. If it needs to consider that failure case at all, it might as
well skip the optimized path for what are slow-path operations that do
not demand optimization.

> 
> What I don't like about a synchronous approach is that you get the
> situation where the driver timesout, but the hw is still doing the
> background command, no? And there is no way to "cancel" it. Sure,
> any new incoming mbox commands during that time would self-serialize
> naturally by hw. But when the bg command is done and we have irqs enabled,
> the isr will be triggered and we no longer have the context. You would
> also have a discrepancy in a returned timeout error vs a different
> return code from the actual hw, specially if for example the operation
> was successful. afaict the only thing that would allow users to know
> the real status of the sent bg command would be the Background Operation
> Status command, but that is far from ideal.

The kernel needs to give up at some point, so the status for that lost
command is "timeout". Then the mailbox state is indeterminate until the
next successful command submission and the driver can get back in sync
with the hardware state.

The typical solution for this case is trigger device error handling up
to and including reset to get the driver back in sync with the hardware
state. Unless I am overlooking something CXL is atypical / stuck because
any resets that might recover the mbox are destructive to HDM decoding.

> >The cxl_internal_send_cmd() caller gets a synchronous response and can
> >handle it from there. No need for conflict detection nor async execution
> >contexts. Caller is responsible for making sure ->poll_count and
> >->poll_interval are reasonable.
> 
> ...
> 
> >The complexity I see here is the synchronization needed to make sure
> >that the isr is aligned with the current background command queued for
> >execution and in turn synchronized with incoming commands. My
> >observation is that current mailbox state synchronization is
> >sufficient for:
> >
> >- Transfer FW: broken up into multiple transfers
> >- Scan Media: broken up by Scan Media Physical Address Length values
> >  that try to get the Estimated Scan Media Time below
> >  CXL_MAILBOX_TIMEOUT_MS per iteration.
> >- Sanitize: the device is effectively dead during this operation, so
> >  there is little need to time-share
> 
> But tbh I find it much more intuitive to get an error then having to
> block for who knows how long.

Yes, agree it would be reasonable to just have the driver reject all
other commands while sanitize is in-flight.

> Either way this would still require special cases such as sanitize
> where the poller should not timeout. Of course there is nothing in the
> approach from this series that prevents users from being mindful of
> the background command and breaking things up, where possible.

Right, especially because all the users are kernel internal. So it's a
code complexity question. It's easier to add flexibility than rip it out
later so I am leaning towards excruciatingly simple until that becomes
unbearable... which in retrospect would have saved some of the DOE
implementation thrash.

[PATCH 2/3] cxl/mem: Support sanitation commands

[Davidlohr Bueso]

Implement support for the non-pmem exclusive sanitize (aka overwrite)
and secure erase commands, per CXL specs.

To properly support this feature, create a 'security' sysfs file that
when read will list the current pmem security state or overwrite, and
when written to, perform the requested operation.

As with ndctl-speak, the use cases here would be:

$> cxl sanitize --erase memX
$> cxl sanitize --overwrite memX
$> cxl sanitize --wait-overwrite memX

Where userspace can implement entirely the wait/query mechanism for
waiting for the sanitize to complete (albeit no poll support for
the security sysfs file).

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 Documentation/ABI/testing/sysfs-bus-cxl |  19 +++++
 drivers/cxl/core/mbox.c                 | 104 +++++++++++++++++++++++-
 drivers/cxl/core/memdev.c               |  54 ++++++++++++
 drivers/cxl/cxlmem.h                    |  10 +++
 include/uapi/linux/cxl_mem.h            |   2 +
 5 files changed, 188 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 8494ef27e8d2..18e26ae3d75f 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -58,6 +58,25 @@ Description:
 		affinity for this device.
 
 
+What:		/sys/bus/cxl/devices/memX/security
+Date:		December, 2022
+KernelVersion:	v6.2
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		Reading this file will display the security state for that
+		device. The following states are available: disabled, frozen,
+		locked, unlocked and overwrite. When writing to the file, the
+		following commands are supported:
+		* overwrite - Sanitize the device to securely re-purpose or
+		  decommission it. This is done by ensuring that all user data
+		  and meta-data, whether it resides in persistent capacity,
+		  volatile capacity, or the label storage area, is made
+		  permanently unavailable by whatever means is appropriate for
+		  the media type. This causes all CPU caches to be flushed.
+		* erase - Secure Erase user data by changing the media encryption
+		  keys for all user data areas of the device. This causes all
+		  CPU caches to be flushed.
+
 What:		/sys/bus/cxl/devices/*/devtype
 Date:		June, 2021
 KernelVersion:	v5.14
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index bfee9251c81c..5ffdab10073d 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -4,6 +4,7 @@
 #include <linux/security.h>
 #include <linux/debugfs.h>
 #include <linux/mutex.h>
+#include <linux/memregion.h>
 #include <cxlmem.h>
 #include <cxl.h>
 
@@ -11,6 +12,7 @@
 
 static bool cxl_raw_allow_all;
 static struct workqueue_struct *cxl_mbox_bgpoll_wq;
+static struct cxl_mem_bgcommand_ops sanitize_bgops;
 
 /**
  * DOC: cxl mbox
@@ -70,6 +72,8 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
 	CXL_CMD(CLEAR_POISON, 0x48, 0, 0),
 	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
 	CXL_BGCMD(SCAN_MEDIA, 0x11, 0, 0, NULL),
+	CXL_BGCMD(SANITIZE, 0, 0, 0, &sanitize_bgops),
+	CXL_CMD(SECURE_ERASE, 0, 0, 0),
 	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
 	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
 	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
@@ -136,7 +140,6 @@ static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
 	cxl_for_each_cmd(c)
 		if (c->opcode == opcode)
 			return c;
-
 	return NULL;
 }
 
@@ -887,6 +890,8 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds)
 	 * Setup permanently kernel exclusive commands, i.e. the
 	 * mechanism is driven through sysfs, keyctl, etc...
 	 */
+	set_bit(CXL_MEM_COMMAND_ID_SANITIZE, cxlds->exclusive_cmds);
+	set_bit(CXL_MEM_COMMAND_ID_SECURE_ERASE, cxlds->exclusive_cmds);
 	set_bit(CXL_MEM_COMMAND_ID_SET_PASSPHRASE, cxlds->exclusive_cmds);
 	set_bit(CXL_MEM_COMMAND_ID_DISABLE_PASSPHRASE, cxlds->exclusive_cmds);
 	set_bit(CXL_MEM_COMMAND_ID_UNLOCK, cxlds->exclusive_cmds);
@@ -970,6 +975,103 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_dev_state_identify, CXL);
 
+static int sanitize_bgcmd_conflicts(u16 new)
+{
+	/* forbid anyone but health related commands */
+	if (new == CXL_MBOX_OP_GET_HEALTH_INFO)
+		return 0;
+	return -EBUSY;
+}
+
+static unsigned long sanitize_bgcmd_delay(struct cxl_dev_state *cxlds)
+{
+	unsigned int tmo;
+
+	if (!cxlds)
+		return 0;
+
+	tmo = cxlds->sec.sanitize_tmo + 10;
+	cxlds->sec.sanitize_tmo = min(15U * 60U, tmo);
+	return tmo * HZ;
+}
+
+static void sanitize_bgcmd_post(struct cxl_dev_state *cxlds, bool success)
+{
+	if (!cxlds->mbox_irq)
+		cxlds->sec.sanitize_tmo = 0;
+	if (success)
+		cpu_cache_invalidate_memregion(IORES_DESC_CXL);
+}
+
+static struct cxl_mem_bgcommand_ops sanitize_bgops = {
+	.conflicts = sanitize_bgcmd_conflicts,
+	.delay = sanitize_bgcmd_delay,
+	.post = sanitize_bgcmd_post,
+};
+
+/**
+ * cxl_mem_sanitize() - Send sanitation related commands to the device.
+ * @cxlds: The device data for the operation
+ * @op: The command opcode to send
+ *
+ * Return: 0 if the command was executed successfully, regardless of
+ * whether or not the actual security operation is done in the background.
+ * Upon error, return the result of the mailbox command or -EINVAL if
+ * security requirements are not met. CPU caches are flushed before and
+ * after succesful completion of each command.
+ *
+ * See CXL 2.0 @8.2.9.5.5 Sanitize.
+ */
+int cxl_mem_sanitize(struct cxl_dev_state *cxlds, u16 op)
+{
+	int rc;
+	u32 sec_out;
+	struct cxl_mem_command *cmd;
+
+	cmd = cxl_mem_find_command(op);
+	if (!cmd || !test_bit(cmd->info.id, cxlds->enabled_cmds))
+		return -EINVAL;
+
+	if (!cpu_cache_has_invalidate_memregion())
+		return -EINVAL;
+
+	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE,
+			       NULL, 0, &sec_out, sizeof(sec_out), NULL);
+	if (rc)
+		return rc;
+
+	/*
+	 * Prior to using these commands, any security applied to
+	 * the user data areas of the device shall be DISABLED (or
+	 * UNLOCKED for secure erase case).
+	 */
+	if (sec_out & CXL_PMEM_SEC_STATE_USER_PASS_SET)
+		return -EINVAL;
+
+	if (op == CXL_MBOX_OP_SANITIZE) {
+		u16 ret_code; /* hw */
+
+		cpu_cache_invalidate_memregion(IORES_DESC_CXL);
+
+		rc = cxl_mbox_send_cmd(cxlds, op, NULL, 0, NULL, 0, &ret_code);
+		if (rc == 0 && ret_code != CXL_MBOX_CMD_RC_BACKGROUND)
+			cpu_cache_invalidate_memregion(IORES_DESC_CXL);
+	} else if (op == CXL_MBOX_OP_SECURE_ERASE) {
+		if (sec_out & CXL_PMEM_SEC_STATE_LOCKED)
+			return -EINVAL;
+
+		cpu_cache_invalidate_memregion(IORES_DESC_CXL);
+
+		rc = cxl_mbox_send_cmd(cxlds, op, NULL, 0, NULL, 0, NULL);
+		if (rc == 0)
+			cpu_cache_invalidate_memregion(IORES_DESC_CXL);
+	} else
+		rc = -EINVAL;
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_sanitize, CXL);
+
 static int add_dpa_res(struct device *dev, struct resource *parent,
 		       struct resource *res, resource_size_t start,
 		       resource_size_t size, const char *type)
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 20ce488a7754..0b79c2b6720e 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -106,12 +106,66 @@ static ssize_t numa_node_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(numa_node);
 
+static ssize_t security_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	u32 sec_out = 0;
+	int rc;
+
+	if (cxl_mbox_bgcmd_running(cxlds) == CXL_MBOX_OP_SANITIZE)
+		return sprintf(buf, "overwrite\n");
+
+	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE,
+			       NULL, 0, &sec_out, sizeof(sec_out), NULL);
+	if (rc)
+		return rc;
+
+	if (!(sec_out & CXL_PMEM_SEC_STATE_USER_PASS_SET))
+		return sprintf(buf, "disabled\n");
+	if (sec_out & CXL_PMEM_SEC_STATE_FROZEN)
+		return sprintf(buf, "frozen\n");
+	if (sec_out & CXL_PMEM_SEC_STATE_LOCKED)
+		return sprintf(buf, "locked\n");
+	else
+		return sprintf(buf, "unlocked\n");
+}
+
+#define CXL_SEC_CMD_SIZE 32
+static ssize_t security_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t len)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	char cmd[CXL_SEC_CMD_SIZE+1];
+	ssize_t rc;
+
+	rc = sscanf(buf, "%"__stringify(CXL_SEC_CMD_SIZE)"s", cmd);
+	if (rc < 1)
+		return -EINVAL;
+
+	if (sysfs_streq(cmd, "overwrite"))
+		rc = cxl_mem_sanitize(cxlds, CXL_MBOX_OP_SANITIZE);
+	else if (sysfs_streq(cmd, "erase"))
+		rc = cxl_mem_sanitize(cxlds, CXL_MBOX_OP_SECURE_ERASE);
+	else
+		rc = -EINVAL;
+
+	if (rc == 0)
+		rc = len;
+	return rc;
+}
+static DEVICE_ATTR_RW(security);
+
 static struct attribute *cxl_memdev_attributes[] = {
 	&dev_attr_serial.attr,
 	&dev_attr_firmware_version.attr,
 	&dev_attr_payload_max.attr,
 	&dev_attr_label_storage_size.attr,
 	&dev_attr_numa_node.attr,
+	&dev_attr_security.attr,
 	NULL,
 };
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index e7cb2f2fadc4..1dd1caabd41c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -215,6 +215,7 @@ struct cxl_endpoint_dvsec_info {
  * @mbox_irq: @dev supports mailbox interrupts
  * @mbox_bg: opcode for the in-flight background operation on @dev
  * @mbox_bgpoll: self-polling delayed work item
+ * @sec: device security
  *
  * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
  * details on capacity parameters.
@@ -254,6 +255,10 @@ struct cxl_dev_state {
 	bool mbox_irq;
 	atomic_t mbox_bg;
 	struct delayed_work __maybe_unused mbox_bgpoll;
+
+	struct {
+		unsigned int __maybe_unused sanitize_tmo;
+	} sec;
 };
 
 enum cxl_opcode {
@@ -279,6 +284,8 @@ enum cxl_opcode {
 	CXL_MBOX_OP_GET_SCAN_MEDIA_CAPS	= 0x4303,
 	CXL_MBOX_OP_SCAN_MEDIA		= 0x4304,
 	CXL_MBOX_OP_GET_SCAN_MEDIA	= 0x4305,
+	CXL_MBOX_OP_SANITIZE		= 0x4400,
+	CXL_MBOX_OP_SECURE_ERASE	= 0x4401,
 	CXL_MBOX_OP_GET_SECURITY_STATE	= 0x4500,
 	CXL_MBOX_OP_SET_PASSPHRASE	= 0x4501,
 	CXL_MBOX_OP_DISABLE_PASSPHRASE	= 0x4502,
@@ -473,6 +480,7 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
 struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
 void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
 void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
+
 #ifdef CONFIG_CXL_SUSPEND
 void cxl_mem_active_inc(void);
 void cxl_mem_active_dec(void);
@@ -487,6 +495,8 @@ static inline void cxl_mem_active_dec(void)
 
 void cxl_mbox_bgcmd_work(struct work_struct *work);
 
+int cxl_mem_sanitize(struct cxl_dev_state *cxlds, u16 cmd);
+
 struct cxl_hdm {
 	struct cxl_component_regs regs;
 	unsigned int decoder_count;
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index 82bdad4ce5de..fbf619976eb7 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -40,6 +40,8 @@
 	___C(CLEAR_POISON, "Clear Poison"),                               \
 	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
 	___C(SCAN_MEDIA, "Scan Media"),                                   \
+	___C(SANITIZE, "Sanitize"),                                       \
+	___C(SECURE_ERASE, "Secure Erase"),				  \
 	___C(GET_SCAN_MEDIA, "Get Scan Media Results"),                   \
 	___C(GET_SECURITY_STATE, "Get Security State"),			  \
 	___C(SET_PASSPHRASE, "Set Passphrase"),				  \
-- 
2.38.1

RE: [PATCH 2/3] cxl/mem: Support sanitation commands

[Dan Williams]

Davidlohr Bueso wrote:
> Implement support for the non-pmem exclusive sanitize (aka overwrite)
> and secure erase commands, per CXL specs.
> 
> To properly support this feature, create a 'security' sysfs file that
> when read will list the current pmem security state or overwrite, and
> when written to, perform the requested operation.
> 
> As with ndctl-speak, the use cases here would be:
> 
> $> cxl sanitize --erase memX
> $> cxl sanitize --overwrite memX
> $> cxl sanitize --wait-overwrite memX
> 
> Where userspace can implement entirely the wait/query mechanism for
> waiting for the sanitize to complete (albeit no poll support for
> the security sysfs file).
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |  19 +++++
>  drivers/cxl/core/mbox.c                 | 104 +++++++++++++++++++++++-
>  drivers/cxl/core/memdev.c               |  54 ++++++++++++
>  drivers/cxl/cxlmem.h                    |  10 +++
>  include/uapi/linux/cxl_mem.h            |   2 +
>  5 files changed, 188 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 8494ef27e8d2..18e26ae3d75f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -58,6 +58,25 @@ Description:
>  		affinity for this device.
>  
>  
> +What:		/sys/bus/cxl/devices/memX/security
> +Date:		December, 2022
> +KernelVersion:	v6.2
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		Reading this file will display the security state for that
> +		device. The following states are available: disabled, frozen,
> +		locked, unlocked and overwrite. When writing to the file, the
> +		following commands are supported:
> +		* overwrite - Sanitize the device to securely re-purpose or
> +		  decommission it. This is done by ensuring that all user data
> +		  and meta-data, whether it resides in persistent capacity,
> +		  volatile capacity, or the label storage area, is made
> +		  permanently unavailable by whatever means is appropriate for
> +		  the media type. This causes all CPU caches to be flushed.
> +		* erase - Secure Erase user data by changing the media encryption
> +		  keys for all user data areas of the device. This causes all
> +		  CPU caches to be flushed.
> +
>  What:		/sys/bus/cxl/devices/*/devtype
>  Date:		June, 2021
>  KernelVersion:	v5.14
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index bfee9251c81c..5ffdab10073d 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -4,6 +4,7 @@
>  #include <linux/security.h>
>  #include <linux/debugfs.h>
>  #include <linux/mutex.h>
> +#include <linux/memregion.h>
>  #include <cxlmem.h>
>  #include <cxl.h>
>  
> @@ -11,6 +12,7 @@
>  
>  static bool cxl_raw_allow_all;
>  static struct workqueue_struct *cxl_mbox_bgpoll_wq;
> +static struct cxl_mem_bgcommand_ops sanitize_bgops;
>  
>  /**
>   * DOC: cxl mbox
> @@ -70,6 +72,8 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(CLEAR_POISON, 0x48, 0, 0),
>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>  	CXL_BGCMD(SCAN_MEDIA, 0x11, 0, 0, NULL),
> +	CXL_BGCMD(SANITIZE, 0, 0, 0, &sanitize_bgops),
> +	CXL_CMD(SECURE_ERASE, 0, 0, 0),
>  	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
>  	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
>  	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
> @@ -136,7 +140,6 @@ static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
>  	cxl_for_each_cmd(c)
>  		if (c->opcode == opcode)
>  			return c;
> -
>  	return NULL;
>  }
>  
> @@ -887,6 +890,8 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds)
>  	 * Setup permanently kernel exclusive commands, i.e. the
>  	 * mechanism is driven through sysfs, keyctl, etc...
>  	 */
> +	set_bit(CXL_MEM_COMMAND_ID_SANITIZE, cxlds->exclusive_cmds);
> +	set_bit(CXL_MEM_COMMAND_ID_SECURE_ERASE, cxlds->exclusive_cmds);
>  	set_bit(CXL_MEM_COMMAND_ID_SET_PASSPHRASE, cxlds->exclusive_cmds);
>  	set_bit(CXL_MEM_COMMAND_ID_DISABLE_PASSPHRASE, cxlds->exclusive_cmds);
>  	set_bit(CXL_MEM_COMMAND_ID_UNLOCK, cxlds->exclusive_cmds);
> @@ -970,6 +975,103 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_dev_state_identify, CXL);
>  
> +static int sanitize_bgcmd_conflicts(u16 new)
> +{
> +	/* forbid anyone but health related commands */
> +	if (new == CXL_MBOX_OP_GET_HEALTH_INFO)
> +		return 0;
> +	return -EBUSY;
> +}
> +
> +static unsigned long sanitize_bgcmd_delay(struct cxl_dev_state *cxlds)
> +{
> +	unsigned int tmo;
> +
> +	if (!cxlds)
> +		return 0;
> +
> +	tmo = cxlds->sec.sanitize_tmo + 10;
> +	cxlds->sec.sanitize_tmo = min(15U * 60U, tmo);
> +	return tmo * HZ;
> +}
> +
> +static void sanitize_bgcmd_post(struct cxl_dev_state *cxlds, bool success)
> +{
> +	if (!cxlds->mbox_irq)
> +		cxlds->sec.sanitize_tmo = 0;
> +	if (success)
> +		cpu_cache_invalidate_memregion(IORES_DESC_CXL);
> +}
> +
> +static struct cxl_mem_bgcommand_ops sanitize_bgops = {
> +	.conflicts = sanitize_bgcmd_conflicts,
> +	.delay = sanitize_bgcmd_delay,
> +	.post = sanitize_bgcmd_post,
> +};
> +
> +/**
> + * cxl_mem_sanitize() - Send sanitation related commands to the device.
> + * @cxlds: The device data for the operation
> + * @op: The command opcode to send
> + *
> + * Return: 0 if the command was executed successfully, regardless of
> + * whether or not the actual security operation is done in the background.
> + * Upon error, return the result of the mailbox command or -EINVAL if
> + * security requirements are not met. CPU caches are flushed before and
> + * after succesful completion of each command.
> + *
> + * See CXL 2.0 @8.2.9.5.5 Sanitize.
> + */
> +int cxl_mem_sanitize(struct cxl_dev_state *cxlds, u16 op)
> +{
> +	int rc;
> +	u32 sec_out;
> +	struct cxl_mem_command *cmd;
> +
> +	cmd = cxl_mem_find_command(op);
> +	if (!cmd || !test_bit(cmd->info.id, cxlds->enabled_cmds))
> +		return -EINVAL;
> +
> +	if (!cpu_cache_has_invalidate_memregion())
> +		return -EINVAL;
> +
> +	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE,
> +			       NULL, 0, &sec_out, sizeof(sec_out), NULL);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Prior to using these commands, any security applied to
> +	 * the user data areas of the device shall be DISABLED (or
> +	 * UNLOCKED for secure erase case).
> +	 */
> +	if (sec_out & CXL_PMEM_SEC_STATE_USER_PASS_SET)
> +		return -EINVAL;
> +
> +	if (op == CXL_MBOX_OP_SANITIZE) {
> +		u16 ret_code; /* hw */
> +
> +		cpu_cache_invalidate_memregion(IORES_DESC_CXL);
> +
> +		rc = cxl_mbox_send_cmd(cxlds, op, NULL, 0, NULL, 0, &ret_code);
> +		if (rc == 0 && ret_code != CXL_MBOX_CMD_RC_BACKGROUND)
> +			cpu_cache_invalidate_memregion(IORES_DESC_CXL);
> +	} else if (op == CXL_MBOX_OP_SECURE_ERASE) {
> +		if (sec_out & CXL_PMEM_SEC_STATE_LOCKED)
> +			return -EINVAL;
> +
> +		cpu_cache_invalidate_memregion(IORES_DESC_CXL);
> +
> +		rc = cxl_mbox_send_cmd(cxlds, op, NULL, 0, NULL, 0, NULL);
> +		if (rc == 0)
> +			cpu_cache_invalidate_memregion(IORES_DESC_CXL);
> +	} else
> +		rc = -EINVAL;
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mem_sanitize, CXL);
> +
>  static int add_dpa_res(struct device *dev, struct resource *parent,
>  		       struct resource *res, resource_size_t start,
>  		       resource_size_t size, const char *type)
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 20ce488a7754..0b79c2b6720e 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -106,12 +106,66 @@ static ssize_t numa_node_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(numa_node);
>  
> +static ssize_t security_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	u32 sec_out = 0;
> +	int rc;
> +
> +	if (cxl_mbox_bgcmd_running(cxlds) == CXL_MBOX_OP_SANITIZE)
> +		return sprintf(buf, "overwrite\n");
> +
> +	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE,
> +			       NULL, 0, &sec_out, sizeof(sec_out), NULL);
> +	if (rc)
> +		return rc;
> +
> +	if (!(sec_out & CXL_PMEM_SEC_STATE_USER_PASS_SET))
> +		return sprintf(buf, "disabled\n");
> +	if (sec_out & CXL_PMEM_SEC_STATE_FROZEN)
> +		return sprintf(buf, "frozen\n");
> +	if (sec_out & CXL_PMEM_SEC_STATE_LOCKED)
> +		return sprintf(buf, "locked\n");
> +	else
> +		return sprintf(buf, "unlocked\n");
> +}
> +
> +#define CXL_SEC_CMD_SIZE 32
> +static ssize_t security_store(struct device *dev,
> +			      struct device_attribute *attr,
> +			      const char *buf, size_t len)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	char cmd[CXL_SEC_CMD_SIZE+1];
> +	ssize_t rc;
> +
> +	rc = sscanf(buf, "%"__stringify(CXL_SEC_CMD_SIZE)"s", cmd);
> +	if (rc < 1)
> +		return -EINVAL;
> +
> +	if (sysfs_streq(cmd, "overwrite"))
> +		rc = cxl_mem_sanitize(cxlds, CXL_MBOX_OP_SANITIZE);
> +	else if (sysfs_streq(cmd, "erase"))
> +		rc = cxl_mem_sanitize(cxlds, CXL_MBOX_OP_SECURE_ERASE);

The above needs quite a bit more safety that the device is not currently
actively decoding any HPA ranges.

I realize now after sending the comments in patch1 that Sanitize defeats
the dream of being able to timeslice background operations. On the other
hand, it really is a special case that is only meant to be run before
decommissioning the device. I think it is fine that it monopolizes the
device because the device is literally unusable during the process.

> +	else
> +		rc = -EINVAL;
> +
> +	if (rc == 0)
> +		rc = len;
> +	return rc;
> +}
> +static DEVICE_ATTR_RW(security);
> +
>  static struct attribute *cxl_memdev_attributes[] = {
>  	&dev_attr_serial.attr,
>  	&dev_attr_firmware_version.attr,
>  	&dev_attr_payload_max.attr,
>  	&dev_attr_label_storage_size.attr,
>  	&dev_attr_numa_node.attr,
> +	&dev_attr_security.attr,

This likely wants is_visible() handling to hide the attribute if the
device does not support security operations.

>  	NULL,
>  };
>  
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index e7cb2f2fadc4..1dd1caabd41c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -215,6 +215,7 @@ struct cxl_endpoint_dvsec_info {
>   * @mbox_irq: @dev supports mailbox interrupts
>   * @mbox_bg: opcode for the in-flight background operation on @dev
>   * @mbox_bgpoll: self-polling delayed work item
> + * @sec: device security
>   *
>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
>   * details on capacity parameters.
> @@ -254,6 +255,10 @@ struct cxl_dev_state {
>  	bool mbox_irq;
>  	atomic_t mbox_bg;
>  	struct delayed_work __maybe_unused mbox_bgpoll;
> +
> +	struct {
> +		unsigned int __maybe_unused sanitize_tmo;
> +	} sec;

I missed this on the last patch, but __maybe_unused has no effect at
declaration time, only at definition time. Otherwise, I wish I had a
facility that would tell me which of my structure members are no longer
used in code.

>  };
>  
>  enum cxl_opcode {
> @@ -279,6 +284,8 @@ enum cxl_opcode {
>  	CXL_MBOX_OP_GET_SCAN_MEDIA_CAPS	= 0x4303,
>  	CXL_MBOX_OP_SCAN_MEDIA		= 0x4304,
>  	CXL_MBOX_OP_GET_SCAN_MEDIA	= 0x4305,
> +	CXL_MBOX_OP_SANITIZE		= 0x4400,
> +	CXL_MBOX_OP_SECURE_ERASE	= 0x4401,
>  	CXL_MBOX_OP_GET_SECURITY_STATE	= 0x4500,
>  	CXL_MBOX_OP_SET_PASSPHRASE	= 0x4501,
>  	CXL_MBOX_OP_DISABLE_PASSPHRASE	= 0x4502,
> @@ -473,6 +480,7 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
>  struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
>  void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
>  void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
> +
>  #ifdef CONFIG_CXL_SUSPEND
>  void cxl_mem_active_inc(void);
>  void cxl_mem_active_dec(void);
> @@ -487,6 +495,8 @@ static inline void cxl_mem_active_dec(void)
>  
>  void cxl_mbox_bgcmd_work(struct work_struct *work);
>  
> +int cxl_mem_sanitize(struct cxl_dev_state *cxlds, u16 cmd);
> +
>  struct cxl_hdm {
>  	struct cxl_component_regs regs;
>  	unsigned int decoder_count;
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 82bdad4ce5de..fbf619976eb7 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -40,6 +40,8 @@
>  	___C(CLEAR_POISON, "Clear Poison"),                               \
>  	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
>  	___C(SCAN_MEDIA, "Scan Media"),                                   \
> +	___C(SANITIZE, "Sanitize"),                                       \
> +	___C(SECURE_ERASE, "Secure Erase"),				  \
>  	___C(GET_SCAN_MEDIA, "Get Scan Media Results"),                   \
>  	___C(GET_SECURITY_STATE, "Get Security State"),			  \
>  	___C(SET_PASSPHRASE, "Set Passphrase"),				  \
> -- 
> 2.38.1
> 

Re: [PATCH 2/3] cxl/mem: Support sanitation commands

[Davidlohr Bueso]

On Tue, 06 Dec 2022, Dan Williams wrote:

>> +static ssize_t security_store(struct device *dev,
>> +			      struct device_attribute *attr,
>> +			      const char *buf, size_t len)
>> +{
>> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	char cmd[CXL_SEC_CMD_SIZE+1];
>> +	ssize_t rc;
>> +
>> +	rc = sscanf(buf, "%"__stringify(CXL_SEC_CMD_SIZE)"s", cmd);
>> +	if (rc < 1)
>> +		return -EINVAL;
>> +
>> +	if (sysfs_streq(cmd, "overwrite"))
>> +		rc = cxl_mem_sanitize(cxlds, CXL_MBOX_OP_SANITIZE);
>> +	else if (sysfs_streq(cmd, "erase"))
>> +		rc = cxl_mem_sanitize(cxlds, CXL_MBOX_OP_SECURE_ERASE);
>
>The above needs quite a bit more safety that the device is not currently
>actively decoding any HPA ranges.

I'll look into this, but overall I am relying on the fact that by hw, writes
won't have any effect and reads will return random data on all locations
corresponding to that device, sanitized yet or not.

>I realize now after sending the comments in patch1 that Sanitize defeats
>the dream of being able to timeslice background operations. On the other
>hand, it really is a special case that is only meant to be run before
>decommissioning the device. I think it is fine that it monopolizes the
>device because the device is literally unusable during the process.
>
>> +	else
>> +		rc = -EINVAL;
>> +
>> +	if (rc == 0)
>> +		rc = len;
>> +	return rc;
>> +}
>> +static DEVICE_ATTR_RW(security);
>> +
>>  static struct attribute *cxl_memdev_attributes[] = {
>>  	&dev_attr_serial.attr,
>>  	&dev_attr_firmware_version.attr,
>>  	&dev_attr_payload_max.attr,
>>  	&dev_attr_label_storage_size.attr,
>>  	&dev_attr_numa_node.attr,
>> +	&dev_attr_security.attr,
>
>This likely wants is_visible() handling to hide the attribute if the
>device does not support security operations.

Can is_visible() work on single attribute? I was only able to get it
for an attribute group. I had also considered ifdefing the thing based
on ARCH_HAS_CPU_CACHE_INVALIDATE_MEMREGION but ended up not caring
if security file was shown as we have the remap scenario you pointed
out, so hiding security features became less appealing.

>
>>  	NULL,
>>  };
>>
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index e7cb2f2fadc4..1dd1caabd41c 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -215,6 +215,7 @@ struct cxl_endpoint_dvsec_info {
>>   * @mbox_irq: @dev supports mailbox interrupts
>>   * @mbox_bg: opcode for the in-flight background operation on @dev
>>   * @mbox_bgpoll: self-polling delayed work item
>> + * @sec: device security
>>   *
>>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
>>   * details on capacity parameters.
>> @@ -254,6 +255,10 @@ struct cxl_dev_state {
>>  	bool mbox_irq;
>>  	atomic_t mbox_bg;
>>  	struct delayed_work __maybe_unused mbox_bgpoll;
>> +
>> +	struct {
>> +		unsigned int __maybe_unused sanitize_tmo;
>> +	} sec;
>
>I missed this on the last patch, but __maybe_unused has no effect at
>declaration time, only at definition time. Otherwise, I wish I had a
>facility that would tell me which of my structure members are no longer
>used in code.

heh yeah, I mostly put them regardless as it nicely documents the
irq vs polling requirements. I'll remove.

Thanks,
Davidlohr

Re: [PATCH 2/3] cxl/mem: Support sanitation commands

[Dan Williams]

Davidlohr Bueso wrote:
> On Tue, 06 Dec 2022, Dan Williams wrote:
> 
> >> +static ssize_t security_store(struct device *dev,
> >> +			      struct device_attribute *attr,
> >> +			      const char *buf, size_t len)
> >> +{
> >> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> >> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> >> +	char cmd[CXL_SEC_CMD_SIZE+1];
> >> +	ssize_t rc;
> >> +
> >> +	rc = sscanf(buf, "%"__stringify(CXL_SEC_CMD_SIZE)"s", cmd);
> >> +	if (rc < 1)
> >> +		return -EINVAL;
> >> +
> >> +	if (sysfs_streq(cmd, "overwrite"))
> >> +		rc = cxl_mem_sanitize(cxlds, CXL_MBOX_OP_SANITIZE);
> >> +	else if (sysfs_streq(cmd, "erase"))
> >> +		rc = cxl_mem_sanitize(cxlds, CXL_MBOX_OP_SECURE_ERASE);
> >
> >The above needs quite a bit more safety that the device is not currently
> >actively decoding any HPA ranges.
> 
> I'll look into this, but overall I am relying on the fact that by hw, writes
> won't have any effect and reads will return random data on all locations
> corresponding to that device, sanitized yet or not.
> 
> >I realize now after sending the comments in patch1 that Sanitize defeats
> >the dream of being able to timeslice background operations. On the other
> >hand, it really is a special case that is only meant to be run before
> >decommissioning the device. I think it is fine that it monopolizes the
> >device because the device is literally unusable during the process.
> >
> >> +	else
> >> +		rc = -EINVAL;
> >> +
> >> +	if (rc == 0)
> >> +		rc = len;
> >> +	return rc;
> >> +}
> >> +static DEVICE_ATTR_RW(security);
> >> +
> >>  static struct attribute *cxl_memdev_attributes[] = {
> >>  	&dev_attr_serial.attr,
> >>  	&dev_attr_firmware_version.attr,
> >>  	&dev_attr_payload_max.attr,
> >>  	&dev_attr_label_storage_size.attr,
> >>  	&dev_attr_numa_node.attr,
> >> +	&dev_attr_security.attr,
> >
> >This likely wants is_visible() handling to hide the attribute if the
> >device does not support security operations.
> 
> Can is_visible() work on single attribute? I was only able to get it
> for an attribute group. I had also considered ifdefing the thing based
> on ARCH_HAS_CPU_CACHE_INVALIDATE_MEMREGION but ended up not caring
> if security file was shown as we have the remap scenario you pointed
> out, so hiding security features became less appealing.

is_visible() is a callback for the group, but it is called for each
attribute as an argument. So you can do things like:

    if (a == &dev_attr_security.attr && !haz_security_commands)
       return 0;

Re: [PATCH 2/3] cxl/mem: Support sanitation commands

[Jonathan Cameron]

On Mon,  5 Dec 2022 17:15:00 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Implement support for the non-pmem exclusive sanitize (aka overwrite)
> and secure erase commands, per CXL specs.
> 
> To properly support this feature, create a 'security' sysfs file that
> when read will list the current pmem security state or overwrite, and
> when written to, perform the requested operation.
> 
> As with ndctl-speak, the use cases here would be:
> 
> $> cxl sanitize --erase memX
> $> cxl sanitize --overwrite memX
> $> cxl sanitize --wait-overwrite memX  
> 
> Where userspace can implement entirely the wait/query mechanism for
> waiting for the sanitize to complete (albeit no poll support for
> the security sysfs file).
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Hi Davidlohr,

Given I'm late to the game and there has been lots of discussion I'll
focus on just the ABI.

> +What:		/sys/bus/cxl/devices/memX/security
> +Date:		December, 2022
> +KernelVersion:	v6.2
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		Reading this file will display the security state for that
> +		device. The following states are available: disabled, frozen,
> +		locked, unlocked and overwrite. When writing to the file, the
> +		following commands are supported:
> +		* overwrite - Sanitize the device to securely re-purpose or
> +		  decommission it. This is done by ensuring that all user data
> +		  and meta-data, whether it resides in persistent capacity,
> +		  volatile capacity, or the label storage area, is made
> +		  permanently unavailable by whatever means is appropriate for
> +		  the media type. This causes all CPU caches to be flushed.
> +		* erase - Secure Erase user data by changing the media encryption
> +		  keys for all user data areas of the device. This causes all
> +		  CPU caches to be flushed.

General rule of sysfs is one file, one thing.  I think this interface needs splitting.
RO attribute security_state
WO attribute security_overwrite (or maybe security_sanitize as overwriting is an
  implementation choice?)
WO attribute security_erase

Jonathan

Re: [PATCH 2/3] cxl/mem: Support sanitation commands

[Davidlohr Bueso]

On Mon, 19 Dec 2022, Jonathan Cameron wrote:

>On Mon,  5 Dec 2022 17:15:00 -0800
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> Implement support for the non-pmem exclusive sanitize (aka overwrite)
>> and secure erase commands, per CXL specs.
>>
>> To properly support this feature, create a 'security' sysfs file that
>> when read will list the current pmem security state or overwrite, and
>> when written to, perform the requested operation.
>>
>> As with ndctl-speak, the use cases here would be:
>>
>> $> cxl sanitize --erase memX
>> $> cxl sanitize --overwrite memX
>> $> cxl sanitize --wait-overwrite memX
>>
>> Where userspace can implement entirely the wait/query mechanism for
>> waiting for the sanitize to complete (albeit no poll support for
>> the security sysfs file).
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>
>Hi Davidlohr,
>
>Given I'm late to the game and there has been lots of discussion I'll
>focus on just the ABI.
>
>> +What:		/sys/bus/cxl/devices/memX/security
>> +Date:		December, 2022
>> +KernelVersion:	v6.2
>> +Contact:	linux-cxl@vger.kernel.org
>> +Description:
>> +		Reading this file will display the security state for that
>> +		device. The following states are available: disabled, frozen,
>> +		locked, unlocked and overwrite. When writing to the file, the
>> +		following commands are supported:
>> +		* overwrite - Sanitize the device to securely re-purpose or
>> +		  decommission it. This is done by ensuring that all user data
>> +		  and meta-data, whether it resides in persistent capacity,
>> +		  volatile capacity, or the label storage area, is made
>> +		  permanently unavailable by whatever means is appropriate for
>> +		  the media type. This causes all CPU caches to be flushed.
>> +		* erase - Secure Erase user data by changing the media encryption
>> +		  keys for all user data areas of the device. This causes all
>> +		  CPU caches to be flushed.
>
>General rule of sysfs is one file, one thing.  I think this interface needs splitting.
>RO attribute security_state
>WO attribute security_overwrite (or maybe security_sanitize as overwriting is an
>  implementation choice?)
>WO attribute security_erase

Fine by me. How about instead: security/{state, sanitize, erase}?

Re: [PATCH 2/3] cxl/mem: Support sanitation commands

[Jonathan Cameron]

On Mon, 19 Dec 2022 12:47:44 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Mon, 19 Dec 2022, Jonathan Cameron wrote:
> 
> >On Mon,  5 Dec 2022 17:15:00 -0800
> >Davidlohr Bueso <dave@stgolabs.net> wrote:
> >  
> >> Implement support for the non-pmem exclusive sanitize (aka overwrite)
> >> and secure erase commands, per CXL specs.
> >>
> >> To properly support this feature, create a 'security' sysfs file that
> >> when read will list the current pmem security state or overwrite, and
> >> when written to, perform the requested operation.
> >>
> >> As with ndctl-speak, the use cases here would be:
> >>  
> >> $> cxl sanitize --erase memX
> >> $> cxl sanitize --overwrite memX
> >> $> cxl sanitize --wait-overwrite memX  
> >>
> >> Where userspace can implement entirely the wait/query mechanism for
> >> waiting for the sanitize to complete (albeit no poll support for
> >> the security sysfs file).
> >>
> >> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>  
> >
> >Hi Davidlohr,
> >
> >Given I'm late to the game and there has been lots of discussion I'll
> >focus on just the ABI.
> >  
> >> +What:		/sys/bus/cxl/devices/memX/security
> >> +Date:		December, 2022
> >> +KernelVersion:	v6.2
> >> +Contact:	linux-cxl@vger.kernel.org
> >> +Description:
> >> +		Reading this file will display the security state for that
> >> +		device. The following states are available: disabled, frozen,
> >> +		locked, unlocked and overwrite. When writing to the file, the
> >> +		following commands are supported:
> >> +		* overwrite - Sanitize the device to securely re-purpose or
> >> +		  decommission it. This is done by ensuring that all user data
> >> +		  and meta-data, whether it resides in persistent capacity,
> >> +		  volatile capacity, or the label storage area, is made
> >> +		  permanently unavailable by whatever means is appropriate for
> >> +		  the media type. This causes all CPU caches to be flushed.
> >> +		* erase - Secure Erase user data by changing the media encryption
> >> +		  keys for all user data areas of the device. This causes all
> >> +		  CPU caches to be flushed.  
> >
> >General rule of sysfs is one file, one thing.  I think this interface needs splitting.
> >RO attribute security_state
> >WO attribute security_overwrite (or maybe security_sanitize as overwriting is an
> >  implementation choice?)
> >WO attribute security_erase  
> 
> Fine by me. How about instead: security/{state, sanitize, erase}?

Sounds good.

Jonathan

[PATCH 3/3] tools/testing/cxl: Add "Secure Erase" opcode support

[Davidlohr Bueso]

Add support to emulate a CXL mem device support the "Secure Erase"
operation.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 tools/testing/cxl/test/mem.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 35d9ad04e0d6..0f3101f65901 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -149,6 +149,30 @@ static int mock_partition_info(struct cxl_dev_state *cxlds,
 	return 0;
 }
 
+static int mock_secure_erase(struct cxl_dev_state *cxlds,
+			     struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mockmem_data *mdata = dev_get_platdata(cxlds->dev);
+
+	if (cmd->size_in != 0)
+		return -EINVAL;
+
+	if (cmd->size_out != 0)
+		return -EINVAL;
+
+	if (mdata->security_state & CXL_PMEM_SEC_STATE_USER_PASS_SET) {
+		cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
+		return -ENXIO;
+	}
+
+	if (mdata->security_state & CXL_PMEM_SEC_STATE_LOCKED) {
+		cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
 static int mock_get_security_state(struct cxl_dev_state *cxlds,
 				   struct cxl_mbox_cmd *cmd)
 {
@@ -567,6 +591,9 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
 	case CXL_MBOX_OP_GET_HEALTH_INFO:
 		rc = mock_health_info(cxlds, cmd);
 		break;
+	case CXL_MBOX_OP_SECURE_ERASE:
+		rc = mock_secure_erase(cxlds, cmd);
+		break;
 	case CXL_MBOX_OP_GET_SECURITY_STATE:
 		rc = mock_get_security_state(cxlds, cmd);
 		break;
-- 
2.38.1

RE: [PATCH 3/3] tools/testing/cxl: Add "Secure Erase" opcode support

[Dan Williams]

Davidlohr Bueso wrote:
> Add support to emulate a CXL mem device support the "Secure Erase"
> operation.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  tools/testing/cxl/test/mem.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 35d9ad04e0d6..0f3101f65901 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -149,6 +149,30 @@ static int mock_partition_info(struct cxl_dev_state *cxlds,
>  	return 0;
>  }
>  
> +static int mock_secure_erase(struct cxl_dev_state *cxlds,
> +			     struct cxl_mbox_cmd *cmd)
> +{
> +	struct cxl_mockmem_data *mdata = dev_get_platdata(cxlds->dev);
> +
> +	if (cmd->size_in != 0)
> +		return -EINVAL;
> +
> +	if (cmd->size_out != 0)
> +		return -EINVAL;
> +
> +	if (mdata->security_state & CXL_PMEM_SEC_STATE_USER_PASS_SET) {
> +		cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
> +		return -ENXIO;
> +	}
> +
> +	if (mdata->security_state & CXL_PMEM_SEC_STATE_LOCKED) {
> +		cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
> +		return -ENXIO;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mock_get_security_state(struct cxl_dev_state *cxlds,
>  				   struct cxl_mbox_cmd *cmd)
>  {
> @@ -567,6 +591,9 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
>  	case CXL_MBOX_OP_GET_HEALTH_INFO:
>  		rc = mock_health_info(cxlds, cmd);
>  		break;
> +	case CXL_MBOX_OP_SECURE_ERASE:
> +		rc = mock_secure_erase(cxlds, cmd);
> +		break;

Would be nice to have a sanitize mock that goes to sleep for 10 seconds
to test how command queueing is handled.

>  	case CXL_MBOX_OP_GET_SECURITY_STATE:
>  		rc = mock_get_security_state(cxlds, cmd);
>  		break;
> -- 
> 2.38.1
> 

RE: [PATCH v2 0/3] cxl: BG operations and device sanitation

[Dan Williams]

Davidlohr Bueso wrote:
> Hi,
> 
> This series adds support for the rest of the security related cxl operation
> which are not strictly dependent on PMEM.
> 
> Patch 1 adds the required background cmd handling bits. While this is currently
> only polling support, the idea is to follow up with an irq support once the
> bits land in Ira's event work. All polling-specific code has been marked
> as __maybe_unused.

I will take a look, but this tends to put me in the mindset of this is
v6.3 material that should wait for its user. Was that your intent?

> Patch 2 implements the sanitation commands (overwrite and secure erase).

My concern with landing non-PMEM secure erase is that it is not scoped
by partition type, and there is no existing appartus like there is on
the nvdimm side to make sure an erase event does not collide with a
device in active use.

However, this is just my reaction to the cover letter, maybe you covered
that aspect in the patches.

> Patch 3 adds a support for secure erase mocking.
> 
> As for testing, while I used the mock device to test the secure erase command, I
> ended up hacking up a prototype[0] for qemu to support overwrite and bg commands.
> So while the some of the paths this series introduces have been exercised, there
> is of course a lot more to do.
> 
> Applies on top of the 'for-6.2/cxl-security' in cxl.git.
> 
> Changes from rfc (https://lore.kernel.org/linux-cxl/20220804045029.2905056-1-dave@stgolabs.net/):
> 
> * Patch 1:
>  - Added a cxlds->mbox_irq flag in order to separate the polling-specific requirements.
>  - Made bg workqueue concurrent.
>  - Address iteration mem commands style (Jonathan). Now just rely on cxl_mem_find_command + check
>    bg flag + callback.
>  - Don't error in cxl_mbox_check_cmd_bgcmd() if conflicts() callback is not defined (Jonathan).
>  - Error out in cxl_mbox_send_cmd() upon corruption in cxl_mbox_bgcmd_begin() (Jonathan).
>  - Do not prematurely add all bg capable opcodes (Jonathan).
> 
> * Patch 2:
>   - Rework polling delay hinting (Dave).
>   - Rely on CEL to see if command is supported instead of unconditionally sending it (Jonathan).
> 
> * Added patch 3.
> 
> [0]: https://github.com/davidlohr/qemu/commit/64a93a5b824b59d3b547f06f7fbb1269fb4790ce
> 
> Thanks!
> 
> Davidlohr Bueso (3):
>   cxl/mbox: Add background operation handling machinery
>   cxl/mem: Support sanitation commands
>   tools/testing/cxl: Add "Secure Erase" opcode support
> 
>  Documentation/ABI/testing/sysfs-bus-cxl |  19 ++
>  drivers/cxl/core/core.h                 |   3 +-
>  drivers/cxl/core/mbox.c                 | 317 +++++++++++++++++++++++-
>  drivers/cxl/core/memdev.c               |  54 ++++
>  drivers/cxl/core/port.c                 |   1 +
>  drivers/cxl/cxl.h                       |   8 +
>  drivers/cxl/cxlmem.h                    |  65 ++++-
>  drivers/cxl/pci.c                       |   4 +
>  drivers/cxl/pmem.c                      |   5 +-
>  drivers/cxl/security.c                  |  13 +-
>  include/uapi/linux/cxl_mem.h            |   2 +
>  tools/testing/cxl/test/mem.c            |  27 ++
>  12 files changed, 497 insertions(+), 21 deletions(-)
> 
> --
> 2.38.1
> 

Re: [PATCH v2 0/3] cxl: BG operations and device sanitation

[Davidlohr Bueso]

On Tue, 06 Dec 2022, Dan Williams wrote:

>Davidlohr Bueso wrote:
>> Hi,
>>
>> This series adds support for the rest of the security related cxl operation
>> which are not strictly dependent on PMEM.
>>
>> Patch 1 adds the required background cmd handling bits. While this is currently
>> only polling support, the idea is to follow up with an irq support once the
>> bits land in Ira's event work. All polling-specific code has been marked
>> as __maybe_unused.
>
>I will take a look, but this tends to put me in the mindset of this is
>v6.3 material that should wait for its user. Was that your intent?

Thanks for having a look. My intent was for 6.3, yes.

>
>> Patch 2 implements the sanitation commands (overwrite and secure erase).
>
>My concern with landing non-PMEM secure erase is that it is not scoped
>by partition type, and there is no existing appartus like there is on
>the nvdimm side to make sure an erase event does not collide with a
>device in active use.

I can drop the secure erase part of patch 2 and just leave overwrite.

Thanks,
Davidlohr

Re: [PATCH v2 0/3] cxl: BG operations and device sanitation

[Dan Williams]

Davidlohr Bueso wrote:
> On Tue, 06 Dec 2022, Dan Williams wrote:
> 
> >Davidlohr Bueso wrote:
> >> Hi,
> >>
> >> This series adds support for the rest of the security related cxl operation
> >> which are not strictly dependent on PMEM.
> >>
> >> Patch 1 adds the required background cmd handling bits. While this is currently
> >> only polling support, the idea is to follow up with an irq support once the
> >> bits land in Ira's event work. All polling-specific code has been marked
> >> as __maybe_unused.
> >
> >I will take a look, but this tends to put me in the mindset of this is
> >v6.3 material that should wait for its user. Was that your intent?
> 
> Thanks for having a look. My intent was for 6.3, yes.
> 
> >
> >> Patch 2 implements the sanitation commands (overwrite and secure erase).
> >
> >My concern with landing non-PMEM secure erase is that it is not scoped
> >by partition type, and there is no existing appartus like there is on
> >the nvdimm side to make sure an erase event does not collide with a
> >device in active use.
> 
> I can drop the secure erase part of patch 2 and just leave overwrite.

They both have the same problem, they need extra safety to make
sure that the device is not in active usage during the operation.