[RFC PATCH 0/2] cxl: BG operations and device sanitation

[RFC PATCH 0/2] cxl: BG operations and device sanitation

[Davidlohr Bueso]

Hello,

The following is a followup to some of the discussions around CXL device security
and sanitation[0, 1]. It is marked as RFC mostly to see if the background handling
will satisfy all users, not just sanitize/overwrite. For example there has been ideas
to avoid command hogging the device and/or interleaving scan media regions instead
of all in one, etc. More details in each patch, but:

Patch 1 adds the required background cmd handling bits. While this is currently
only polling, it would be good to know if there are any fundamental blockers for
supporting irqs (beyond just background ops) between PCIe and CXL. For example,
are there any requirements/difficulties that is not the standard MSI/MSI-X PCI
vector allocation + devm_request_irq()? I have not looked at this into details but
it the topic has come up in the past as delicate', iirc.

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

As for testing, while I used the mock device to test the secure erase command, I
ended up hacking up a prototype[2] 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 against Dave's cxl-security branch[2] which deals with the pmem-only bits.

Thanks,
Davidlohr

[0]: https://lore.kernel.org/all/20220708172455.gi37dh3od4w5lqrd@offworld/ 
[1]: https://lore.kernel.org/all/165791918718.2491387.4203738301057301285.stgit@djiang5-desk3.ch.intel.com/
[2]: https://github.com/davidlohr/qemu/commit/64a93a5b824b59d3b547f06f7fbb1269fb4790ce
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-security

Davidlohr Bueso (2):
  cxl/mbox: Add background operation handling machinery
  cxl/mem: Support sanitation commands

 Documentation/ABI/testing/sysfs-bus-cxl |  19 ++
 drivers/cxl/core/core.h                 |   2 +-
 drivers/cxl/core/mbox.c                 | 304 +++++++++++++++++++++++-
 drivers/cxl/core/memdev.c               |  58 +++++
 drivers/cxl/core/port.c                 |   9 +-
 drivers/cxl/cxl.h                       |   8 +
 drivers/cxl/cxlmem.h                    |  65 ++++-
 drivers/cxl/pci.c                       |   3 +-
 drivers/cxl/pmem.c                      |   5 +-
 drivers/cxl/security.c                  |  13 +-
 include/uapi/linux/cxl_mem.h            |   2 +
 11 files changed, 461 insertions(+), 27 deletions(-)

--
2.37.1

Re: [RFC PATCH 0/2] cxl: BG operations and device sanitation

[Davidlohr Bueso]

*sigh* Cc Dave.

On Wed, 03 Aug 2022, Davidlohr Bueso wrote:

>Hello,
>
>The following is a followup to some of the discussions around CXL device security
>and sanitation[0, 1]. It is marked as RFC mostly to see if the background handling
>will satisfy all users, not just sanitize/overwrite. For example there has been ideas
>to avoid command hogging the device and/or interleaving scan media regions instead
>of all in one, etc. More details in each patch, but:
>
>Patch 1 adds the required background cmd handling bits. While this is currently
>only polling, it would be good to know if there are any fundamental blockers for
>supporting irqs (beyond just background ops) between PCIe and CXL. For example,
>are there any requirements/difficulties that is not the standard MSI/MSI-X PCI
>vector allocation + devm_request_irq()? I have not looked at this into details but
>it the topic has come up in the past as delicate', iirc.
>
>Patch 2 implements the sanitation commands (overwrite and secure erase).
>
>As for testing, while I used the mock device to test the secure erase command, I
>ended up hacking up a prototype[2] 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 against Dave's cxl-security branch[2] which deals with the pmem-only bits.
>
>Thanks,
>Davidlohr
>
>[0]: https://lore.kernel.org/all/20220708172455.gi37dh3od4w5lqrd@offworld/
>[1]: https://lore.kernel.org/all/165791918718.2491387.4203738301057301285.stgit@djiang5-desk3.ch.intel.com/
>[2]: https://github.com/davidlohr/qemu/commit/64a93a5b824b59d3b547f06f7fbb1269fb4790ce
>[3]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-security
>
>Davidlohr Bueso (2):
>  cxl/mbox: Add background operation handling machinery
>  cxl/mem: Support sanitation commands
>
> Documentation/ABI/testing/sysfs-bus-cxl |  19 ++
> drivers/cxl/core/core.h                 |   2 +-
> drivers/cxl/core/mbox.c                 | 304 +++++++++++++++++++++++-
> drivers/cxl/core/memdev.c               |  58 +++++
> drivers/cxl/core/port.c                 |   9 +-
> drivers/cxl/cxl.h                       |   8 +
> drivers/cxl/cxlmem.h                    |  65 ++++-
> drivers/cxl/pci.c                       |   3 +-
> drivers/cxl/pmem.c                      |   5 +-
> drivers/cxl/security.c                  |  13 +-
> include/uapi/linux/cxl_mem.h            |   2 +
> 11 files changed, 461 insertions(+), 27 deletions(-)
>
>--
>2.37.1
>

[PATCH 1/2] 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.

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 |   2 +-
 drivers/cxl/core/mbox.c | 199 ++++++++++++++++++++++++++++++++++++++--
 drivers/cxl/core/port.c |   9 +-
 drivers/cxl/cxl.h       |   8 ++
 drivers/cxl/cxlmem.h    |  62 ++++++++++++-
 drivers/cxl/pci.c       |   3 +-
 drivers/cxl/pmem.c      |   5 +-
 drivers/cxl/security.c  |  13 +--
 8 files changed, 274 insertions(+), 27 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 1a50c0fc399c..ffc625fdb795 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -19,7 +19,7 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
 
 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 f162743e598d..db6e5a4d1f6d 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_bgcmd_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,130 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
 	return cxl_command_names[c->info.id].name;
 }
 
+unsigned long cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode)
+{
+	struct cxl_mem_command *c;
+	unsigned long ret = 0;
+
+	cxl_for_each_cmd(c) {
+		if (!(c->flags & CXL_CMD_FLAG_BACKGROUND))
+			continue;
+		if (opcode != c->opcode)
+			continue;
+
+		if (c->bgops && c->bgops->delay)
+			ret = c->bgops->delay(cxlds);
+		break;
+	}
+	return ret * HZ;
+}
+
+void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds,
+			 u16 opcode, bool success)
+{
+	struct cxl_mem_command *c;
+
+	cxl_for_each_cmd(c) {
+		if (!(c->flags & CXL_CMD_FLAG_BACKGROUND))
+			continue;
+		if (opcode != c->opcode)
+			continue;
+
+		if (c->bgops && c->bgops->post)
+			c->bgops->post(cxlds, success);
+		break;
+	}
+}
+
+/*
+ * Ensure that ->mbox_send() can run safely when a background
+ * command is running. If so, returns zero, otherwise -EBUSY.
+ *
+ * check 1. bg cmd running. In most cases we can just be on
+ *          our merry way.
+ * check 2. @new incoming command is not 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 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;
+
+	cxl_for_each_cmd(c) {
+		if (new != c->opcode)
+			continue;
+
+		/* 2 */
+		if (c->flags & CXL_CMD_FLAG_BACKGROUND)
+			break;
+		/* 3 */
+		if (c->bgops && c->bgops->conflicts)
+			return c->bgops->conflicts(new);
+		break;
+	}
+
+	return -EBUSY;
+}
+
+/*
+ * Background operation polling mode.
+ */
+static 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, bg_dwork.work);
+
+	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_bgcmd_wq, &cxlds->bg_dwork, 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);
+	}
+}
+
 /**
  * cxl_mbox_send_cmd() - Send a mailbox command to a device.
  * @cxlds: The device data for the operation
@@ -153,6 +283,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: (output) HW returned code.
  *
  * Context: Any context.
  * Return:
@@ -167,8 +298,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 +315,46 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
 	if (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.
+	 */
+	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);
+		}
+
+		get_device(cxlds->dev);
+		/* do first poll check asap before using any hinting */
+		queue_delayed_work(cxl_mbox_bgcmd_wq, &cxlds->bg_dwork, 0);
+		fallthrough;
+	}
+	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
@@ -198,6 +364,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
 		if (mbox_cmd.size_out != out_size)
 			return -EIO;
 	}
+
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mbox_send_cmd, CXL);
@@ -575,7 +742,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 +795,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);
@@ -733,7 +900,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;
@@ -766,7 +933,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;
 
@@ -845,6 +1012,9 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
 	}
 
 	mutex_init(&cxlds->mbox_mutex);
+	INIT_DELAYED_WORK(&cxlds->bg_dwork, cxl_mbox_bgcmd_work);
+	atomic_set(&cxlds->bg, 0);
+
 	cxlds->dev = dev;
 
 	return cxlds;
@@ -853,7 +1023,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_dev_state_create, CXL);
 
 static struct dentry *cxl_debugfs;
 
-void __init cxl_mbox_init(void)
+int __init cxl_mbox_init(void)
 {
 	struct dentry *mbox_debugfs;
 
@@ -861,9 +1031,18 @@ void __init cxl_mbox_init(void)
 	mbox_debugfs = debugfs_create_dir("mbox", cxl_debugfs);
 	debugfs_create_bool("raw_allow_all", 0600, mbox_debugfs,
 			    &cxl_raw_allow_all);
+
+	cxl_mbox_bgcmd_wq = alloc_ordered_workqueue("cxl_mbox_bgcmd", 0);
+	if (!cxl_mbox_bgcmd_wq) {
+		debugfs_remove_recursive(cxl_debugfs);
+		return -ENOMEM;
+	}
+
+	return 0;
 }
 
 void cxl_mbox_exit(void)
 {
 	debugfs_remove_recursive(cxl_debugfs);
+	destroy_workqueue(cxl_mbox_bgcmd_wq);
 }
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index dbce99bdffab..1350b99e7287 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -104,7 +104,7 @@ static ssize_t name##_show(struct device *dev,                       \
 			   struct device_attribute *attr, char *buf) \
 {                                                                    \
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);              \
-                                                                     \
+								     \
 	return sysfs_emit(buf, "%s\n",                               \
 			  (cxld->flags & (flag)) ? "1" : "0");       \
 }                                                                    \
@@ -1525,11 +1525,13 @@ static __init int cxl_core_init(void)
 {
 	int rc;
 
-	cxl_mbox_init();
+	rc = cxl_mbox_init();
+	if (rc)
+		return rc;
 
 	rc = cxl_memdev_init();
 	if (rc)
-		return rc;
+		goto err_mbox;
 
 	cxl_bus_wq = alloc_ordered_workqueue("cxl_port", 0);
 	if (!cxl_bus_wq) {
@@ -1547,6 +1549,7 @@ static __init int cxl_core_init(void)
 	destroy_workqueue(cxl_bus_wq);
 err_wq:
 	cxl_memdev_exit();
+err_mbox:
 	cxl_mbox_exit();
 	return rc;
 }
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 6799b27c7db2..5eca9dc7397b 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -80,14 +80,22 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr)
 /* 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 4bcb02f625b4..c05dc1b8189a 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -192,6 +192,8 @@ struct cxl_endpoint_dvsec_info {
  * @info: Cached DVSEC information about the device.
  * @serial: PCIe Device Serial Number
  * @mbox_send: @dev specific transport for transmitting mailbox commands
+ * @bg: opcode for the in-flight background operation on @dev
+ * @bg_dwork: self-polling work item
  *
  * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
  * details on capacity parameters.
@@ -225,12 +227,15 @@ struct cxl_dev_state {
 	u64 serial;
 
 	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
+	atomic_t bg;
+	struct delayed_work bg_dwork;
 };
 
 enum cxl_opcode {
 	CXL_MBOX_OP_INVALID		= 0x0000,
 	CXL_MBOX_OP_RAW			= CXL_MBOX_OP_INVALID,
 	CXL_MBOX_OP_GET_FW_INFO		= 0x0200,
+	CXL_MBOX_OP_TRANSFER_FW		= 0x0201,
 	CXL_MBOX_OP_ACTIVATE_FW		= 0x0202,
 	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
 	CXL_MBOX_OP_GET_LOG		= 0x0401,
@@ -250,17 +255,46 @@ 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_GET_SECURITY_STATE	= 0x4500,
 	CXL_MBOX_OP_SET_PASSPHRASE	= 0x4501,
 	CXL_MBOX_OP_DISABLE_PASSPHRASE	= 0x4502,
 	CXL_MBOX_OP_UNLOCK		= 0x4503,
 	CXL_MBOX_OP_FREEZE_SECURITY	= 0x4504,
 	CXL_MBOX_OP_PASSPHRASE_ERASE	= 0x4505,
+	CXL_MBOX_OP_BIND_VPPB		= 0x5201,
+	CXL_MBOX_OP_UNBIND_VPPB		= 0x5202,
 	CXL_MBOX_OP_MAX			= 0x10000
 };
 
-#define DEFINE_CXL_CEL_UUID                                                    \
-	UUID_INIT(0xda9c0b5, 0xbf41, 0x4b78, 0x8f, 0x79, 0x96, 0xb1, 0x62,     \
+/*
+ * CXL 3.0 supported commands that can run in the background.
+ *
+ * Commands that require a longer execution time shall be
+ * completed asynchronously in the background. At a hw level
+ * only one command can be executed in the background at a
+ * time, and therefore additional background commands shall
+ * be rejected with the busy return code.
+ *
+ * Barriers pair with release/acquire semantics. When done,
+ * clearing ->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->bg, opcode);
+}
+static inline int cxl_mbox_bgcmd_running(struct cxl_dev_state *cxlds)
+{
+	return atomic_read_acquire(&cxlds->bg);
+}
+static inline void cxl_mbox_bgcmd_done(struct cxl_dev_state *cxlds)
+{
+	atomic_set_release(&cxlds->bg, 0);
+}
+
+#define DEFINE_CXL_CEL_UUID						\
+	UUID_INIT(0xda9c0b5, 0xbf41, 0x4b78, 0x8f, 0x79, 0x96, 0xb1, 0x62, \
 		  0x3b, 0x3f, 0x17)
 
 #define DEFINE_CXL_VENDOR_DEBUG_UUID                                           \
@@ -323,16 +357,32 @@ struct cxl_mbox_set_partition_info {
 
 #define  CXL_SET_PARTITION_IMMEDIATE_FLAG	BIT(0)
 
+/**
+ * struct cxl_mbox_bgcmd_ops - Ad-hoc handling of background cmds
+ * @conflicts: How to handle a @new command when one is currently executing
+ * @post: Execute after the command completes
+ * @delay: Delay hint for polling on command completion
+ */
+struct cxl_mem_bgcommand_ops {
+	int (*conflicts)(u16 new);
+	void (*post)(struct cxl_dev_state *cxlds, bool success);
+	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 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 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
@@ -344,8 +394,9 @@ struct cxl_mem_command {
 	struct cxl_command_info info;
 	enum cxl_opcode opcode;
 	u32 flags;
-#define CXL_CMD_FLAG_NONE 0
+	struct cxl_mem_bgcommand_ops *bgops;
 #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
+#define CXL_CMD_FLAG_BACKGROUND BIT(1)
 };
 
 #define CXL_PMEM_SEC_STATE_USER_PASS_SET	0x01
@@ -383,7 +434,10 @@ 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);
+void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds, u16 opcode, bool success);
+unsigned long cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode);
 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);
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 5a0ae46d4989..57ad19050c2c 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -176,7 +176,8 @@ 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_SUCCESS) {
+	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS &&
+	    mbox_cmd->return_code != CXL_MBOX_CMD_RC_BACKGROUND) {
 		dev_dbg(dev, "Mailbox operation had an error: %s\n",
 			cxl_mbox_cmd_rc2str(mbox_cmd));
 		return 0; /* completed but caller must check return_code */
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 1a85168bd2d1..63f51124bc24 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -156,7 +156,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;
@@ -188,7 +189,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 eb65c299c890..735cbe4d817a 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;
 
@@ -71,7 +71,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);
 	kfree(set_pass);
 	return rc;
 }
@@ -95,7 +95,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);
 	kfree(dis_pass);
 	return rc;
 }
@@ -118,7 +118,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,
@@ -132,7 +133,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;
 
@@ -161,7 +162,7 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
 	/* Flush all cache before we erase mem device */
 	flush_cache_all();
 	rc =  cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_ERASE,
-				erase, sizeof(*erase), NULL, 0);
+				erase, sizeof(*erase), NULL, 0, NULL);
 	kfree(erase);
 	if (rc < 0)
 		return rc;
-- 
2.37.1

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

[Jonathan Cameron]

On Wed,  3 Aug 2022 21:50:28 -0700
Davidlohr Bueso <dave@stgolabs.net> 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.
> 
> 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

I'd refer to sanitize rather than overwrite to keep aligned with spec.
It's not so much that command isn't allowed as it will return an error.
Is there any advantage in explicitly not sending the commands that will
error?  Either way the caller sees an error.


>   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.

Hmm. Is there a return code we could use for this rather than changing
the function signature?  I'm thinking similar long running commands
in Crypto return -EINPROGRESS to indicate a similar situation. 
> 
> - 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 |   2 +-
>  drivers/cxl/core/mbox.c | 199 ++++++++++++++++++++++++++++++++++++++--
>  drivers/cxl/core/port.c |   9 +-
>  drivers/cxl/cxl.h       |   8 ++
>  drivers/cxl/cxlmem.h    |  62 ++++++++++++-
>  drivers/cxl/pci.c       |   3 +-
>  drivers/cxl/pmem.c      |   5 +-
>  drivers/cxl/security.c  |  13 +--
>  8 files changed, 274 insertions(+), 27 deletions(-)
> 

...

> +unsigned long cxl_mbox_bgcmd_delay(struct cxl_dev_state *cxlds, u16 opcode)
> +{
> +	struct cxl_mem_command *c;
> +	unsigned long ret = 0;
> +
> +	cxl_for_each_cmd(c) {

If this gets called on a non background command might make sense to return an
error.  If so, check for opcode match first, then background if there
is a match and if that's not set error out.

		
> +		if (!(c->flags & CXL_CMD_FLAG_BACKGROUND))
> +			continue;
> +		if (opcode != c->opcode)
> +			continue;
> +
> +		if (c->bgops && c->bgops->delay)
> +			ret = c->bgops->delay(cxlds);
> +		break;
> +	}
> +	return ret * HZ;
> +}
> +
> +void cxl_mbox_bgcmd_post(struct cxl_dev_state *cxlds,
> +			 u16 opcode, bool success)
> +{
> +	struct cxl_mem_command *c;
> +
> +	cxl_for_each_cmd(c) {
> +		if (!(c->flags & CXL_CMD_FLAG_BACKGROUND))
> +			continue;
> +		if (opcode != c->opcode)
> +			continue;

We definitely shouldn't have duplicate entries, so I'd
check opcode match first, then if there is a match and
background not set error out.

> +
> +		if (c->bgops && c->bgops->post)
> +			c->bgops->post(cxlds, success);
> +		break;
> +	}
> +}
> +
> +/*
> + * Ensure that ->mbox_send() can run safely when a background
> + * command is running. If so, returns zero, otherwise -EBUSY.

As above, I'm not sure this is necessary given you should get
a sensible error code in response to any such message.
E.g. media disabled.

I'm not against defense in depth though if you want to add
this block list.  It's just strikes me as tricky to maintain
when new commands are added in future specs.  Perhaps better
to make it a problem for the hardware.
 
> + *
> + * check 1. bg cmd running. In most cases we can just be on
> + *          our merry way.
Comment is confusing. Perhaps
		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 not capable of running

is capable?

> + *          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 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;
> +
> +	cxl_for_each_cmd(c) {
> +		if (new != c->opcode)
> +			continue;

Feels like a cxl_find_cmd() might be useful...


> +
> +		/* 2 */
> +		if (c->flags & CXL_CMD_FLAG_BACKGROUND)
> +			break;

My preference would be a direct return.

> +		/* 3 */
> +		if (c->bgops && c->bgops->conflicts)
> +			return c->bgops->conflicts(new);
> +		break;
		If there is no conflicts() function I would
		think that we are fine to run it. So return 0?

> +	}
> +
> +	return -EBUSY;
> +}
> +
> +/*
> + * Background operation polling mode.
> + */
> +static 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, bg_dwork.work);
> +
> +	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_bgcmd_wq, &cxlds->bg_dwork, 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);
> +	}
> +}
> +
>  /**
>   * cxl_mbox_send_cmd() - Send a mailbox command to a device.
>   * @cxlds: The device data for the operation
> @@ -153,6 +283,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: (output) HW returned code.
>   *
>   * Context: Any context.
>   * Return:
> @@ -167,8 +298,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 +315,46 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>  	if (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.
> +	 */
> +	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) {

Why not return an error?  I'd have thought this was fairly fatal...

> +			dev_WARN(cxlds->dev,
> +				 "Corrupted background cmd (opcode 0x%04x)\n",
> +				 err);
> +		}
> +
> +		get_device(cxlds->dev);
> +		/* do first poll check asap before using any hinting */
> +		queue_delayed_work(cxl_mbox_bgcmd_wq, &cxlds->bg_dwork, 0);
> +		fallthrough;

falling through to a break seems unnecessary, just break; here probably
slightly easier to read.

> +	}
> +	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
> @@ -198,6 +364,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>  		if (mbox_cmd.size_out != out_size)
>  			return -EIO;
>  	}
> +
Unrelated change - drop it from this patch.
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mbox_send_cmd, CXL);
> @@ -575,7 +742,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 +795,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);
> @@ -733,7 +900,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;
> @@ -766,7 +933,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;
>  
> @@ -845,6 +1012,9 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
>  	}
>  
>  	mutex_init(&cxlds->mbox_mutex);
> +	INIT_DELAYED_WORK(&cxlds->bg_dwork, cxl_mbox_bgcmd_work);
> +	atomic_set(&cxlds->bg, 0);
> +
>  	cxlds->dev = dev;
>  
>  	return cxlds;
> @@ -853,7 +1023,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_dev_state_create, CXL);
>  
>  static struct dentry *cxl_debugfs;
>  


> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index dbce99bdffab..1350b99e7287 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -104,7 +104,7 @@ static ssize_t name##_show(struct device *dev,                       \
>  			   struct device_attribute *attr, char *buf) \
>  {                                                                    \
>  	struct cxl_decoder *cxld = to_cxl_decoder(dev);              \
> -                                                                     \
> +								     \
Unrelated change that shouldn't be in this patch.

>  	return sysfs_emit(buf, "%s\n",                               \
>  			  (cxld->flags & (flag)) ? "1" : "0");       \
>  }                                                                    \

...

> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 4bcb02f625b4..c05dc1b8189a 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -192,6 +192,8 @@ struct cxl_endpoint_dvsec_info {
>   * @info: Cached DVSEC information about the device.
>   * @serial: PCIe Device Serial Number
>   * @mbox_send: @dev specific transport for transmitting mailbox commands
> + * @bg: opcode for the in-flight background operation on @dev
> + * @bg_dwork: self-polling work item
>   *
>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
>   * details on capacity parameters.
> @@ -225,12 +227,15 @@ struct cxl_dev_state {
>  	u64 serial;
>  
>  	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> +	atomic_t bg;
> +	struct delayed_work bg_dwork;
>  };
>  
>  enum cxl_opcode {
>  	CXL_MBOX_OP_INVALID		= 0x0000,
>  	CXL_MBOX_OP_RAW			= CXL_MBOX_OP_INVALID,
>  	CXL_MBOX_OP_GET_FW_INFO		= 0x0200,
> +	CXL_MBOX_OP_TRANSFER_FW		= 0x0201,
>  	CXL_MBOX_OP_ACTIVATE_FW		= 0x0202,
>  	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
>  	CXL_MBOX_OP_GET_LOG		= 0x0401,
> @@ -250,17 +255,46 @@ 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,

Why introduce all these now?  Better to keep to introducing at first use.

>  	CXL_MBOX_OP_GET_SECURITY_STATE	= 0x4500,
>  	CXL_MBOX_OP_SET_PASSPHRASE	= 0x4501,
>  	CXL_MBOX_OP_DISABLE_PASSPHRASE	= 0x4502,
>  	CXL_MBOX_OP_UNLOCK		= 0x4503,
>  	CXL_MBOX_OP_FREEZE_SECURITY	= 0x4504,
>  	CXL_MBOX_OP_PASSPHRASE_ERASE	= 0x4505,
> +	CXL_MBOX_OP_BIND_VPPB		= 0x5201,
> +	CXL_MBOX_OP_UNBIND_VPPB		= 0x5202,
>  	CXL_MBOX_OP_MAX			= 0x10000
>  };
>  
> -#define DEFINE_CXL_CEL_UUID                                                    \
> -	UUID_INIT(0xda9c0b5, 0xbf41, 0x4b78, 0x8f, 0x79, 0x96, 0xb1, 0x62,     \
> +/*
> + * CXL 3.0 supported commands that can run in the background.

I'd drop the 3.0 reference as was true with CXL 2.0 as well and
it might confuse people with CXL 2.0 devices.

> + *
> + * Commands that require a longer execution time shall be
> + * completed asynchronously in the background. At a hw level
> + * only one command can be executed in the background at a
> + * time, and therefore additional background commands shall
> + * be rejected with the busy return code.
> + *
> + * Barriers pair with release/acquire semantics. When done,
> + * clearing ->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->bg, opcode);
> +}
> +static inline int cxl_mbox_bgcmd_running(struct cxl_dev_state *cxlds)
> +{
> +	return atomic_read_acquire(&cxlds->bg);
> +}
> +static inline void cxl_mbox_bgcmd_done(struct cxl_dev_state *cxlds)
> +{
> +	atomic_set_release(&cxlds->bg, 0);
> +}
> +
> +#define DEFINE_CXL_CEL_UUID						\
> +	UUID_INIT(0xda9c0b5, 0xbf41, 0x4b78, 0x8f, 0x79, 0x96, 0xb1, 0x62, \
>  		  0x3b, 0x3f, 0x17)
>  
>  #define DEFINE_CXL_VENDOR_DEBUG_UUID                                           \
> @@ -323,16 +357,32 @@ struct cxl_mbox_set_partition_info {
>  
>  #define  CXL_SET_PARTITION_IMMEDIATE_FLAG	BIT(0)
>  

>   * 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
> @@ -344,8 +394,9 @@ struct cxl_mem_command {
>  	struct cxl_command_info info;
>  	enum cxl_opcode opcode;
>  	u32 flags;
> -#define CXL_CMD_FLAG_NONE 0
Why remove this?  If not used should be removed in a different patch.

> +	struct cxl_mem_bgcommand_ops *bgops;
>  #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
> +#define CXL_CMD_FLAG_BACKGROUND BIT(1)
>  };
>  

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

[Davidlohr Bueso]

On Thu, 25 Aug 2022, Jonathan Cameron wrote:

>> +/*
>> + * Ensure that ->mbox_send() can run safely when a background
>> + * command is running. If so, returns zero, otherwise -EBUSY.
>
>As above, I'm not sure this is necessary given you should get
>a sensible error code in response to any such message.
>E.g. media disabled.

So this is necessary to prevent mischief in the polling case
where you can have windows where the driver is out of sync
with the hardware - hw finishes async command but driver
(sleeping poller) has not acknowledged this. In the irq case,
yeah this would be redundant and not needed.

Thanks,
Davidlohr

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

[Jonathan Cameron]

On Mon, 21 Nov 2022 13:48:04 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Thu, 25 Aug 2022, Jonathan Cameron wrote:
> 
> >> +/*
> >> + * Ensure that ->mbox_send() can run safely when a background
> >> + * command is running. If so, returns zero, otherwise -EBUSY.  
> >
> >As above, I'm not sure this is necessary given you should get
> >a sensible error code in response to any such message.
> >E.g. media disabled.  
> 
> So this is necessary to prevent mischief in the polling case
> where you can have windows where the driver is out of sync
> with the hardware - hw finishes async command but driver
> (sleeping poller) has not acknowledged this. In the irq case,
> yeah this would be redundant and not needed.
> 

Ok.  A worked example might be useful for where this matters
at a higher level.  For example, if there is no conflict at
the hardware level what goes wrong in the software as a result
of dropping the protection on the one that we haven't yet detected
as finished...

Jonathan


> Thanks,
> Davidlohr

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

[Davidlohr Bueso]

Implement support for the non-pmem exclusive sanitize (aka overwrite)
and secure erase commands, per CXL 3.0 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

While userspace can implement entirely the wait/query mechanism for
waiting for the sanitize to complete. Unlike persistent memory
equivalents, there is no command to query in CXL, and as such we can
safely just use cxlds->bg.

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

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 7c2b846521f3..88631b492a11 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -52,6 +52,25 @@ Description:
 		host PCI device for this memory device, emit the CPU node
 		affinity for this device.
 
+What:		/sys/bus/cxl/devices/memX/security
+Date:		August, 2022
+KernelVersion:	v5.21
+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 db6e5a4d1f6d..06bbb760b392 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -41,6 +41,8 @@ static struct workqueue_struct *cxl_mbox_bgcmd_wq;
 #define CXL_BGCMD(_id, sin, sout, _flags, _bgops)                              \
 	__CXL_CMD(_id, sin, sout, _flags | CXL_CMD_FLAG_BACKGROUND, _bgops)
 
+static struct cxl_mem_bgcommand_ops sanitize_bgops;
+
 #define CXL_VARIABLE_PAYLOAD	~0U
 /*
  * This table defines the supported mailbox commands for the driver. This table
@@ -71,6 +73,8 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
 	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
 	CXL_BGCMD(SCAN_MEDIA, 0x11, 0, 0, NULL),
 	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
+	CXL_BGCMD(SANITIZE, 0, 0, 0, &sanitize_bgops),
+	CXL_CMD(SECURE_ERASE, 0, 0, 0),
 	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
 	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
 	CXL_CMD(DISABLE_PASSPHRASE, 0x40, 0, 0),
@@ -962,6 +966,107 @@ 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 long secs;
+	u64 total_mem;
+
+	if (!cxlds)
+		return 0;
+
+	total_mem = cxlds->total_bytes / CXL_CAPACITY_MULTIPLIER;
+
+	if (total_mem <= 16)
+		secs = 1;
+	else if (total_mem <= 64)
+		secs = 10;
+	else if (total_mem <= 256)
+		secs = 20;
+	else if (total_mem <= 512)
+		secs = 40;
+	else if (total_mem <= 1024)
+		secs = 50;
+	else
+		secs = 60; /* max */
+	return secs;
+}
+
+static void sanitize_bgcmd_post(struct cxl_dev_state *cxlds, bool success)
+{
+	if (success)
+		flush_cache_all();
+}
+
+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
+ * @cmd: 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 cmd)
+{
+	int rc;
+	bool skip_security;
+	u32 sec_out;
+	u16 ret_code; /* hw */
+
+	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE,
+			       NULL, 0, &sec_out, sizeof(sec_out), &ret_code);
+	/* this may just be plain unsupported, do not error out */
+	skip_security = (ret_code == CXL_MBOX_CMD_RC_UNSUPPORTED);
+	if (rc && !skip_security)
+		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 (!skip_security && (sec_out & CXL_PMEM_SEC_STATE_USER_PASS_SET))
+		return -EINVAL;
+
+	if (cmd == CXL_MBOX_OP_SANITIZE) {
+		flush_cache_all();
+
+		rc = cxl_mbox_send_cmd(cxlds, cmd, NULL, 0, NULL, 0, &ret_code);
+		if (rc == 0 && ret_code != CXL_MBOX_CMD_RC_BACKGROUND)
+			flush_cache_all();
+	} else if (cmd == CXL_MBOX_OP_SECURE_ERASE) {
+		if (!skip_security && (sec_out & CXL_PMEM_SEC_STATE_LOCKED))
+			return -EINVAL;
+
+		flush_cache_all();
+
+		rc = cxl_mbox_send_cmd(cxlds, cmd, NULL, 0, NULL, 0, NULL);
+		if (rc == 0)
+			flush_cache_all();
+	} else
+		rc = -EINVAL;
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_sanitize, CXL);
+
 int cxl_mem_create_range_info(struct cxl_dev_state *cxlds)
 {
 	int rc;
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index f7cdcd33504a..db3c5eab7099 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -106,12 +106,70 @@ static ssize_t numa_node_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(numa_node);
 
+#define CXL_SEC_CMD_SIZE 32
+
+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;
+	u16 ret_code;
+	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), &ret_code);
+	if (ret_code == CXL_MBOX_CMD_RC_UNSUPPORTED)
+		return sprintf(buf, "disabled\n");
+	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");
+}
+
+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 c05dc1b8189a..ee56a4802b34 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -256,6 +256,7 @@ enum cxl_opcode {
 	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,
@@ -457,6 +458,8 @@ static inline void cxl_mem_active_dec(void)
 }
 #endif
 
+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 6da25f2e1bf9..2cea8fb33249 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -41,6 +41,8 @@
 	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
 	___C(SCAN_MEDIA, "Scan Media"),                                   \
 	___C(GET_SCAN_MEDIA, "Get Scan Media Results"),                   \
+	___C(SANITIZE, "Sanitize"),                                       \
+	___C(SECURE_ERASE, "Secure Erase"),				  \
 	___C(GET_SECURITY_STATE, "Get Security State"),			  \
 	___C(SET_PASSPHRASE, "Set Passphrase"),				  \
 	___C(DISABLE_PASSPHRASE, "Disable Passphrase"),			  \
-- 
2.37.1

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

[Jonathan Cameron]

On Wed,  3 Aug 2022 21:50:29 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Implement support for the non-pmem exclusive sanitize (aka overwrite)
> and secure erase commands, per CXL 3.0 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  
> 
> While userspace can implement entirely the wait/query mechanism for
> waiting for the sanitize to complete. Unlike persistent memory
> equivalents, there is no command to query in CXL, and as such we can
> safely just use cxlds->bg.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |  19 +++++
>  drivers/cxl/core/mbox.c                 | 105 ++++++++++++++++++++++++
>  drivers/cxl/core/memdev.c               |  58 +++++++++++++
>  drivers/cxl/cxlmem.h                    |   3 +
>  include/uapi/linux/cxl_mem.h            |   2 +
>  5 files changed, 187 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 7c2b846521f3..88631b492a11 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -52,6 +52,25 @@ Description:
>  		host PCI device for this memory device, emit the CPU node
>  		affinity for this device.
>  
> +What:		/sys/bus/cxl/devices/memX/security
> +Date:		August, 2022
> +KernelVersion:	v5.21

v6.1

> +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 db6e5a4d1f6d..06bbb760b392 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -41,6 +41,8 @@ static struct workqueue_struct *cxl_mbox_bgcmd_wq;
>  #define CXL_BGCMD(_id, sin, sout, _flags, _bgops)                              \
>  	__CXL_CMD(_id, sin, sout, _flags | CXL_CMD_FLAG_BACKGROUND, _bgops)
>  
> +static struct cxl_mem_bgcommand_ops sanitize_bgops;
> +
>  #define CXL_VARIABLE_PAYLOAD	~0U
>  /*
>   * This table defines the supported mailbox commands for the driver. This table
> @@ -71,6 +73,8 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>  	CXL_BGCMD(SCAN_MEDIA, 0x11, 0, 0, NULL),
>  	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
> +	CXL_BGCMD(SANITIZE, 0, 0, 0, &sanitize_bgops),
> +	CXL_CMD(SECURE_ERASE, 0, 0, 0),
>  	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
>  	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
>  	CXL_CMD(DISABLE_PASSPHRASE, 0x40, 0, 0),
> @@ -962,6 +966,107 @@ 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 long secs;
> +	u64 total_mem;
> +
> +	if (!cxlds)
> +		return 0;
> +
> +	total_mem = cxlds->total_bytes / CXL_CAPACITY_MULTIPLIER;
> +
> +	if (total_mem <= 16)
> +		secs = 1;
> +	else if (total_mem <= 64)
> +		secs = 10;
> +	else if (total_mem <= 256)
> +		secs = 20;
> +	else if (total_mem <= 512)
> +		secs = 40;
> +	else if (total_mem <= 1024)
> +		secs = 50;
> +	else
> +		secs = 60; /* max */
> +	return secs;
> +}
> +
> +static void sanitize_bgcmd_post(struct cxl_dev_state *cxlds, bool success)
> +{
> +	if (success)
> +		flush_cache_all();
> +}
> +
> +static struct cxl_mem_bgcommand_ops sanitize_bgops = {
> +	.conflicts = sanitize_bgcmd_conflicts,
> +	.delay = sanitize_bgcmd_delay,
> +	.post = sanitize_bgcmd_post,
> +};

blank line here.

> +/**
> + * cxl_mem_sanitize() - Send sanitation related commands to the device.
> + * @cxlds: The device data for the operation
> + * @cmd: 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 cmd)
> +{
> +	int rc;
> +	bool skip_security;
> +	u32 sec_out;
> +	u16 ret_code; /* hw */
> +
> +	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE,
> +			       NULL, 0, &sec_out, sizeof(sec_out), &ret_code);
> +	/* this may just be plain unsupported, do not error out */
> +	skip_security = (ret_code == CXL_MBOX_CMD_RC_UNSUPPORTED);

Ah. Now I'm understanding why you had the ret_code. I'd rather see this
done using the CEL so we don't send the command if not supported.

> +	if (rc && !skip_security)
> +		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 (!skip_security && (sec_out & CXL_PMEM_SEC_STATE_USER_PASS_SET))
> +		return -EINVAL;
> +
> +	if (cmd == CXL_MBOX_OP_SANITIZE) {
> +		flush_cache_all();
> +
> +		rc = cxl_mbox_send_cmd(cxlds, cmd, NULL, 0, NULL, 0, &ret_code);
> +		if (rc == 0 && ret_code != CXL_MBOX_CMD_RC_BACKGROUND)
> +			flush_cache_all();
> +	} else if (cmd == CXL_MBOX_OP_SECURE_ERASE) {
> +		if (!skip_security && (sec_out & CXL_PMEM_SEC_STATE_LOCKED))
> +			return -EINVAL;
> +
> +		flush_cache_all();
> +
> +		rc = cxl_mbox_send_cmd(cxlds, cmd, NULL, 0, NULL, 0, NULL);
> +		if (rc == 0)
> +			flush_cache_all();
> +	} else
> +		rc = -EINVAL;
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mem_sanitize, CXL);
> +
>  int cxl_mem_create_range_info(struct cxl_dev_state *cxlds)
>  {
>  	int rc;
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index f7cdcd33504a..db3c5eab7099 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -106,12 +106,70 @@ static ssize_t numa_node_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(numa_node);
>  
> +#define CXL_SEC_CMD_SIZE 32
> +
> +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;
> +	u16 ret_code;
> +	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), &ret_code);
> +	if (ret_code == CXL_MBOX_CMD_RC_UNSUPPORTED)

As above - if not supported, don't send command.

If none of this stuff is supported, hide the sysfs interface via
the is_visible callback.

> +		return sprintf(buf, "disabled\n");
> +	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");
> +}
> +
> +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 c05dc1b8189a..ee56a4802b34 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -256,6 +256,7 @@ enum cxl_opcode {
>  	CXL_MBOX_OP_SCAN_MEDIA		= 0x4304,
>  	CXL_MBOX_OP_GET_SCAN_MEDIA	= 0x4305,
>  	CXL_MBOX_OP_SANITIZE		= 0x4400,

This definition should be pulled forwards from previous patch I think.

> +	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,
> @@ -457,6 +458,8 @@ static inline void cxl_mem_active_dec(void)
>  }
>  #endif
>  
> +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 6da25f2e1bf9..2cea8fb33249 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -41,6 +41,8 @@
>  	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
>  	___C(SCAN_MEDIA, "Scan Media"),                                   \
>  	___C(GET_SCAN_MEDIA, "Get Scan Media Results"),                   \
> +	___C(SANITIZE, "Sanitize"),                                       \
> +	___C(SECURE_ERASE, "Secure Erase"),				  \
>  	___C(GET_SECURITY_STATE, "Get Security State"),			  \
>  	___C(SET_PASSPHRASE, "Set Passphrase"),				  \
>  	___C(DISABLE_PASSPHRASE, "Disable Passphrase"),			  \

Re: [RFC PATCH 0/2] cxl: BG operations and device sanitation

[Dave Jiang]

On 8/3/2022 9:50 PM, Davidlohr Bueso wrote:
> Hello,
>
> The following is a followup to some of the discussions around CXL device security
> and sanitation[0, 1]. It is marked as RFC mostly to see if the background handling
> will satisfy all users, not just sanitize/overwrite. For example there has been ideas
> to avoid command hogging the device and/or interleaving scan media regions instead
> of all in one, etc. More details in each patch, but:
>
> Patch 1 adds the required background cmd handling bits. While this is currently
> only polling, it would be good to know if there are any fundamental blockers for
> supporting irqs (beyond just background ops) between PCIe and CXL. For example,
> are there any requirements/difficulties that is not the standard MSI/MSI-X PCI
> vector allocation + devm_request_irq()? I have not looked at this into details but
> it the topic has come up in the past as delicate', iirc.
>
> Patch 2 implements the sanitation commands (overwrite and secure erase).
>
> As for testing, while I used the mock device to test the secure erase command, I
> ended up hacking up a prototype[2] 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 against Dave's cxl-security branch[2] which deals with the pmem-only bits.

 From the operational sense everything looks good to me. As for the 
polling delay on overwrite, with pre-CXL pmem on Optane, we've 
discovered that overwrite can take a long time depending on the size. 
Sometimes MANY hours if the size is really large. We just opted to 
increment the polling interval as time went on [1] instead of based on size.

[1]: 
https://elixir.bootlin.com/linux/v5.19/source/drivers/nvdimm/security.c#L434


> Thanks,
> Davidlohr
>
> [0]: https://lore.kernel.org/all/20220708172455.gi37dh3od4w5lqrd@offworld/
> [1]: https://lore.kernel.org/all/165791918718.2491387.4203738301057301285.stgit@djiang5-desk3.ch.intel.com/
> [2]: https://github.com/davidlohr/qemu/commit/64a93a5b824b59d3b547f06f7fbb1269fb4790ce
> [3]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-security
>
> Davidlohr Bueso (2):
>    cxl/mbox: Add background operation handling machinery
>    cxl/mem: Support sanitation commands
>
>   Documentation/ABI/testing/sysfs-bus-cxl |  19 ++
>   drivers/cxl/core/core.h                 |   2 +-
>   drivers/cxl/core/mbox.c                 | 304 +++++++++++++++++++++++-
>   drivers/cxl/core/memdev.c               |  58 +++++
>   drivers/cxl/core/port.c                 |   9 +-
>   drivers/cxl/cxl.h                       |   8 +
>   drivers/cxl/cxlmem.h                    |  65 ++++-
>   drivers/cxl/pci.c                       |   3 +-
>   drivers/cxl/pmem.c                      |   5 +-
>   drivers/cxl/security.c                  |  13 +-
>   include/uapi/linux/cxl_mem.h            |   2 +
>   11 files changed, 461 insertions(+), 27 deletions(-)
>
> --
> 2.37.1
>

Re: [RFC PATCH 0/2] cxl: BG operations and device sanitation

[Davidlohr Bueso]

On Thu, 04 Aug 2022, Dave Jiang wrote:

>From the operational sense everything looks good to me. As for the
>polling delay on overwrite, with pre-CXL pmem on Optane, we've
>discovered that overwrite can take a long time depending on the size.
>Sometimes MANY hours if the size is really large. We just opted to
>increment the polling interval as time went on [1] instead of based on
>size.

Thanks for having a look. Sure, we can do that, I have no particular attachment
to doing it based on size (it's just the way it occured to me). I am curious,
though: While regardless of size vs time based estimates, are the numbers
expected to be similar for volatile regions? All these numbers being from
nvdimm DSM docs.

Thanks,
Davidlohr

Re: [RFC PATCH 0/2] cxl: BG operations and device sanitation

[Dave Jiang]

On 8/4/2022 1:07 PM, Davidlohr Bueso wrote:
> On Thu, 04 Aug 2022, Dave Jiang wrote:
>
>> From the operational sense everything looks good to me. As for the
>> polling delay on overwrite, with pre-CXL pmem on Optane, we've
>> discovered that overwrite can take a long time depending on the size.
>> Sometimes MANY hours if the size is really large. We just opted to
>> increment the polling interval as time went on [1] instead of based on
>> size.
>
> Thanks for having a look. Sure, we can do that, I have no particular 
> attachment
> to doing it based on size (it's just the way it occured to me). I am 
> curious,
> though: While regardless of size vs time based estimates, are the numbers
> expected to be similar for volatile regions? All these numbers being from
> nvdimm DSM docs.

I don't either. Just pointing out that's what we did with the Optane 
stuff. I think that the volatile devices (DRAM?) would probably be a lot 
faster when it comes to writes. So maybe won't take as long. And also 
perhaps smaller in size in the immediate future? Just guessing.


>
> Thanks,
> Davidlohr